[FFmpeg-devel] [PATCH] ALS decoder

Thilo Borgmann thilo.borgmann
Sun Aug 30 15:21:24 CEST 2009


Reimar D?ffinger schrieb:
> On Sun, Aug 30, 2009 at 02:01:27AM +0200, Thilo Borgmann wrote:
>> 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)
> 
> Silly me...
> (k - 1) / 2 - 1 = (k + 1) / 2 - 2 of course.
> (previous values if any being i = 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).
> 
> Which makes the correct formula i <= r as I originally said.
> For the even case, i == r is never possible, and for the odd case the
> loop must be run with i == r.
> 
>> i < r + 1 does the trick, but I have no mathematical derivation nor a
>> test for odd or even k.
> 
> i < r + 1 for integers and unless you have an overflow is the same as i
> <= r
Hm... I tried it _again_ and it works... so somehow I must have messed
up my first test with <=.


> 
>> 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?
> 
> It could more easily be unrolled, but I doubt this loop would gain much from it, not
> to mention the compiler being unlikely to do it.
> On the other hand, "(k + 1) >> 1" eats up one extra register.
And one operation less with <=.


> 
>>>>>> +        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.
> 
> Yes that one I do agree with, which is why I usually suggest it most
> when the if part is much larger than the else part (partially true here)
> since that to me is an indication that the if part is a special case
> (if you look at it the right way at least). But since you know the
> algorithm and I don't, do it as you want.
> Though at least the {} of the else could be removed...
Oh, I've already heard otherwise from people on the list and it makes
perfect sense to me to use {} for both branches even if just one of them
needs it. I like the consistency.



>>>>>> +        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()...
> 
> I don't really like "hiding" such variable modifications in the for
> loop, it is unexpected. But I admit either one will probably be
> confusing when you see it the first time so from my side do whichever
> you prefer.
I agree. So I would like to stick to that "hiding" for possible
one-liner loops if nobody vetos on that.


The <= condition will be part of the next revision, thanks!

-Thilo



More information about the ffmpeg-devel mailing list