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

[Tim Nicholson]

On 24/01/13 13:02, Matthieu Bouron wrote:
> On Thu, Jan 24, 2013 at 1:52 PM, Tim Nicholson <nichot20 at yahoo.com> wrote:
>> On 24/01/13 10:44, Matthieu Bouron wrote:
>>> 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:
>>> [...]
>>>
>>> >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.
>>
>> True, I hadn't noticed that the current code didn't check that the
>> bitrate was valid for the requested framerate in order to decide the
>> cid, only that the bitrate was one of those allowed for the
>> framesize/interlace/bit depth.
>>
>>> 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 } },
>>>
>>> [..]
>>>> 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).
>>>
>> Ahh, I did not know that there was a second part yet to come that
>> effectively sorted all the bits out in a consistent manner as I was
>> suggesting...
>>
>> Does this mean that specifying the incorrect bitrate for the requested
>> framerate will no longer be possible, or at least flagged as incorrect?
> 
> Associate an invalid framerate would no longer be possible.
> 
> The upcoming patch will comme in two parts:
>   - associate bitrates with framerates and add a bitrate check in the
> ff_dnxhd_find_cid function,
>   - print valid dnxhd profiles when codec parameters are invalid.
> 
>>
>> Seems like my issues are nothing to do with your patch(es) but the
>> current implementation, which I only became aware of when looking at
>> your patch.
>>

...and which you plan to fix in the upcoming patch but in a slightly
different way to how I was thinking.

Michael, I therefore have no objections to this patch.


-- 
Tim