[FFmpeg-devel] [PATCH] ALS decoder

[Reimar Döffinger]

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

> 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.

> >>>> +        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...

> Sorry I can't find better words...

Good enough for me.

> >>>> +        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.