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

[Ian Caulfield]

Updated decoder patch attached.

On 14/10/2007, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Fri, Oct 12, 2007 at 01:29:46PM +0100, Ian Caulfield wrote:
>
>
> please use the normal get_bits*() and av_crc() once over the whole data
>

Normal get_bits functions used - a checksum function has been added to
do an av_crc() over the full bytes of data, and then process the last
few bits bit-by-bit.

>
> > +
>
> > +/** Calculate an 8-bit CRC.
> > + *  The way MLP calculates CRCs is different to the standard implementation */
>
> no the way MLP calculates CRCs is wrong, it destroys many of the good
> properties of crcs, for example there always exists a 2 bit error which wont
> be detected with the MLP design no matter how good the underlaying crc is in
> detecting n bit errors
> they could as well just have summed all bytes for checksumming
>

Renamed to checksum

>
> > +
> > +static uint8_t mlp_crc8(AVCRC *crctab, uint8_t crc, uint8_t *buf, int buf_size)
> > +{
> > +    crc = av_crc(crctab, 0, &crc, 1);
> > +    crc = av_crc(crctab, crc, buf, buf_size - 1);
> > +    crc ^= buf[buf_size-1];
> > +    return crc;
> > +}
>
> the first av_crc() call can be avoided as the initial crc value is a constant
>

Fixed

>
> [...]
> > +    for (ch = m->min_channel[substr]; ch <= m->max_channel[substr]; ch++) {
> > +        m->filter_order[ch][0] = 0;
> > +        m->filter_order[ch][1] = 0;
> > +        m->filter_coeff_q[ch][0] = 0;
> > +        m->filter_coeff_q[ch][1] = 0;
> > +
> > +        memset(m->filter_coeff[ch], 0, sizeof(m->filter_coeff[ch]));
> > +        memset(m->filter_state[ch], 0, sizeof(m->filter_state[ch]));
> > +
> > +        /* Default audio coding is 24-bit raw PCM */
> > +        m->huff_offset[ch] = 0;
> > +        m->codebook[ch] = 0;
> > +        m->huff_lsbs[ch] = 24;
> > +    }
>
> there seem to be no checks for the validity of the index, if so this is
> BAD
>

Checks added for the various min/max channel parameters.

>
> [...]
> > +/** Generate a PCM sample using the prediction filters and a residual value
> > + *  read from the data stream, and update the filter state.
> > + */
> > +
> > +static int filter_sample(MLPDecodeContext *m, int substr,
> > +                         int channel, int32_t residual)
> > +{
> > +    int i;
> > +    int64_t accum = 0;
> > +    int32_t result;
> > +
> > +    /* TODO: Move this code to DSPContext? */
> > +
> > +    for (i = 0; i < m->filter_order[channel][0]; i++)
> > +        accum += (int64_t)m->filter_state[channel][0][i] *
> > +                 m->filter_coeff[channel][0][i];
> > +
> > +    for (i = 0; i < m->filter_order[channel][1]; i++)
> > +        accum += (int64_t) m->filter_state[channel][1][i] *
> > +                 m->filter_coeff[channel][1][i];
>
> do these really need 64bit?
>

filter_state is up to 24-bit, filter_coeff is 16-bit, so output is
theoretically 40-bits. In practice it should be possible to use a
32-bit version for some blocks - I haven't addressed this yet, but it
could be a future optimisation.

>
> [...]
> > +            int32_t sample, filtered;
> > +            sample = read_huff(m, gbp, substr, ch);
> > +            filtered = filter_sample(m, substr, ch, sample);
>
> declaration and initalization can be merged
>

Fixed

>
> [...]
> > +        m->sample_buffer[i][maxchan+1] = ((int8_t)((seed >> 15) & 0xff)) << m->noise_shift[substr];
> > +        m->sample_buffer[i][maxchan+2] = ((int8_t)((seed >> 7) & 0xff)) << m->noise_shift[substr];
>
> the &0xff shouldnt be neede if you cast to int8_t
>

Fixed

>
> [...]
> > +        m->noise_buffer[i] = noise_table[(seed >> 15) & 0xff];
> > +
> > +        tmp = seed >> 15;
> > +        seed = (seed << 8) ^ tmp ^ (tmp << 5);
> > +        seed &= 0x7fffff;
>
> tmp = seed >> 15;
> m->noise_buffer[i] = noise_table[tmp];
> seed = (seed << 8) ^ tmp ^ (tmp << 5);
> seed &= 0x7fffff;
>

Changed

>
> [...]
> > +/**
> > + * Read an access unit from the stream.
> > + * Returns -1 on error, 0 if not enough data is present in the input stream
> > + * otherwise returns the number of bytes consumed.
> > + */
> > +
> > +static int read_access_unit(MLPDecodeContext *m, uint8_t *buf, int buf_size)
> > +{
> > +    GetBitContext gb;
> > +    int length, substr;
> > +    int substream_start;
> > +
> > +    if (buf_size < 2)
> > +        return 0;
> > +
> > +    if (m->out_buf_remaining < 6 * 40 * 4)
> > +        return 0;
>
> isnt that an error?
> also are you sure that no more than 6 * 40 * 4 can be written?
>

I've changed this to check against the actual amount that will be
written, and to return an error.

>
> is there anything which limits the size of blockpos, it seems used in many
> loops and would lead to exploitable bugs if it can get larger than the arrays
> also if its not checked anywhere please recheck everything for similar bugs!

I've added a check before any data is written to make sure that the
pos doesn't go beyond the amount of audio data that should be
generated by that block.

Ian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mlpdec.patch
Type: text/x-diff
Size: 41702 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071017/a4e4d4ee/attachment.patch>