[FFmpeg-devel] [PATCH] WMA Voice decoder

[Ronald S. Bultje]

Hi,

On Mon, Feb 1, 2010 at 2:35 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> wrote:
> On Sun, Jan 31, 2010 at 09:46:43PM -0500, Ronald S. Bultje wrote:
>> On Sat, Jan 30, 2010 at 6:48 PM, Reimar D?ffinger
>> <Reimar.Doeffinger at gmx.de> wrote:
>> > "short"? Also specifying the type only once is ugly.
>> > Also if you use frame_size with a shift they all can be uint8_t
>> > (though probably it's better to just "waste" those 34 bytes and
>> > leave frame_size 16 bits, but the others don't need to be.
>>
>> OK, so I changed this to be holdable in 32bits per frame type, maybe
>> that's a bit too much just let me know how far I should go.
>
> I'm generally in favour of the most readable, at least as long as we
> are talking about saving a few _bytes_, if it was a kB that'd be different.

(Dis)assembly-readable or source-readable? I mean, readability is not
affected by me adding a :3 to the field.

>> >> + ? ? ? ?for (m = 0; m < num; m++)
>> >> + ? ? ? ? ? ?lsps[m] += base_q[n] + mul_q[n] * table[m + values[n] * num];
>> >
>> > I consider it better to extract constant stuff like values[n]*num outside the loop.
>> > Particularly since due to aliasing rules the compiler might not be able to
>> > to pull e.g. the base_q[n] out without that (hm, I admit I don't know for sure
>> > if the const might be enough for that).
>>
>> I did this for the values[n] * num. For the base_q[n], it seems
>> overkill, I mean, it's marked const so the compiler should know
>> better. I think expanding base_q[n] outside the loop is too little
>> honour for gcc (maybe deserved, but still).
>
> Um, the thing is that lsps and base_q is the same type, if you don't
> add a restrict keyword the compiler usually simply _can't_ know that
> lsps[m] is not the same as base_q[n] and thus _has_ to reload
> base_q[n] each time.
> I agree though that possibly the better solution is to add restrict (even
> though I consider gcc stupid enough that I expect at least some version
> to still get it wrong).

Aha, I just learned what restrict is. So I only have to add restrict
to the three arguments here (also since everything else is const),
right?

static void dequant_lsps(double *restrict lsps, int num,
                         const uint16_t *values,
                         const uint16_t *sizes,
                         int n_stages, const uint8_t *table,
                         const double *restrict mul_q,
                         const double *restrict base_q)
[..]

>> >> + ? ? ? ? ? ?ff_acelp_interpolatef(&excitation[n], &excitation[n - pitch],
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?ff_wmavoice_ipol1_coeffs, 17,
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(idx >= 0) | (abs(idx) << 1), 9, 1);
>> >
>> > Maybe FFABS? Also that kind of thing is quite common, there may be a function for it.
>>
>> it's hard to test the speed implifcation of that because it's so
>> insignificant. I think at some point I said we should test abs() vs.
>> FFABS(), there's no clear guideline for that right now.
>>
>> I couldn't find a function, but I may just be inexperienced in finding
>> them. :-).
>
> There might be only to decode a value encoded that way.

I intend to re-arrange the table values so that I can simply do idx +
4 (idx is in the range [-4,3]). Will follow in my next patch.

Ronald