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

[Ian Caulfield]

> [...]
> > +    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
to have missed resetting the in_sync variable. Will fix.

> also if i really didnt miss something and this can lead to writing file
> data over the end of arrays then please recheck your code, yes all of it
> for similar issues

Will do.


> [...]
> > +    for (ch = 0; ch <= m->max_matrix_channel[substr]; ch++) {
> > +        m->ch_assign[substr][ch] = get_bits(gbp, 6);
> > +        av_log(m->avctx, AV_LOG_DEBUG, "ch_assign[%d][%d] = %d\n",
> > +               substr, ch, m->ch_assign[substr][ch]);
> > +    }
>
> this is not used anywhere

Not yet - I've yet to find a stream that uses these fields in a
non-1:1 fashion, so I'm not 100% sure on their usage. I'll add some
checking in to fault if ch_assign[ch] != ch.


> [...]
> > +    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?

> [...]
> > +    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.

Also, is the parser patch OK to commit?

Ian