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

Michael Niedermayer michaelni
Sat Dec 1 23:08:57 CET 2007


On Thu, Nov 29, 2007 at 10:55:56AM +0000, Ian Caulfield wrote:
> On Nov 12, 2007 1:25 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > Hi
> >
> >
> > On Thu, Nov 08, 2007 at 01:33:46PM +0000, Ian Caulfield wrote:
> > > >
> > > > 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)
> 
> As well as an error being returned, the "have parameters" flag in the
> context is cleared, causing all packets to be rejected until we get a
> (valid) sync packet.

there is no variable, constant or otherwise called "have parameters" or have*


> > >
> > > > [...]
> > > > > +    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?
> 
> Unfortunately so - the sync packets contain not just things like "6
> channels, 48KHz", but also encoding parameters which change throughout
> the stream to improve compression - an earlier version of the decoder
> had a bug where one of the parameters wasn't reset properly and it
> sounded awful. I think trying to fudge by on the last set of
> parameters is likely to produce nasty output...

well, but some sound will have to be placed where decoding fails ...
from the outside of the decoder we are limited to things like putting
silence there or repeating the last valid output
does this sound better?


[...]

> +    int         out_buf_remaining;

see previous review


[...]
> +    uint8_t     restart_header_present[MAX_SUBSTREAMS];

see previous review


[...]
> +static uint8_t mlp_restart_checksum(const uint8_t *buf, unsigned int bit_size)
> +{
> +    int i;
> +    int num_bytes = (bit_size + 2) / 8;
> +
> +    uint8_t crc = crc_1D[buf[0] & 0x3f];
> +    crc = av_crc(crc_1D, crc, buf + 1, num_bytes - 2);
> +    crc ^= buf[num_bytes - 1];
> +
> +    for (i = 0; i < (bit_size + 2) % 8; i++) {
> +        if (crc & 0x80)
> +            crc = (crc << 1) ^ 0x1D;
> +        else
> +            crc = crc << 1;

see previous review

remainder not reviewed

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

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- 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/20071201/57f5fa74/attachment.pgp>



More information about the ffmpeg-devel mailing list