[FFmpeg-devel] [PATCH] MLP/TrueHD Decoder - 2nd try
Michael Niedermayer
michaelni
Fri Jul 4 05:20:37 CEST 2008
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:
> [...]
>>>> [...]
>>>>> /** 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?
i thought of fliping the contents begin to end. But maybe its not ideal
[...]
> //! Right shift to apply to output of filter.
> uint8_t filter_coeff_q[MAX_CHANNELS][NUM_FILTERS];
The variable name is not very good
[...]
> /** 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 ?
[...]
> 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.
[...]
> /** 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 ?
[...]
And except things mentioned above iam fine with the patch.
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- 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/20080704/1bec9bdd/attachment.pgp>
More information about the ffmpeg-devel
mailing list