[FFmpeg-devel] [PATCH] ALS decoder

[Thilo Borgmann]

>>>> +    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.
Ah ok, Done.



>>>> +/** Reads and decodes a Rice codeword.
>>>> + */
>>>> +static int64_t decode_rice(GetBitContext *gb, unsigned int k)
>>>> +{
>>>> ...
>>>> +}
>>> This function looks like it was designed specifically to make gcc fail:
>>>...
>> Tricky. Works like a charm. Done.
> 
> Did it get any faster?
Just a little bit:
750 dezicycles in rullgard changes, 1048359 runs, 217 skips
764 dezicycles in previous function, 1048540 runs, 36 skips



>>>> +    // 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...
Indeed, 2 of 3. Factorized.



>>>> +int8_t parcor_rice_table[3][20][2] = {
>>>> ...
> I usually do something like this:
> 
> int8_t parcor_rice_table[3][20][2] = {
>     { { -52, 4 }, { xx, yy }, { zz, ww }, { ...    }
>       /* ... */
>       { ...    }, { ...    }, { ...    }, { ...    } },
> };
> 
>>>> ...
> Here I'd do like this:
> 
> int32_t parcor_scaled_values[] =
>     -1048544, -1048288, -1047776, -1047008,
> };
> 
Done.

All changes included in revision 5.

Thanks!

-Thilo