[FFmpeg-devel] [PATCH] lavc/dnxhddata: fix bitrates for cid 1251 and 1252 in cid table

[Matthieu Bouron]

On Thu, Jan 24, 2013 at 10:27 AM, Tim Nicholson <nichot20 at yahoo.com> wrote:
> On 22/01/13 15:31, Matthieu Bouron wrote:
>> On Tue, Jan 22, 2013 at 4:04 PM, Tim Nicholson <nichot20 at yahoo.com> wrote:
>>> On 22/01/13 13:34, Matthieu Bouron wrote:
>>>> On Tue, Jan 22, 2013 at 2:18 PM, Tim Nicholson <nichot20 at yahoo.com> wrote:
>>>>> On 21/01/13 20:14, Matthieu Bouron wrote:
>>>>>> ---
>>>>>>
>>>>>> Hi there,
>>>>>>
>>>>>> According to SMPTE S2019-1, bitrates should be:
>>>>>>   - 180Mbps for cid 1251 at 50fps,
>>>>>>   - 120Mbps for cid 1252 at 50fps.
>>>>>
>>>>> Looking at Annex F in my SMPTE S2019-1-2008 suggests 25fps figures are
>>>>> wrong too..
>>>>>
>>>>> cid 1251 is { 90, 80, 180, 220 }
>>>>> cid 1252 is { 60, 60, 120, 145 }
>>>>>
>>>>
>>>> The 75Mbps value "may" refer to the 29.97fps profile (which is
>>>> declared here: http://www.avid.com/static/resources/FR/documents/dnxhd.pdf
>>>> but not in the S2019). The single 60Mbps value refers here to the
>>>> 23.976fps and 25fps profiles (this is how i understand it).
>>>
>>>
>>> So the original values are based on Avid's spec (although the 75 should
>>> be 72), and you want to change some of them to conform to SMPTE VC3 spec?
>>>
>>> It is unhelpful that the values sources are not documented, since you
>>> were quoting SMPTE I assumed the values that I suggested also needed a
>>> tweak were for 25fps since 29.97fps does not exist in the cid's under
>>> discussion...
>>>
>>> What is your thinking behind having values that appear to be drawn from
>>> two different specification sources? Is this wise?
>>
>> I do not know where the actual values came from, maybe Baptiste can
>> comment on this.
>> The only document i actually trust is the SMPTE S2019-1 and the avid
>> document i linked was found by random googling when i was trying to
>> look for a 75Mbps 720p profile.
>>
>> I tried to correct thoses values to be more consitent with the SMPTE
>> spec since the smpte bitrate (180) is used for the cid 1250 50fps but
>> not for the cid 1251 50fps (175 instead of 180).
>>
>
> I have no problem with that. I think my greater concern is that there
> are also other figures in cid's 1251 & 1252 which are either wrong for
> 25fps, or relate to the rate for a frame rate that is not listed in the
> VC3 Annex, while one that is listed is not provided for. This is
> inconsistent with the approach for other cid's in the list, see below.
>
>
>> IMHO, mixing the missing dnx profiles with the vc3 ones sounds ok
>> untill those dnx profiles are effectively supported by ffmpeg.
>
> I think that is probably OK provided it is clear where the figures come
> from, and which values in the array relate to what.
>
> Looking at other cid's, for example 1237, the SMPTE Annexe only lists:-
> 23.976, 25, 50 and 59.94 data rates whereas Avid list:-
> 23.976/24, 25, 29.97, 50 59.94/60
>
> The ffmpeg CIDEntry is:-
>
> { 115, 120, 145, 240, 290 }
>
> which is basically the SMPTE values with the extra 29.97 figure (145)
> added in in between the 25 and 50 values. This seems to me to be a
> sensible way of "mixing the missing dnx profiles".
>
> However this suggests that you supposition that 'The 75Mbps value "may"
> refer to the 29.97fps profile' is not correct. Following the above
> example I would have expected:-
>
> { 90, 90, 110, 180, 220 }
> and
> { 60, 60, 75, 120, 145 }
>

>From what i understand, 60Mbps and 90Mbps are not added twice to the array
since a single value can be used for comparaison to choose the right cid to use.
This is why i supposed that the 75Mbps refers to the 29.97 framerate.

The patch i prepared to associate bitrates to framerates will add the
redundant values
as follow:

bitrates = { 60, 60, 75, 120, 145 },
framerates = { { 24000, 1001 }, { 25, 1 }, { 30000, 1001 }, { 50, 1 },
{ 60000 / 1001 } },

> I note however that cids 1241-43 only have 2 entries in the array as per
> both the Annex and Avid, however the fact that the array is variable
> length makes it impossible to relate the array index absolutely to a
> frame rate, and confirm or deny your supposition.
>
> According to the header file this array is a "Helpher to choose
> variants, rounded to nearest 5Mb/s" so how is it actually used?
> Would not the 5 entry array be more appropriate?

If only two entries are listed in both the SMPTE and AVID specs, only
two entries should be listed in the bitrates and the future framerates array.

bitrates = { 185, 220 }
framerates = { { 25, 1 }, { 30000, 1001 } }

>
> In summary, what I am really asking is,"does your patch address all the
> errors/inconsistencies in this area, and would it not be better to do it
> in one hit rather than dribs and drabs?"

IMHO, the patch addresses "all" the inconsistencies i found in regards of
the way the actual code works. Redudant value like 60Mbps or 90Mbps can
be added in the same time as the framerates array.

I'll send the patch to the ml as soon as i can (likely to be tonight).

Regards,
Matthieu

>>
>> Matthieu
>>
>> [...]
>
>
> --
> Tim
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel