[FFmpeg-devel] [PATCH] MLP/TrueHD Decoder - 2nd try
Ramiro Polla
ramiro
Thu Jul 3 02:47:11 CEST 2008
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:
> [...]
>>> [...]
>>>>> [...]
>>>>>> /** 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 =)
>>> yes, but do we really need that amount?
>>> if the huff decode and filter loops are split (i assume thats possible)
>>> then
>>> we can do a
>>> for(channels)
>>> for(sample in block)
>>> which would only need sizeof(int32_t) * 2 * MAX_BLOCKSIZE
>>> and a memcpy() or 2 for a reading writing the state
>> Done this way and got some speedup. But for now I leave filter_state_buffer
>> in the context. Should I move it to the stack as in attached
>> filter_state_buffer_stack.diff patch?
>
> yes
Applied.
> [...]
>> /** Maximum number of substreams that can be decoded. This could also be set
>> * higher, but again I haven't seen any examples with more than two. */
>> #define MAX_SUBSTREAMS 2
>>
>
>> /** Maximum sample frequency supported. */
>> #define MAX_SAMPLERATE 192000
>
> slightly unclear, supported by our implementation or MLP?
According to the coded values, MLP should support up to 48000 << 7. But
then again, I didn't do the RE, and all the samples I've seen don't go
higher than 192000.
Should there be a comment stating this code does not follow specs and is
based on RE and so some values may be off?
>> /** The maximum number of audio samples within one access unit. */
>> #define MAX_BLOCKSIZE (40 * (MAX_SAMPLERATE / 48000))
>> /** The next power of two greater than MAX_BLOCKSIZE. */
>> #define MAX_BLOCKSIZE_POW2 (64 * (MAX_SAMPLERATE / 48000))
>>
>> /** Number of allowed filters. */
>> #define NUM_FILTERS 2
>>
>> /** The maximum number of taps in either the IIR or FIR filter.
>> * I believe MLP actually specifies the maximum order for IIR filters is four,
>> * and that the sum of the orders of both filters must be <= 8. */
>> #define MAX_FILTER_ORDER 8
>>
>> /** Number of bits used for VLC lookup - longest huffman code is 9. */
>> #define VLC_BITS 9
>>
>>
>> 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.";
>>
>> typedef struct SubStream {
>
>> //! For each substream, whether a restart header has been read
>> uint8_t restart_seen;
>
> The comment is not very informative beyond what is obvious from the variable
> name
Changed.
>> //@{
>> /** Restart header data */
>> //! The sync word used at the start of the last restart header
>> uint16_t restart_sync_word;
>>
>> //! The index of the first channel coded in this substream
>> uint8_t min_channel;
>> //! The index of the last channel coded in this substream
>> uint8_t max_channel;
>> //! The number of channels input into the rematrix stage
>> uint8_t max_matrix_channel;
>>
>> //! The left shift applied to random noise in 0x31ea substreams
>> uint8_t noise_shift;
>> //! The current seed value for the pseudorandom noise generator(s)
>> uint32_t noisegen_seed;
>>
>
>> //! Does this substream contain extra info to check the size of VLC blocks?
>> uint8_t data_check_present;
>
> Describing variables with questions sounds a little odd.
Changed.
>> //! Bitmask of which parameter sets are conveyed in a decoding parameter block
>> uint8_t param_presence_flags;
>> #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;
>>
>
>> //! Output channel of matrix
>> uint8_t matrix_ch[MAX_MATRICES];
>
> matrix_out_ch
Changed.
> [...]
>> //! Do we have valid stream data read from a major sync block?
>> uint8_t params_valid;
>
> again description by question
Changed.
> [...]
>> /** Initialize static data, constant between all invocations of the codec. */
>>
>> static void init_static()
>
> av_cold
Changed. And should all init and close functions be av_cold as well? I
think this has not been enforced on some new code.
>> {
>> if (!huff_vlc[0].bits) {
>
> checking the last set element avoids race conditions (its not a real issues
> as avcodec_open() isnt thread safe)
So, can I leave it as is? And I remember you telling Robert that those
checks could be suppressed for the VLC tables in his AAC code. Does this
also apply here?
> [...]
>> /** Read a restart header from a block in a substream. This contains parameters
>> * required to decode the audio that do not change very often. Generally
>> * (always) present only in blocks following a major sync.
>> */
>>
>> static int read_restart_header(MLPDecodeContext *m, GetBitContext *gbp,
>> const uint8_t *buf, unsigned int substr)
>> {
>> SubStream *s = &m->substream[substr];
>> unsigned int ch;
>> int sync_word, tmp;
>> uint8_t checksum;
>> uint8_t lossless_check;
>> int start_count = get_bits_count(gbp);
>>
>
>> sync_word = get_bits(gbp, 14);
>>
>> if ((sync_word & 0x3ffe) != 0x31ea) {
>
> sync_word = get_bits(gbp, 13);
> if(sync_word != ...
sync_word is later used in the code to check if it's either 0x31ea or
0x31eb.
> [...]
>> for (ch = s->min_channel; ch <= s->max_channel; ch++) {
>> m->filter_order [ch][FIR] = 0;
>> m->filter_order [ch][IIR] = 0;
>> m->filter_coeff_q[ch][FIR] = 0;
>> m->filter_coeff_q[ch][IIR] = 0;
>>
>> memset(m->filter_coeff[ch], 0, sizeof(m->filter_coeff[ch]));
>> memset(m->filter_state[ch], 0, sizeof(m->filter_state[ch]));
>
> If order (= number of taps) is 0 then the coeffs should not matter
And if it's not zero they're read from the bitstream, so these memsets
are pointless. Removed.
> [...]
>> 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);
>> if (coeff_bits < 1 || coeff_bits > 16) {
>> av_log(m->avctx, AV_LOG_ERROR,
>> "%cIR filter coeff_bits must be between 1 and 16\n",
>> fchar);
>> return -1;
>> }
>> if (coeff_bits + coeff_shift > 16) {
>> av_log(m->avctx, AV_LOG_ERROR,
>> "Sum of coeff_bits and coeff_shift for %cIR filter must be 16 or less\n",
>> fchar);
>> return -1;
>> }
>>
>> for (i = 0; i < order; i++)
>> m->filter_coeff[channel][filter][i] =
>> get_sbits(gbp, coeff_bits) << coeff_shift;
>
> Cant coeff_shift just be subtracted from filter_coeff_q ?
filter_coeff_q is the same for both filters. coeff_shift can differ.
Besides wouldn't this just change a shift for a minus while reading the
data and leave all the rest of the muls and shifts the same?
> [...]
>
>> s->matrix_ch[mat] = get_bits(gbp, 4);
>> frac_bits = get_bits(gbp, 4);
>> s->lsb_bypass[mat] = get_bits1(gbp);
>
> vertical align
Aligned.
> [...]
>> /** 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)
>> {
>> SubStream *s = &m->substream[substr];
>> unsigned int i, ch, expected_stream_pos = 0;
>>
>> if (s->data_check_present) {
>
>> expected_stream_pos = get_bits_count(gbp) + get_bits(gbp, 16);
>
> is there something that prevents gcc from doing get_bits() first?
IIRC it's unspecified in C if the same data is used on two functions on
the same line. Can anyone from the Ministry of C Compliance shed some light?
Changed anyways.
Ramiro Polla
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mlpdec.c
Type: text/x-csrc
Size: 39903 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080703/a5644cad/attachment.c>
More information about the ffmpeg-devel
mailing list