[FFmpeg-devel] [PATCH] ALS decoder

Måns Rullgård mans
Sat Aug 22 11:30:11 CEST 2009


Thilo Borgmann <thilo.borgmann at googlemail.com> writes:

>>> +    // read the fixed items
>>> +    sconf->als_id               = get_bits_long(&gb, 32);
>>> +    sconf->samp_freq            = m4ac.sample_rate;
>>> +    skip_bits_long(&gb, 32);
>> 
>> Maybe comment what is being skipped here and elsewhere.
> Ok, I thought it is obvious. Done.

If you're reading the spec with the other eye, yes.

>>> +    sconf->crc_enabled          = get_bits1(&gb);
>>> +    sconf->rlslms               = get_bits1(&gb);
>>> +    skip_bits(&gb, 5);                                      // skip 5 reserved bits
>> 
>> Please lose some of that whitespace.  Compliments on the alignment
>> though.  Diego will love you.
> I did this above, but here, not even the 80-characters reason exist.
> Why give up readability?

I was referring to the spaces before the comment on the last line.
The alignment of the assignments is very nice.

>>> +/** Reads and decodes a Rice codeword.
>>> + */
>>> +static int64_t decode_rice(GetBitContext *gb, unsigned int k)
>>> +{
>>> +    int64_t value = 0;
>>> +    int64_t q = 0;
>>> +    int     max = gb->size_in_bits - get_bits_count(gb) - k;
>>> +
>>> +    if (!k) {
>>> +        q = get_unary(gb, 0, max);
>>> +        return (q & 1) ? -((q + 1) >> 1) : ((q + 1) >> 1);
>>> +    } else if (k == 1) {
>>> +        q = get_unary(gb, 0, max);
>>> +        return get_bits1(gb) ? q : -(q + 1);
>>> +    } else {
>>> +        unsigned int r, sub_sign;
>>> +
>>> +        q         = get_unary(gb, 0, max);
>>> +        sub_sign  = get_bits1(gb);
>>> +        r         = get_bits_long(gb, k - 1);
>>> +
>>> +        value = (q << (k - 1)) + r;
>>> +
>>> +        return sub_sign ? value : -(value + 1);
>>> +    }
>>> +}
>> 
>> This function looks like it was designed specifically to make gcc fail:
>> 
>> 1.  64-bit variables used unnecessarily.
>> 2.  GCC hates complex conditionals.
>> 3.  Compilers in general are bad at bit-hacks.
>> 
>> It seems to me that int is enough for 'q' in the first two cases.
>> get_unary() returns int, so we start with at most that many bits.  To
>> avoid overflow in q+1, do this instead:
>> 
>>     int r = q & 1;
>>     return r ? -((q >> 1) + r) : ((q >> 1) + r);
>> 
>> Furthermore, gcc does stupid things with that code even with plain int
>> variables.  Rewriting in a few more steps helps massively:
>> 
>>     int r = q & 1;
>>     q = (q >> 1) + r;
>>     return r ? -q : q;
>> 
>> This reduces the number of instructions on ARM from 6 to 4 (from 4 to
>> 3 with armcc).  On MIPS it goes from 9 with a branch to 5 branch-free.
>> 
>> Finally, -(v + 1) is equivalent to ~v on two's complement machines,
>> which we have previously agreed to assume we have.  Hence we should
>> write the return values in the second and third cases like this:
>> 
>>     return sign ? value : ~value;
>> 
>> This prevents 32-bit overflow in the k==1 case and gives better code
>> with several compilers in the final case, where we must resort to
>> 64-bit maths.
> Tricky. Works like a charm. Done.

Did it get any faster?

