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

[Michael Niedermayer]

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)
> +{
> +    int channels = 0;
> +
> +    if (chanmap & 0x0001)   // L,R
> +        channels += 2;
> +    if (chanmap & 0x0002)   // C
> +        channels += 1;
> +    if (chanmap & 0x0004)   // LFE
> +        channels += 1;
> +    if (chanmap & 0x0008)   // Ls,Rs
> +        channels += 2;
> +    if (chanmap & 0x0010)   // Lvh,Rvh
> +        channels += 2;
> +    if (chanmap & 0x0020)   // Lc,Rc
> +        channels += 2;
> +    if (chanmap & 0x0040)   // Lrs,Rrs
> +        channels += 2;
> +    if (chanmap & 0x0080)   // Cs
> +        channels += 1;
> +    if (chanmap & 0x0100)   // Ts
> +        channels += 1;
> +    if (chanmap & 0x0200)   // Lsd,Rsd
> +        channels += 2;
> +    if (chanmap & 0x0400)   // Lw,Rw
> +        channels += 2;
> +    if (chanmap & 0x0800)   // Cvh
> +        channels += 1;
> +    if (chanmap & 0x1000)   // LFE2
> +        channels += 1;
> +
> +    return channels;
> +}

for(i=0; i<13; i++)
    channels += chan_count[i] * ((chanmap>>i)&1);


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


[...]
> +        if (i < 0)
> +            pc->bytes_left -= -i;

-= - -> +=

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


[...]
> +        if ((((parity_bits >> 4) ^ parity_bits) & 0xF) != 0xF) {
> +            av_log(NULL, AV_LOG_INFO, "mlpparse: parity check failed\n");
> +            pc->in_sync = 0;
> +            pc->state = 0;
> +            return -1;
> +        }
> +    } else {
> +        MLPHeaderInfo mh;
> +

> +        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


> +            pc->in_sync = 0;
> +            pc->state = 0;
> +            return -1;

duplicate, a goto no_sync could be used ...


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

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/20071015/44cf00f0/attachment.pgp>