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

[Michael Niedermayer]

Hi

On Fri, Oct 12, 2007 at 01:29:46PM +0100, Ian Caulfield wrote:
> Hi all,
> 
> I've been working for a while on reverse engineering the MLP audio
> stream format, and writing my own decoder. I've finally got to a point
> where I have something ready for review. I'm going to try to get the
> documentation I've produced on the format up on the multimediawiki as
> well.
> 
> I've attached the code as two patches - the first adds a parser, the
> second the actual decoder.
> 
> Current caveats:
> 
>  - The decoder is limited by a #define to a maximum of six channels. I
> imagine that 7.1 streams will be more-or-less a case of changing the
> #defines, but I'd like to see a sample stream first to check.
>  - Surround files are output with the channels in codec order (L, R,
> C, LFE, Ls, Rs for 5.1). There was some discussion on the list earlier
> on adding this kind of code to libavcodec instead of individual
> codecs, so I've left out reordering code for now.
>  - Funky MLP stuff, like having some channels at 96KHz and some at
> 48KHz isn't supported
>  - Blu-Ray TrueHD streams aren't supported. Apparently these consist
> of an AC3 base layer, plus a TrueHD layer with residual data or
> somesuch in. Anyone have any samples?
> 
> Extracting a two-channel downmix from a 5.1 stream is supported using
> avctx->request_channels
> 
> Comments welcome :)
[...]
> +static inline int mlp_samplerate(int in)
> +{
> +    int rate;
> +
> +    if (in == 0xF)
> +        return 0;
> +
> +    if (in & 8)
> +        rate = 44100;
> +    else
> +        rate = 48000;
> +
> +    rate <<= in & 7;
> +
> +    return rate;
> +}

this doesnt look speed critical, so why is it inline?


[...]
> +    if (buf_size < 28) {
> +        av_log(log, AV_LOG_ERROR, "Packet too short, unable to read major sync\n");
> +        return -1;
> +    }
> +
> +    if (!crc_init)
> +    {
> +        av_crc_init(crc_2D, 0, 16, 0x002D, sizeof(crc_2D));
> +        crc_init = 1;
> +    }

the {} are placed inconsistant



> +
> +    crc = av_crc(crc_2D, 0, buf, 24) ^ AV_RL16(buf+24);
> +    if (crc != AV_RL16(buf+26))
> +    {
> +        av_log(log, AV_LOG_ERROR, "Major sync info header CRC error\n");
> +        return -1;
> +    }

i would rather write
crc = av_crc(crc_2D, 0, buf, 24);
if (crc != (AV_RL16(buf+26) ^ AV_RL16(buf+24)))



> +
> +    if (get_bits_long(gbp, 24) != 0xf8726f) /* Sync words */
> +        return -1;
> +

> +    mh->stream_type = get_bits(gbp, 8);
> +    if (mh->stream_type != 0xba && mh->stream_type != 0xbb)
> +        return -1;
> +
> +    if (mh->stream_type == 0xbb) {
> +        mh->group1_bits = mlp_quants[get_bits(gbp, 4)];
> +        mh->group2_bits = mlp_quants[get_bits(gbp, 4)];
> +
> +        ratebits = get_bits(gbp, 4);
> +        mh->group1_samplerate = mlp_samplerate(ratebits);
> +        mh->group2_samplerate = mlp_samplerate(get_bits(gbp, 4));
> +
> +        skip_bits(gbp, 11);
> +
> +        mh->channels_mlp = get_bits(gbp, 5);
> +    } else {
> +        mh->group1_bits = 24; // TODO: Is this information actually conveyed anywhere?
> +        mh->group2_bits = 0;
> +
> +        ratebits = get_bits(gbp, 4);
> +        mh->group1_samplerate = mlp_samplerate(ratebits);
> +        mh->group2_samplerate = 0;
> +
> +        skip_bits(gbp, 8);
> +
> +        mh->channels_thd_stream1 = get_bits(gbp, 5);
> +
> +        skip_bits(gbp, 2);
> +
> +        mh->channels_thd_stream2 = get_bits(gbp, 13);
> +    }

if bb
else if ba
else return -1

[...]
> +static int mlp_parse(AVCodecParserContext *s,
> +                     AVCodecContext *avctx,
> +                     const uint8_t **poutbuf, int *poutbuf_size,
> +                     const uint8_t *buf, int buf_size)
> +{
> +    GetBitContext bits;
> +    MLPParseContext *pc = s->priv_data;
> +    int bytes_read;
> +    int sync_present;
> +    uint8_t parity_bits;
> +    int i, p, offset = 0;
> +
> +    *poutbuf_size = 0;
> +
> +presync:
> +    if (!pc->in_sync)
> +    {
> +        for (i = 0; i < buf_size; i++) {
> +            pc->state = (pc->state << 8) | buf[i];
> +            if (pc->state == 0xf8726fba || pc->state == 0xf8726fbb)
> +            {
> +                if (i < 7)
> +                {
> +                    memmove(pc->buf, pc->buf + pc->buf_size - (7 - i), (7 - i));
> +                    pc->buf_size = 7 - i;
> +                } else {
> +                    pc->buf_size = 0;
> +                }
> +                memcpy(pc->buf + pc->buf_size, buf + i - 7, 8 - pc->buf_size);

in the i<7 case buf + i - 7 will point outside of the array

also the parser looks messy, maybe using ff_combine_frame() would help?

ill review the decoder patch later

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

There will always be a question for which you do not know the correct awnser.
-------------- 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/20071013/d4afea8d/attachment.pgp>