>>> +            // reconstruct difference signal for prediction (joint-stereo)
>>> +            if (*js_blocks && raw_other) {
>>> +                int i;
>>> +                if (raw_other > raw_samples) {          // D = R - L
>>> +                    for (i = -1; i >= -sconf->max_order; i--)
>>> +                        raw_samples[i] = raw_other[i] - raw_samples[i];
>>> +                } else {                                // D = R - L
>> 
>> Are those two comments meant to be the same?
> Yes but semantically different. This is meant to be that way.
> It shows, which channel is represented by which variable.

I guess that subtlety was lost on me, although I did understand the
code...

>>> +    // allocate previous raw sample buffer
>>> +    if (!(ctx->prev_raw_samples = av_malloc(sizeof(int64_t) * sconf->max_order))) {
>>> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
>>> +        decode_end(avctx);
>>> +        return AVERROR(ENOMEM);
>>> +    }
>>> +
>>> +    // allocate raw and carried sample buffer
>>> +    if (!(ctx->raw_buffer = av_mallocz(sizeof(int64_t) *
>>> +                                       avctx->channels * channel_size))) {
>>> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
>>> +        decode_end(avctx);
>>> +        return AVERROR(ENOMEM);
>>> +    }
>>> +
>>> +    // allocate raw sample array buffer
>>> +    if (!(ctx->raw_samples = av_malloc(sizeof(int64_t*) * avctx->channels))) {
>>> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer array failed.\n");
>>> +        decode_end(avctx);
>>> +        return AVERROR(ENOMEM);
>>> +    }
>> 
>> This looks very repetitive...
> Thus.... it is good/bad/crazy?
> The log message depends on the buffer allocated, the size of the buffer
> is never the same...

The messages look the same to me...

>>> +    // allocate raw and carried samples buffers
>>> +    ctx->raw_samples[0] = ctx->raw_buffer + sconf->max_order;
>>> +    for (c = 1; c < avctx->channels; c++) {
>>> ...
>>> +
>>> Index: libavcodec/als_data.h
>>> ===================================================================
>>> --- libavcodec/als_data.h	(revision 0)
>>> +++ libavcodec/als_data.h	(revision 0)
>>> @@ -0,0 +1,147 @@
>>> +/*
>>> + * ALS header file for common data
>>> + * Copyright (c) 2009 Thilo Borgmann <thilo.borgmann _at_ googlemail.com>
>>> ...
>>> +#ifndef AVCODEC_ALS_DATA_H
>>> +#define AVCODEC_ALS_DATA_H
>>> +
>>> +
>>> +#include <stdint.h>
>>> +
>>> +/** Rice parameters and corresponding index offsets for decoding the
>>> + *  indices of scaled PARCOR values. The table choosen is set globally
>>> + *  by the encoder and stored in ALSSpecificConfig.
>>> + */
>>> +int8_t parcor_rice_table[3][20][2] = {
>>> +                        {
>>> +                        {-52, 4},
>> 
>> WTF happened to the indentation here?

I usually do something like this:

int8_t parcor_rice_table[3][20][2] = {
    { { -52, 4 }, { xx, yy }, { zz, ww }, { ...    }
      /* ... */
      { ...    }, { ...    }, { ...    }, { ...    } },
};

>>> +/** Scaled PARCOR values used for the first two PARCOR coefficients.
>>> + *  To be indexed by the Rice coded indices.
>>> + *  Generated by: parcor_scaled_values[i] = 32 + ((i * (i+1)) << 7) - (1 << 20)
>>> + */
>>> +int32_t parcor_scaled_values[] = {-1048544, -1048288, -1047776, -1047008,
>>> +                                  -1045984, -1044704, -1043168, -1041376,
>> 
>> And here.
> Would you like block- or function-like indents?
> No kidding - I ask that before it goes back and forth and I don't care
> which one...
> (Or are there table-like indention rules I've missed?)

Here I'd do like this:

int32_t parcor_scaled_values[] =
    -1048544, -1048288, -1047776, -1047008,
};

-- 
M?ns Rullg?rd
mans at mansr.com



More information about the ffmpeg-devel mailing list