[FFmpeg-devel] [PATCH] MLP/TrueHD Decoder - 2nd try

Ramiro Polla ramiro
Tue Jul 1 02:55:39 CEST 2008


Hello Michael,

Michael Niedermayer wrote:
> On Sun, Jun 29, 2008 at 12:41:17PM +0100, Ramiro Polla wrote:
>> Hello,
>>
> [...]
>> mlpdec.c is Ian's MLP/TrueHD decoder with a few modifications as can be 
>> seen in the SoC tree.
>>
> 
>> Valgrind doesn't complain about invalid writes on a bunch of trashed files 
>> and some manually edited files that pass the checks as well.
> 
> I assume that you disabled all checksum checks for these tests?

Oops. A bunch of more tests run with checksums disabled. Still no 
problem seen.

> [...]
>> static const char* sample_message =
>>     "Please file a bug report following the instructions at "
>>     "http://ffmpeg.mplayerhq.hu/bugreports.html and include "
>>     "a sample of this file.";
> 
> Dont we already have something like that somewhere? Or was that another
> unfinished patch, either way above can be shared wth other codecs ...

IIRC it's in Robert's AAC patch. I haven't checked the archives to make 
sure though. Anyways I think it's not in SVN yet.

>> typedef struct MLPDecodeContext {
>>     AVCodecContext *avctx;
>>
>>     //! Do we have valid stream data read from a major sync block?
>>     uint8_t     params_valid;
>>
>>     //! Number of substreams contained within this stream
>>     uint8_t     num_substreams;
>>
>>     //! Index of the last substream to decode - further substreams are skipped
>>     uint8_t     max_decoded_substream;
>>
>>     //! Number of PCM samples contained in each frame
>>     int         access_unit_size;
>>     //! Next power of two above the number of samples in each frame
>>     int         access_unit_size_pow2;
>>
>>     //! For each substream, whether a restart header has been read
>>     uint8_t     restart_seen[MAX_SUBSTREAMS];
> 
> I think it would be more readable if the comments where aligned to the
> right of the variables

And have really long lines? I usually don't like these comments anyways, 
so I can just change it to what people on the list find best.

>>     //@{
>>     /** Restart header data */
>>     //! The sync word used at the start of the last restart header
>>     uint16_t    restart_sync_word[MAX_SUBSTREAMS];
>>
>>     //! The index of the first channel coded in this substream
>>     uint8_t     min_channel[MAX_SUBSTREAMS];
>>     //! The index of the last channel coded in this substream
>>     uint8_t     max_channel[MAX_SUBSTREAMS];
>>     //! The number of channels input into the rematrix stage
>>     uint8_t     max_matrix_channel[MAX_SUBSTREAMS];
>>
>>     //! The left shift applied to random noise in 0x31ea substreams
>>     uint8_t     noise_shift[MAX_SUBSTREAMS];
>>     //! The current seed value for the pseudorandom noise generator(s)
>>     uint32_t    noisegen_seed[MAX_SUBSTREAMS];
>>
>>     //! Does this substream contain extra info to check the size of VLC blocks?
>>     uint8_t     data_check_present[MAX_SUBSTREAMS];
>>
>>     //! Bitmask of which parameter sets are conveyed in a decoding parameter block
>>     uint8_t     param_presence_flags[MAX_SUBSTREAMS];
>> #define PARAM_BLOCKSIZE     (1 << 7)
>> #define PARAM_MATRIX        (1 << 6)
>> #define PARAM_OUTSHIFT      (1 << 5)
>> #define PARAM_QUANTSTEP     (1 << 4)
>> #define PARAM_FIR           (1 << 3)
>> #define PARAM_IIR           (1 << 2)
>> #define PARAM_HUFFOFFSET    (1 << 1)
>>     //@}
>>
>>     //@{
>>     /** Matrix data */
>>
>>     //! Number of matrices to be applied
>>     uint8_t     num_primitive_matrices[MAX_SUBSTREAMS];
>>
>>     //! Output channel of matrix
>>     uint8_t     matrix_ch[MAX_SUBSTREAMS][MAX_MATRICES];
>>
>>     //! Whether the LSBs of the matrix output are encoded in the bitstream
>>     uint8_t     lsb_bypass[MAX_SUBSTREAMS][MAX_MATRICES];
>>     //! Matrix coefficients, stored as 2.14 fixed point
>>     int32_t     matrix_coeff[MAX_SUBSTREAMS][MAX_MATRICES][MAX_CHANNELS+2];
>>     //! Left shift to apply to noise values in 0x31eb substreams
>>     uint8_t     matrix_noise_shift[MAX_SUBSTREAMS][MAX_MATRICES];
>>     //@}
>>
>>     //! Left shift to apply to huffman-decoded residuals
>>     uint8_t     quant_step_size[MAX_SUBSTREAMS][MAX_CHANNELS];
>>
>>     //! Number of PCM samples in current audio block
>>     uint16_t    blocksize[MAX_SUBSTREAMS];
>>     //! Number of PCM samples decoded so far in this frame
>>     uint16_t    blockpos[MAX_SUBSTREAMS];
>>
>>     //! Left shift to apply to decoded PCM values to get final 24-bit output
>>     int8_t      output_shift[MAX_SUBSTREAMS][MAX_CHANNELS];
>>
>>     //@{
>>     /** Filter data. */
>> #define FIR 0
>> #define IIR 1
>>     //! Number of taps in filter
>>     uint8_t     filter_order[MAX_CHANNELS][2];
>>     //! Right shift to apply to output of filter
>>     uint8_t     filter_coeff_q[MAX_CHANNELS][2];
>>
>>     uint8_t     filter_index[MAX_CHANNELS][2];
>>
>>     int32_t     filter_coeff[MAX_CHANNELS][2][MAX_FILTER_ORDER];
>>     int32_t     filter_state[MAX_CHANNELS][2][MAX_FILTER_ORDER];
>>     //@}
>>
>>     //@{
>>     /** Sample data coding infomation */
>>     //! Offset to apply to residual values
>>     int16_t     huff_offset[MAX_CHANNELS];
>>     //! Sign/rounding corrected version of huff_offset
>>     int32_t     sign_huff_offset[MAX_CHANNELS];
>>     //! Which VLC codebook to use to read residuals
>>     uint8_t     codebook[MAX_CHANNELS];
>>     //! Size of residual suffix not encoded using VLC
>>     uint8_t     huff_lsbs[MAX_CHANNELS];
>>     //@}
>>
>>     //! Running XOR of all output samples
>>     int32_t     lossless_check_data[MAX_SUBSTREAMS];
>>
>>     int8_t      noise_buffer[MAX_BLOCKSIZE_POW2];
>>     int8_t      bypassed_lsbs[MAX_BLOCKSIZE][MAX_CHANNELS];
>>     int32_t     sample_buffer[MAX_BLOCKSIZE][MAX_CHANNELS+2];
>> } MLPDecodeContext;
> 
> There are so many blah[substr] all over the place, maybe putting these
> in a SubStream struct would simplify the code?

