[FFmpeg-devel] [PATCH] MLP/TrueHD decoder
Michael Niedermayer
michaelni
Sun Oct 14 22:11:07 CEST 2007
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 :)
>
> Ian
[...]
> +/** Get a number of bits from a bitstream, updating a running CRC in the process */
> +
> +static av_always_inline unsigned int get_bits_crc(GetBitContext *gbp, int bit_count, uint8_t *crctab, uint8_t *crc)
> +{
> + uint8_t crcval = *crc;
> + unsigned int data;
> + data = get_bits_long(gbp, bit_count);
> + while (bit_count > 8) {
> + crcval = crctab[crcval] ^ ((data >> (bit_count - 8)) & 0xff);
> + bit_count -= 8;
> + }
> + *crc = (crcval << bit_count)
> + ^ crctab[crcval >> (8 - bit_count)]
> + ^ (data & ((1 << bit_count) - 1));
> + return data;
> +}
please use the normal get_bits*() and av_crc() once over the whole data
> +
> +/** 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
> +
> +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
[...]
> + 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
[...]
> +/** 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?
[...]
> + int32_t sample, filtered;
> + sample = read_huff(m, gbp, substr, ch);
> + filtered = filter_sample(m, substr, ch, sample);
declaration and initalization can be merged
[...]
> + 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
[...]
> + 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;
[...]
> +/**
> + * 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?
[...]
> + for (substr = 0; substr <= m->max_decoded_substream; substr++) {
> + init_get_bits(&gb, buf, m->substream_data_len[substr] * 16);
> +
> + m->blockpos[substr] = 0;
> + do {
> + if (get_bits1(&gb)) {
> + if (get_bits1(&gb)) {
> + /* A restart header should be present */
> + if (read_restart_header(m, &gb, substr) < 0)
> + goto error;
> + if (!m->restart_header_present[substr])
> + av_log(m->avctx, AV_LOG_INFO, "Restart header present but not indicated in block header.\n");
> + m->restart_seen[substr] = 1;
> + m->restart_header_present[substr] = 0;
> + } else if (m->restart_header_present[substr]) {
> + av_log(m->avctx, AV_LOG_INFO, "Restart header indicated in block header but not present.\n");
> + if (!m->restart_seen[substr])
> + goto error;
> + }
> +
> + if (read_decoding_params(m, &gb, substr) < 0)
> + goto error;
> + }
> +
> + if (read_block_data(m, &gb, substr) < 0)
> + goto error;
> +
> + m->blockpos[substr] += m->blocksize[substr];
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!
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/20071014/e7c80825/attachment.pgp>
More information about the ffmpeg-devel
mailing list