[FFmpeg-devel] [PATCH] ALS decoder

[Reimar Döffinger]

On Sat, Aug 29, 2009 at 01:06:19AM +0200, Thilo Borgmann wrote:
> +    if (n < 31 && ((bs_info >> (30 - n)) & 1)) {

Not sure if its clearer (probably depends on the spec) but that's the
same as
(bs_info << n) & 0x40000000

> +    if (k > 1)
> +        q = (q << (k - 1)) + get_bits_long(gb, k - 1);
> +    else if (!k)
> +        q = (q >> 1);

q >>= 1;
Even for the first one I'd consider
q <<= k - 1;
q += get_bits_long(gb, k - 1);

> +    for (i = 0; i < (k + 1) >> 1; i++) {
> +        int64_t tmp1   = cof[    i    ] + ((MUL64(par[k], cof[k - i - 1]) + (1 << 19)) >> 20);
> +        cof[k - i - 1] = cof[k - i - 1] + ((MUL64(par[k], cof[    i    ]) + (1 << 19)) >> 20);
> +        cof[    i    ] = tmp1;
> +    }

Maybe worth writing as
for (i = 0, r = k - 1; i < (k + 1) >> 1; i++, r--) {
}
(r == k - i - 1)
And I suspect the condition is equivalent to i <= r

> +    if (ctx->cur_frame_length == ctx->last_frame_length) {
> +        unsigned int remaining = ctx->cur_frame_length;
> +
> +        for (b = 0; b < ctx->num_blocks; b++) {
> +            if (remaining < div_blocks[b]) {
> +                div_blocks[b] = remaining;
> +                ctx->num_blocks = b + 1;
> +                break;
> +            } else {
> +                remaining -= div_blocks[b];
> +            }

Since you have a break; there the else just seems to add useless
complexity...

> +        unsigned int const_val_bits;
> +
> +        if (sconf->floating)
> +            const_val_bits = 24;
> +        else
> +            const_val_bits = avctx->bits_per_raw_sample;

5 lines of code for something as simple seems a bit much,
I think either ?: or
unsigned int const_val_bits = avctx->bits_per_raw_sample;
if (sconf->floating)
    const_val_bits = 24;

Seems more appropriate.

> +        s[0] = get_bits(gb, (sconf->resolution > 1) ? 5 : 4);

The () are not really necessary. Not sure if you like it, but you could
also write it as
4 + (sconf->resolution > 1)

> +        if (sconf->adapt_order) {
> +            int opt_order_length = FFMAX(av_ceil_log2((block_length >> 3) - 1), 1);
> +            opt_order_length     = FFMIN(av_ceil_log2(sconf->max_order+1), opt_order_length);
> +            opt_order            = get_bits(gb, opt_order_length);
> +        } else {
> +            opt_order = sconf->max_order;
> +        }

Typical case where I find it preferable to just write
opt_order = sconf->max_order;
_before_ the if (i.e. that's some kind of "simple default case")
and get rid of the else.

> +                // read coefficient 0 to 19
> +                k_max = FFMIN(20, opt_order);
> +                // read coefficients 20 to 126
> +                k_max = FFMIN(127, opt_order);

Is it intentional you put the constants first? I don't really mind, it just
seems unusual.

> +                for (; k < k_max; k++) {
> +                    offset       = k & 1;
> +                    rice_param   = 2;
> +                    quant_index  = decode_rice(gb, rice_param) + offset;
> +                    quant_cof[k] = (quant_index << 14) + (1 << 13);
> +                }
> +
> +                // read coefficients 127 to opt_order
> +                for (; k < opt_order; k++) {
> +                    offset       = 0;
> +                    rice_param   = 1;
> +                    quant_index  = decode_rice(gb, rice_param) + offset;
> +                    quant_cof[k] = (quant_index << 14) + (1 << 13);
> +                }

What's that with putting constants that are used only once into
variables?
Are they left-over from a merge attempt?
While we are at it, what is the speed of
for (; k < opt_order; k++) {
  offset = k < k_max;
  quant_index  = decode_rice(gb, 1 + offset) + (k & offset);
  quant_cof[k] = (quant_index << 14) + (1 << 13);
}

(yes, offset is probably a bad name)

> +        for (sb = 0; sb < sub_blocks; sb++) {
> +            for (k = start; k < sb_length; k++)
> +                current_res[k] = decode_rice(gb, s[sb]);
> +            current_res += sb_length;
> +            start = 0;
> +        }

What's the point of setting start to 0 that often?
Also is it intentional that start does not get set for sub_blocks == 0?

> +        for (smp = 0; smp < block_length; smp++) {
> +            unsigned int max, dequant;
> +
> +            dequant = smp < progressive;
> +            max     = dequant ? smp : progressive;

Declaration and initialization can be merged.

> +        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
> +                for (i = -1; i >= -sconf->max_order; i--)
> +                    raw_samples[i] = raw_samples[i] - raw_other[i];
> +            }

(pseudo-code, will not compile):
a = raw_other; b = raw_samples;
if (a > b) FFSWAP(a, b);
for ...
   raw_samples[i] = b[i] - a[i];
Well, it might be slower...

> +        // reconstruct shifted signal
> +        if (*shift_lsbs)
> +            for (smp = -1; smp >= -sconf->max_order; smp--)
> +                raw_samples[smp] >>= *shift_lsbs;

For the *js_blocks && raw_other case maybe this is worth merging with
the other loop?
Well, maybe not, might be a bit messy.

> +    // read block type flag and read the accordingly

What?

> +    memmove((ctx->raw_samples[c]) - sconf->max_order,
> +            (ctx->raw_samples[c]) - sconf->max_order + sconf->frame_length,

What's up with the extra ()?

> +        memmove((ctx->raw_samples[c]) - sconf->max_order,
> +                (ctx->raw_samples[c]) - sconf->max_order + sconf->frame_length,

Duplicated? Maybe not worth avoiding though, but same comment as above

> +        ptr_div_blocks = &div_blocks[0];

Looks like the same as ptr_div_blocks = div_blocks;
to me.

> +    // allocate previous raw sample buffer
> +    if (!(ctx->prev_raw_samples = av_malloc(sizeof(*ctx->prev_raw_samples) * sconf->max_order)) ||
> +        !(ctx->raw_buffer       = av_mallocz(sizeof(*ctx->raw_buffer) * 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(*ctx->raw_samples) * avctx->channels))) {
> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer array failed.\n");
> +        decode_end(avctx);
> +        return AVERROR(ENOMEM);
> +    }

Is that different message worth the extra code?
And I can't help finding assignments inside the if one of the most ugly
things.
ctx->prev_raw_samples = av_malloc(sizeof(*ctx->prev_raw_samples) * sconf->max_order);
ctx->raw_buffer       = av_mallocz(sizeof(*ctx->raw_buffer) * avctx->channels * channel_size);
ctx->raw_samples      = av_malloc(sizeof(*ctx->raw_samples) * avctx->channels);
if (!ctx->prev_raw_samples || !ctx->raw_buffer || !ctx->raw_samples) {
    ...
}
Seems so much simpler to me.