What kind of simplifications did you have in mind? The result of a 
half-job looked uglier to me. Attached substr_struct.diff patch is what 
I started doing.

> [...]
>> /** Initialize the decoder. */
>>
>> static int mlp_decode_init(AVCodecContext *avctx)
> 
> you dont really need a mlp_ before static functions in mlp*.c

I don't think there's any harm in keeping it. Lots of other files do the 
same.

> and the doxy comment is close to useless

Removed.

> [...]
>>     m->min_channel       [substr] = get_bits(gbp, 4);
>>     m->max_channel       [substr] = get_bits(gbp, 4);
>>     m->max_matrix_channel[substr] = get_bits(gbp, 4);
>>
>>     if (m->min_channel[substr] > m->max_channel[substr]) {
>>         av_log(m->avctx, AV_LOG_ERROR,
>>                "Substream min channel cannot be greater than max channel.\n");
>>         m->min_channel[substr] = m->max_channel[substr]
>>             = m->max_matrix_channel[substr] = 0;
>>         return -1;
>>     }
> 
> i wonder if it would be cleaner to read into local vars and assign to the
> structs after validation but then maybe not i dunno

I removed the setting to zero since it's no longer needed and it doesn't 
look unclean now IMO.

> [...]
>> static int read_filter_params(MLPDecodeContext *m, GetBitContext *gbp,
>>                               unsigned int channel, unsigned int filter)
>> {
>>     const char fchar = filter ? 'I' : 'F';
>>     int i, order;
>>
>>     // filter is 0 for FIR, 1 for IIR
>>     assert(filter < 2);
>>
>>     order = get_bits(gbp, 4);
>>     if (order > MAX_FILTER_ORDER) {
>>         av_log(m->avctx, AV_LOG_ERROR,
>>                "%cIR filter order %d is greater than maximum %d\n",
>>                fchar, order, MAX_FILTER_ORDER);
>>         return -1;
>>     }
>>     m->filter_order[channel][filter] = order;
>>
>>     if (order > 0) {
>>         int coeff_bits, coeff_shift;
>>
>>         m->filter_coeff_q[channel][filter] = get_bits(gbp, 4);
>>
> 
>>         coeff_bits = get_bits(gbp, 5);
>>         coeff_shift = get_bits(gbp, 3);
> 
> vertical align

I knew I had missed one...

> [...]
>>                 m->matrix_ch[substr][mat] = get_bits(gbp, 4);
>>                 frac_bits = get_bits(gbp, 4);
>>                 m->lsb_bypass[substr][mat] = get_bits1(gbp);
>>
>>                 if (m->matrix_ch[substr][mat] > m->max_channel[substr]) {
>>                     av_log(m->avctx, AV_LOG_ERROR,
>>                            "Invalid channel %d specified as output from matrix\n",
>>                            m->matrix_ch[substr][mat]);
>>                     m->matrix_ch[substr][mat] = 0;
>>                     return -1;
>>                 }
> 
> is the = 0 still needed? MAX_CHANNELS is 16 and sample_buffer is large enough
> apparently for that.

It's not needed anymore. That and another one removed.

> [...]
>>     if (m->param_presence_flags[substr] & PARAM_OUTSHIFT)
>>         if (get_bits1(gbp)) {
>>             for (ch = 0; ch <= m->max_matrix_channel[substr]; ch++) {
>>                 m->output_shift[substr][ch] = get_bits(gbp, 4);
>>                 dprintf(m->avctx, "output shift[%d] = %d\n",
>>                         ch, m->output_shift[substr][ch]);
>>                 /* TODO: validate */
>>             }
>>         }
>>
>>     if (m->param_presence_flags[substr] & PARAM_QUANTSTEP)
>>         if (get_bits1(gbp))
>>             for (ch = 0; ch <= m->max_channel[substr]; ch++) {
>>                 m->quant_step_size[substr][ch] = get_bits(gbp, 4);
>>                 /* TODO: validate */
>>
>>                 calculate_sign_huff(m, substr, ch);
>>             }
> 
> inconsistant {}

Made consistent.

> [...]
>> /** 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, unsigned int substr,
>>                          unsigned int channel, int32_t residual)
>> {
>>     unsigned int i, j, index;
>>     int64_t accum = 0;
>>     int32_t result;
>>
>>     /* TODO: Move this code to DSPContext? */
>>
>> #define INDEX(channel, order, pos) \
>>     ((m->filter_index[channel][order] + (pos)) & (MAX_FILTER_ORDER - 1))
>>
>>     for (j = 0; j < 2; j++)
>>         for (i = 0; i < m->filter_order[channel][j]; i++)
>>             accum += (int64_t)m->filter_state[channel][j][INDEX(channel,j,i)] *
>>                      m->filter_coeff[channel][j][i];
>>
>>     accum = accum >> m->filter_coeff_q[channel][FIR];
>>     result = (accum + residual)
>>                 & ~((1 << m->quant_step_size[substr][channel]) - 1);
>>
>>     index = INDEX(channel, FIR, -1);
>>     m->filter_state[channel][FIR][index] = result;
>>     m->filter_index[channel][FIR] = index;
>>
>>     index = INDEX(channel, IIR, -1);
>>     m->filter_state[channel][IIR][index] = result - accum;
>>     m->filter_index[channel][IIR] = index;
>>
>> #undef INDEX
>>
>>     return result;
>> }
> 
> Why the complicated indexing?
> how big would filter_state be if it where a normal flat array? 160 entries?

It would need
sizeof(int32_t) * MAX_BLOCKSIZE * 2 * MAX_CHANNELS = 20480 = 20k
bytes more in the context, and attached filter_index_flat_array.diff 
patch. I assume that's not ok =)

>> /** Read a block of PCM residual (or actual if no filtering active) data.
>>  */
>>
>> static int read_block_data(MLPDecodeContext *m, GetBitContext *gbp,
>>                            unsigned int substr)
>> {
>>     unsigned int i, mat, ch, expected_stream_pos = 0;
>>
> 
>>     if (m->data_check_present[substr])
>>         expected_stream_pos = get_bits_count(gbp) + get_bits(gbp, 16);
>>         /* UNTESTED - find an example stream */
> 
> tell the user to upload one if above is true

Done.

>>     if (m->blockpos[substr] + m->blocksize[substr] > m->access_unit_size) {
>>         av_log(m->avctx, AV_LOG_ERROR, "Too many audio samples in frame\n");
>>         return -1;
>>     }
>>
>>     memset(&m->bypassed_lsbs[m->blockpos[substr]][0], 0,
>>            m->blocksize[substr] * MAX_CHANNELS);
> 
> instead of MAX_CHANNELS a sizeof would be nicer

Changed.

>>     for (i = 0; i < m->blocksize[substr]; i++) {
>>         for (mat = 0; mat < m->num_primitive_matrices[substr]; mat++)
>>             if (m->lsb_bypass[substr][mat])
>>                 m->bypassed_lsbs[i + m->blockpos[substr]][mat] = get_bits1(gbp);
>>
>>         for (ch = m->min_channel[substr]; ch <= m->max_channel[substr]; ch++) {
>>             int32_t sample;
>>
> 
>>             if (read_huff(m, gbp, substr, ch, &sample) < 0)
>>                 return -1;
> 
> please use the return value instead of passing pointers to it

sample may be positive or negative, so a simple < check won't help. But 
sample must not be bigger than 24 bits.

Is attached read_huff_return_bad_code.diff patch ok?

> [...]
>>         for (i = 0; i < m->blockpos[substr]; i++) {
>>             int64_t accum = 0;
>>             for (src_ch = 0; src_ch <= maxchan; src_ch++) {
>>                 accum += (int64_t)m->sample_buffer[i][src_ch]
>>                                   * m->matrix_coeff[substr][mat][src_ch];
>>             }
>>             if (m->matrix_noise_shift[substr][mat]) {
>>                 uint32_t index = m->num_primitive_matrices[substr] - mat;
>>                 index = (i * (index * 2 + 1) + index) & (m->access_unit_size_pow2 - 1);
>>                 accum += m->noise_buffer[index] << (m->matrix_noise_shift[substr][mat] + 7);
>>             }
>>             m->sample_buffer[i][dest_ch] = ((accum >> 14) & ~((1 << m->quant_step_size[substr][dest_ch]) - 1))
>>                                              + m->bypassed_lsbs[i][mat];
>>         }
> 
> following may be faster ...
> 
> index = m->num_primitive_matrices[substr] - mat;
> index2= 2*index+1;
> for (i = 0; i < m->blockpos[substr]; i++) {
> ...
>     if (m->matrix_noise_shift[substr][mat]) {
>         index &= m->access_unit_size_pow2 - 1;
>         accum += m->noise_buffer[index] << (m->matrix_noise_shift[substr][mat] + 7);
>         index += index2;
>     }

I don't have any samples that touch this code. I'm still waiting for Ian 
to send me more samples (and I'm searching for some more also).

Do you prefer if I go ahead and change it anyways?

> [...]
>> static int read_access_unit(AVCodecContext *avctx, void* data, int *data_size,
>>                             const uint8_t *buf, int buf_size)
>> {
>>     MLPDecodeContext *m = avctx->priv_data;
>>     GetBitContext gb;
>>     unsigned int length, substr;
>>     unsigned int substream_start;
>>     unsigned int header_size = 0;
>>     uint8_t substream_parity_present[MAX_SUBSTREAMS];
>>     uint16_t substream_data_len[MAX_SUBSTREAMS];
>>     uint8_t parity_bits = 0;
>>
>>     if (buf_size < 4)
>>         return 0;
>>
>>     length = (AV_RB16(buf) & 0xfff) * 2;
>>
>>     if (length > buf_size)
>>         return -1;
>>
>>     PARITY_2BYTES;
>>     PARITY_2BYTES;
>>
>>     init_get_bits(&gb, buf, (length - 4) * 8);
>>
>>     if (show_bits_long(&gb, 31) == (0xf8726fba >> 1)) {
>>         dprintf(m->avctx, "Found major sync\n");
>>         if (read_major_sync(m, &gb) < 0)
>>             goto error;
>>         header_size += 28;
>>         buf += 28;
>>     }
>>
>>     if (!m->params_valid) {
>>         av_log(m->avctx, AV_LOG_WARNING,
>>                "Stream parameters not seen; skipping frame\n");
>>         *data_size = 0;
>>         return length;
>>     }
>>
>>     substream_start = 0;
>>
>>     for (substr = 0; substr < m->num_substreams; substr++) {
>>         int extraword_present, checkdata_present, end;
>>
>>         extraword_present = get_bits1(&gb);
>>         skip_bits1(&gb);
>>         checkdata_present = get_bits1(&gb);
>>         skip_bits1(&gb);
>>
>>         end = get_bits(&gb, 12) * 2;
>>
>>         PARITY_2BYTES;
> 
> isnt that doing the same as calculate_parity() ? if so why is it using these
> MACROS spread over the whole code ...

Cleaned-up a bit using calculate_parity().

>>         if (extraword_present) {
>>             skip_bits(&gb, 16);
>>             PARITY_2BYTES;
>>         }
>>
>>         if (end + header_size > length) {
>>             av_log(m->avctx, AV_LOG_ERROR,
>>                    "Substream %d data indicated length goes off end of packet.\n",
>>                    substr);
>>             end = length - header_size;
>>         }
>>
>>         if (end < substream_start) {
>>             av_log(avctx, AV_LOG_ERROR,
>>                    "Substream %d data indicated end offset "
>>                    "is before calculated start offset.\n",
>>                    substr);
>>             goto error;
>>         }
>>
>>         if (substr > m->max_decoded_substream)
>>             continue;
>>
>>         substream_parity_present[substr] = checkdata_present;
>>         substream_data_len[substr] = end - substream_start;
>>         substream_start = end;
>>     }
>>
>>     if ((((parity_bits >> 4) ^ parity_bits) & 0xF) != 0xF) {
>>         av_log(avctx, AV_LOG_ERROR, "Parity check failed.\n");
>>         goto error;
>>     }
>>
>>     for (substr = 0; substr <= m->max_decoded_substream; substr++) {
>>         init_get_bits(&gb, buf, substream_data_len[substr] * 8);
>>
>>         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, buf, substr) < 0)
>>                         goto error;
>>                     m->restart_seen[substr] = 1;
>>                 }
>>
>>                 if (!m->restart_seen[substr]) {
>>                     av_log(m->avctx, AV_LOG_ERROR,
>>                            "No restart header present in substream %d.\n",
>>                            substr);
>>                     goto error;
>>                 }
>>
>>                 if (read_decoding_params(m, &gb, substr) < 0)
>>                     goto error;
> 
> Does an error in one substream affect the others? if not these hard errors
> is not ideal.

Now it tries the next substream if one fails.

Ramiro Polla
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mlpdec.c
Type: text/x-csrc
Size: 39622 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080701/673fb98e/attachment.c>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: filter_index_flat_array.diff
Type: text/x-diff
Size: 3302 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080701/673fb98e/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: read_huff_return_bad_code.diff
Type: text/x-diff
Size: 1715 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080701/673fb98e/attachment-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: substr_struct.diff
Type: text/x-diff
Size: 22076 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080701/673fb98e/attachment-0002.diff>



More information about the ffmpeg-devel mailing list