[FFmpeg-devel] [PATCH] MLP/TrueHD decoder
Ian Caulfield
ian.caulfield
Wed Oct 17 17:37:16 CEST 2007
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>
More information about the ffmpeg-devel
mailing list