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

[Michael Niedermayer]

On Wed, Oct 17, 2007 at 01:31:19PM +0100, Ian Caulfield wrote:
> New version of mlp parser patch attached.
> 
> On 15/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Mon, Oct 15, 2007 at 12:26:02PM +0100, Ian Caulfield wrote:
> > > New parser patch attached, I've tried to addess everyone's comments.
> > [...]
> > > +static int truehd_channels(int chanmap)
> >
> > for(i=0; i<13; i++)
> >     channels += chan_count[i] * ((chanmap>>i)&1);
> >
> 
> Fixed
> 
> >
> > [...]
> > > +    // The MLP crc is calculated differently to av_crc, hence the need to
> > > +    // XOR in the last two bytes instead of including them in the CRC
> >
> > i say it again MLP does NOT calculate CRCs differently it calculates them
> > incorrect
> > this is like saying r*3 is a different way of finding the circumference of the
> > circle, it is not differnt its just wrong
> >
> > CRCs are shortened cyclic codes (see any book about CRCs, maybe even
> > wikipedia) what MLP stores are NOT shortened cyclic codes hence not CRCs!
> > that is unless AV_RL16(buf+24) or AV_RL16(buf+26) is a constant
> >
> 
> Renamed to checksum
> 
> > > +    if (pc->bytes_left == 0) {
> > > +        // Find length of this packet
> > > +
> > > +        for (i = 0; (pc->state & 0x10000) == 0 && i < buf_size; i++)
> > > +        {
> > > +            pc->state = (pc->state << 8) | buf[i];
> > > +        }
> > > +
> > > +        if ((pc->state & 0x10000) == 0)
> > > +        {
> > > +            ff_combine_frame(&pc->pc, END_NOT_FOUND, &buf, &buf_size);
> > > +            return buf_size;
> > > +        }
> >
> > the {} placement is inconsistant
> 
> Fixed
> 
> >
> > > +        init_get_bits(&bits, buf + 4, (buf_size - 4) * 8);
> > > +        if (ff_mlp_read_major_sync(NULL, &mh, &bits, buf + 4, buf_size - 4) < 0) {
> >
> > please dont pass 2 redundant things, here its a pointer to the buffer
> > and a GetBitContext
> >
> 
> Fixed
> 
> >
> > > +            pc->in_sync = 0;
> > > +            pc->state = 0;
> > > +            return -1;
> >
> > duplicate, a goto no_sync could be used ...
> >
> 
> Fixed
> 
> Ian

[...]
> +    if (!pc->in_sync) {
> +        // Not in sync - find a major sync header
> +
> +        for (i = 0; i < buf_size; i++) {
> +            pc->state = (pc->state << 8) | buf[i];
> +            if ((pc->state & 0xfffffffe) == 0xf8726fba)
> +            {
> +                pc->in_sync = 1;
> +                break;
> +            }

{} placement is inconsistant
first its 
if() {
then
if()
{


[...]

> +            ff_combine_frame(&pc->pc, END_NOT_FOUND, &buf, &buf_size);

i think using the same name for pc and pc->pc is not good



[...]
> +        // First nibble of a frame is a parity check of the first few nibbles
> +        // Only check when this isn't a sync frame - syncs have a checksum
> +
> +        parity_bits = 0;
> +        for (p = 0; p < 4; p++)
> +            parity_bits ^= buf[p];
> +        for (i = 0; i < pc->num_substreams; i++) {
> +            parity_bits ^= buf[p];
> +            parity_bits ^= buf[p+1];
> +
> +            if (buf[p] & 0x80) {
> +                parity_bits ^= buf[p+2];
> +                parity_bits ^= buf[p+3];
> +                p += 2;
> +            }
> +            p += 2;
> +        }
> +
> +        if ((((parity_bits >> 4) ^ parity_bits) & 0xF) != 0xF) {
> +            av_log(NULL, AV_LOG_INFO, "mlpparse: parity check failed\n");
> +            goto lost_sync;
> +        }

its not the parsers job to check frames
the only reason for a parser to not return part of the input is if it cannot
determine the frame boundaries (for example if the frame length from some
header is invalid/damaged)


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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20071022/eb77de21/attachment.pgp>