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

[Michael Niedermayer]

On Thu, Jan 24, 2013 at 01:18:51PM +0000, Tim Nicholson wrote:
> 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.

applied

thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130125/917bc3a1/attachment.asc>