[FFmpeg-devel] [PATCH] ALS decoder

Thilo Borgmann thilo.borgmann
Sun Aug 30 02:01:27 CEST 2009


Reimar D?ffinger schrieb:
> On Sun, Aug 30, 2009 at 12:02:15AM +0200, Thilo Borgmann wrote:
>>> 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.
> 
> Ok, let's check it out.
> If k is even, the loop ends with
> i = k / 2
> and
> r = k - 1 - k / 2 = k / 2 - 1
> (previous values if any being i = k / 2 - 1 and r = k / 2)
> 
> If k is odd, the loop ends with
> i = (k + 1) / 2
> and
> r = k - 1 - (k + 1) / 2 = (k - 1) / 2 - 1 = (k + 1) / 2
> (previous values if any being i = (k + 1) / 2 - 1 and r = (k + 1) / 2 + 1)
> 
> So i < r should do it (no idea if it is better or worse speed wise, but
> I think it makes the intention clearer).
i < r + 1 does the trick, but I have no mathematical derivation nor a
test for odd or even k.
Also, I don't know much about optimization, but might it be that for the
right hand side of the condition "(k + 1) >> 1" can be optimized better
since it is a static value during the loop while "r + 1" changes with
each iteration?

> 
>>>> +        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.
> 
> The question is why you want to avoid the "unnecessary assignment"?
> 
>>>> +        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.
> 
> I have no real problem accepting it, but I was expecting some kind of
> reason, the compiler is very likely to generate the same code, and if
> not the "if..else" is not particularly likely to be faster.
> Do you actually consider it more readable? Closer to the spec? ...?
Well I want to avoid it because "a=3; if(x) a=4;" is some kind of
strange. Even more, if the case x==true is true for all instances (like
sconf->floating is a global setting never to change within one stream).

Second, a "if(x) a=1; else a=2;" indicates to me the exclusive left hand
/ right hand case for a, while a "a=1; if(x) a=2;" tells me something
about a more special case.

Sorry I can't find better words...


> 
>>>> +        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.
> 
> Nono, it's okay as it was then, I noticed that "trick" in the other
> loops, here for some reason I simply didn't see that start was used to
> initialize k.
Ok, what about the new version from rev. 13?
Although it would not lead to one-line loops, I would like to move other
xyz = 0 from the end of a loop to the for()-definition (like ra_frame =
0; in decode_blocks_ind()) - if that would be generally ok. Of course,
something like raw_sample += div_blocks[b] is too much to be in the for()...

-Thilo



More information about the ffmpeg-devel mailing list