[FFmpeg-devel] [PATCH] MLP/TrueHD decoder

[Michael Niedermayer]

Hi

On Thu, Nov 08, 2007 at 01:33:46PM +0000, Ian Caulfield wrote:
> > [...]
> > > +    if (m->max_channel[substr] >= MAX_CHANNELS
> > > +        || m->max_matrix_channel[substr] >= MAX_CHANNELS)
> > > +    {
> > > +        if (substr > 0) {
> >
> > > +            av_log(m->avctx, AV_LOG_INFO,
> > > +                   "Substream %d contains more channels than the maximum "
> > > +                   "supported by this decoder (%d). Only substreams up to %d "
> > > +                   "will be decoded. Please provide a sample of this file "
> > > +                   "to the FFmpeg development list.\n",
> > > +                   substr, MAX_CHANNELS, substr - 1);
> > > +            m->max_decoded_substream = substr - 1;
> > > +            m->avctx->channels = m->max_channel[substr - 1] + 1;
> > > +        } else {
> >
> > > +            av_log(m->avctx, AV_LOG_INFO,
> > > +                   "This stream contains more channels than the maximum "
> > > +                   "supported by this decoder (%d). Please provide a sample "
> > > +                   "of this file to the FFmpeg development list.\n",
> > > +                   MAX_CHANNELS);
> > > +            return -1;
> > > +        }
> > > +    }
> >
> > so after this max_channel and max_matrix_channel are invalid, is there
> > anything that stops them from being used? if no then this
> > check is as good as no check at all
> 
> The first case sets max_decoded_substream so that these parameters
> won't be used, while the second case returns an error - however I seem

i dont see how returing an error will prevent the use of the invalid value
in future frames
leaving variables in the context which are used to prevent array overflows
at invalid values is fragile, what if the code is changed slightly in the
future and they do become used? (that is assuming they wouldnt be used
currently already in some cases)


[...]
> > [...]
> > > +    if ((show_bits_long(&gb, 31) << 1) == 0xf8726fba) {
> >
> > hmmmm
> > the <<1 is useless, shift your constant!
> 
> The original reasoning was that we're looking at only the top 31 bits
> of a 32-bit value, and shifting the constant would confuse what that
> value was. Is (0xf8726fba >> 1) acceptable, or should I use a comment?

(0xf8726fba >> 1) is ok


> 
> > [...]
> > > +    if (!m->in_sync)
> > > +        return length * 2;
> >
> > please explain this, didnt your parser already drop all "not in sync" parts?
> 
> in_sync is probably a bad name - the parser does drop frames when it's
> not in sync. However, the sync frames contain decoding parameters
> needed for all packets until the next sync frame, so if there's an
> error decoding the sync frame, the decoder can't decode any further
> packets until another one comes along, regardless of whether the
> parser is synced up.

do the parameters change between syncs in practice? or would using the
parameters of the last undamaged sync work as well?

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071112/50360f92/attachment.pgp>