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

[Ramiro Polla]

Michael Niedermayer wrote:
> On Fri, Jul 04, 2008 at 01:15:20AM +0100, Ramiro Polla wrote:
>> Michael Niedermayer wrote:
>>> On Thu, Jul 03, 2008 at 01:47:11AM +0100, Ramiro Polla wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Tue, Jul 01, 2008 at 06:24:55PM +0100, Ramiro Polla wrote:
>>>>>> Michael Niedermayer wrote:
>>>>>>> On Tue, Jul 01, 2008 at 01:55:39AM +0100, Ramiro Polla wrote:
[...]
>>> [...]
>>>>     for (i = 0; i < s->blocksize; i++) {
>>>>         int32_t residual = m->sample_buffer[i + s->blockpos][channel];
>>>>         unsigned int order;
>>>>         int64_t accum = 0;
>>>>         int32_t result;
>>>>
>>>>         /* TODO: Move this code to DSPContext? */
>>>>
>>>>         for (j = 0; j < NUM_FILTERS; j++)
>>>>             for (order = 0; order < m->filter_order[channel][j]; order++)
>>>>                 accum += (int64_t)filter_state_buffer[j][index + order] *
>>>>                         m->filter_coeff[channel][j][order];
>>>>
>>>>         accum  = accum >> filter_coeff_q;
>>>>         result = (accum + residual) & mask;
>>>>
>>>>         --index;
>>>>
>>>>         filter_state_buffer[FIR][index] = result;
>>>>         filter_state_buffer[IIR][index] = result - accum;
>>>>
>>>>         m->sample_buffer[i + s->blockpos][channel] = result;
>>>>     }
>>> is there any reason for -- instead of using i ?
>> Hmmm... What do you think is best:
>> filter_state_buffer_noindex.diff
>> filter_state_buffer_offset.diff
>> or some other way to not have to count twice?
> 
> i thought of fliping the contents begin to end. But maybe its not ideal

I tried a couple of other ways with reading the values in reverse order 
from the bitstream and always increment the buffer_state index, but they 
didn't look as good.

> [...]
>>     //! Right shift to apply to output of filter.
>>     uint8_t     filter_coeff_q[MAX_CHANNELS][NUM_FILTERS];
> 
> The variable name is not very good

Changed.

> [...]
>> /** Initialize static data, constant between all invocations of the codec. */
>>
>> static av_cold void init_static()
>> {
> 
>>     if (!huff_vlc[0].bits) {
> 
> didint you want to remove that ?

Yes. Now I added a check for crc_init only, just like is done in 
mlp_parser.c, so I assumed it was ok.

> [...]
>> static inline void calculate_sign_huff(MLPDecodeContext *m, unsigned int substr,
>>                                        unsigned int ch)
>> {
>>     SubStream *s = &m->substream[substr];
>>     int lsb_bits = m->huff_lsbs[ch] - s->quant_step_size[ch];
>>     int sign_shift = lsb_bits + (m->codebook[ch] ? 2 - m->codebook[ch] : -1);
>>
>>     m->sign_huff_offset[ch] = m->huff_offset[ch];
>>
>>     if (m->codebook[ch] > 0)
>>         m->sign_huff_offset[ch] -= 7 << lsb_bits;
>>
>>     if (sign_shift >= 0)
>>         m->sign_huff_offset[ch] -= 1 << sign_shift;
>> }
> 
> it would be clearer if this didnt set m->sign_huff_offset[ch] but return an
> int instead.

Done.

> [...]
>> /** Read parameters for one of the prediction filters. */
>>
>> 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) {
> 
> is a filter order == 0 even valid ?

As Ian already answered: yes. It happens when data is too random and 
writing the residuals to the filters would be as big as writing the full 
data. The data doesn't even have to be random. The 440hz.mlp sample 
(which is just a pure 440 hz sine wave) uses this from time to time. I'm 
really impressed that the encoder can't code a pure sine wave 
efficiently. Shouldn't it be fairly predictable?

Can you just confirm again if these changes were ok and the decoder is 
good to apply?

Ramiro Polla
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mlpdec.c
Type: text/x-csrc
Size: 39965 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080704/e4729c31/attachment.c>