[FFmpeg-devel] [PATCH] ALS decoder

[Reimar Döffinger]

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

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

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