[FFmpeg-devel] [PATCH] MLP/TrueHD Decoder - 2nd try
Ramiro Polla
ramiro
Fri Jul 4 02:15:20 CEST 2008
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:
[...]
>>> [...]
>>>> /** 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?
>
> "Maximum sample frequency seen in files"
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?
>
> yes
Ok.
[...]
>>> [...]
>>>> /** 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.
>
> yes, these would then be 1 vs 0 checks of the 1 bit read after the sync,
> which IMHO is cleaner.
Done.
>>> [...]
>>>> 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?
>
> hmm, forget my suggestion it was silly ...
>
>
> [...]
>> static int mlp_decode_init(AVCodecContext *avctx)
>
> av_cold
Done.
> [...]
>> 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?
> [...]
>> /** Generate two channels of noise, used in the matrix when
>> * restart_sync_word == 0x31ea. */
>>
>> static void generate_noise_1(MLPDecodeContext *m, unsigned int substr)
>> {
>> SubStream *s = &m->substream[substr];
>> unsigned int i;
>> uint32_t seed = s->noisegen_seed;
>> unsigned int maxchan = s->max_matrix_channel;
>>
>> for (i = 0; i < s->blockpos; i++) {
>> uint16_t seed_shr7 = seed >> 7;
>> m->sample_buffer[i][maxchan+1] = ((int8_t)(seed >> 15)) << s->noise_shift;
>> m->sample_buffer[i][maxchan+2] = ((int8_t) seed_shr7) << s->noise_shift;
>>
>> seed = (seed << 16) ^ seed_shr7 ^ (seed_shr7 << 5);
>> }
>>
>> s->noisegen_seed = seed;
>> }
>>
>> /** Generate a block of noise, used when restart_sync_word == 0x31eb. */
>>
>> static void generate_noise_2(MLPDecodeContext *m, unsigned int substr)
>> {
>
> the 2 functions should be named more correctly like
> generate_2_noise_channels()
> fill_noise_buffer()
Ok.
> [...]
>> /** 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. */
>
> i think "returns <0 on error" is better
Ok.
> [...]
>> rematrix_channels(m, substr - 1);
>>
>> if (output_data(m, substr - 1, data, data_size) < 0)
>> return -1;
>
> shouldnt these rather be max_decoded_substream? I mean substr depends on the
> loop above ending with substr-1 = max_decoded_substream
Changed.
Ramiro Polla
-------------- next part --------------
A non-text attachment was scrubbed...
Name: filter_state_buffer_noindex.diff
Type: text/x-diff
Size: 1532 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080704/9cacc56f/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: filter_state_buffer_offset.diff
Type: text/x-diff
Size: 1715 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080704/9cacc56f/attachment-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mlpdec.c
Type: text/x-csrc
Size: 39900 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080704/9cacc56f/attachment.c>
More information about the ffmpeg-devel
mailing list