[FFmpeg-devel] [PATCH] ALS decoder

[Thilo Borgmann]

>>> [...]
>>>> +/** Reads and decodes a Rice codeword.
>>>> + */
>>>> +static int32_t decode_rice(GetBitContext *gb, unsigned int k)
>>>> +{
>>>> +    int max   = gb->size_in_bits - get_bits_count(gb) - k;
>>>> +    int32_t q = get_unary(gb, 0, max);
>>> i suspect int is fine for these 2 instead of int32_t ?
>> Is int at least 32 bits long? k <= 32 and therefore q might have to hold
>> up to 32 bits.
> 
> int in POSIX is at least 32bits long, yes

This makes (u)int32_t useless, doesn't it?


> 
> 
>>
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < (k + 1) >> 1; i++) {
>>>> +        int64_t tmp1 = cof[    i    ] + ((MUL64(par[k], cof[k - i - 1]) + (1 << 19)) >> 20);
>>>> +        int64_t tmp2 = cof[k - i - 1] + ((MUL64(par[k], cof[    i    ]) + (1 << 19)) >> 20);
>>>> +
>>>> +        if (tmp1 < -INT32_MAX || tmp1 > INT32_MAX ||
>>>> +            tmp2 < -INT32_MAX || tmp2 > INT32_MAX)
>>>> +            return -1;
>>>> +
>>>> +        cof[k - i - 1] = tmp2;
>>>> +        cof[    i    ] = tmp1;
>>> if we didnt do overflow checks ...
>>>
>>> int tmp1        = (MUL64(par[k], cof[k - i - 1]) + (1 << 19)) >> 20;
>>> cof[k - i - 1] += (MUL64(par[k], cof[    i    ]) + (1 << 19)) >> 20;
>>> cof[    i    ] += tmp1;
>> I agree mathematically, but this doesn't work - most likely because of
>> the += operator. Used the previous no-check version (no tmp2).
> 
> the only failure i can see quickyl is with odd k and the middle value being
> transformed twice, that is k-i-1 == i
> it might make sense to handle the middle value specially outside the loop

You're right. This way saves around 25% of the decicycles.


> 
> 
> [...]
>>> [...]
>>>> +/** Computes the bytes left to decode for the current frame
>>>> + */
>>>> +static void zero_remaining(unsigned int b, unsigned int b_max,
>>>> +                           unsigned int *div_blocks, int32_t *buf)
>>>> +{
>>> div_blocks should be const, also this likel applies to other arguments of
>>> other functions
>> Are there some common FFmpeg-wide rules to declare something const?
>> I've done what I think might be useful...
> 
> pointers in function arguments should be const when possible as it often
> makes understanding the code easier also it might help some compilers
> optimizing the code better

Ok I think I've const all of these.


Will be part of revision 14. Thanks!

-Thilo