[FFmpeg-devel] [PATCH] ALS decoder

[Thilo Borgmann]

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



>> +        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.
I like the ?: because it does not add an unnecessary assignment in the
sconf->floating case.



>> +        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.
I like it better as is for the same reason as above.


> 
>> +                // 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.
I don't care at all... "usualized".


> 
>> +                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?
It is left from coding close to the specs... start = 0 would be
avoidable by using if's which is worse, I think.
The sub_blocks == 0 case does not care about start so that is alright.

The other hints make this whole block a lot better!


>> +        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...
I like it but there is the same additional assignment thing here. Adapted.



>> +        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
Looks like but does something else actually (note "c++;" in one case).




>> +    // 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) {
>     ...
> }
Ok.


Thanks!

-Thilo