[FFmpeg-devel] [PATCH] WMA Voice decoder

[Ronald S. Bultje]

Hi,

2010/1/20 M?ns Rullg?rd <mans at mansr.com>:
> "Ronald S. Bultje" <rsbultje at gmail.com> writes:
>> 2010/1/20 M?ns Rullg?rd <mans at mansr.com>:
>>> "Ronald S. Bultje" <rsbultje at gmail.com> writes:
>>>> +static int
>>>> +get_vbm_bits(GetBitContext *gb)
>>>> +{
>>>> + ? ?int n, res;
>>>> +
>>>> + ? ?for (n = 0; ; n++) {
>>>> + ? ? ? ?res = get_bits(gb, 2);
>>>> + ? ? ? ?if (res < 3 || n == 6 /** don't increase n to 7 */)
>>>> + ? ? ? ? ? ?break;
>>>> + ? ?}
>>>> +
>>>> + ? ?return 3 * n + res;
>>>> +}
>>>
>>> Is this called a lot? ?If yes, it can be optimised.
>>
>> Once per frame (160 samples), so not really.
>
> Damn, bit-hacking is fun. ;-)

I didn't say you shouldn't do it. :-).

>>>> + ? ? ? ? ? ? ? ?pitch[n] = (fac ? ? ? ? ? ? ? ? * s->cur_pitch_val +
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?(n_blocks_x2 - fac) * s->last_pitch_val +
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?frame_descs[bd_idx].n_blocks) / n_blocks_x2;
>>>
>>> These could use MUL16/MAC16.
>>
>> Stupid question: why? (I seem to recall Michael claiming at some point
>> that he didn't want functions / macros that would basically do
>> something like this, I think he literally gave multiplication as an
>> example, and lo-and-behold, there's MUL16! - so why is this
>> preferrably over a generic multiplication?)
>
> The operands here are restricted to 16 bits, but gcc has no way of
> knowing that. ?16x16 multiplications are often twice as fast as
> 32x32. ?Using int16_t is not appropriate either, since then gcc would
> be constantly masking the value to ensure it stays within 16 bits even
> though we know it will never overflow. ?Until compilers acquire some
> means of providing such hints (and they make use of them), we're stuck
> with macros like these.

Ok, done then. I've also added a log_n_blocks field and use that +
shift instead of the div, should be faster. (n_blocks is always a
power of 2.)

>>>> +static void copy_bits(PutBitContext *pb,
>>>> + ? ? ? ? ? ? ? ? ? ? ?const uint8_t *data, int size,
>>>> + ? ? ? ? ? ? ? ? ? ? ?GetBitContext *gb, int nbits)
>>>> +{
>>>> + ? ?int rmn_bytes, rmn_bits;
>>>> +
>>>> + ? ?rmn_bits = rmn_bytes = get_bits_left(gb);
>>>> + ? ?if (rmn_bits < nbits)
>>>> + ? ? ? ?return;
>>>> + ? ?rmn_bits &= 7; rmn_bytes >>= 3;
>>>> + ? ?if ((rmn_bits = FFMIN(rmn_bits, nbits)) > 0)
>>>> + ? ? ? ?put_bits(pb, rmn_bits, get_bits(gb, rmn_bits));
>>>> + ? ?ff_copy_bits(pb, data + size - rmn_bytes,
>>>> + ? ? ? ? ? ? ? ? FFMIN(nbits - rmn_bits, rmn_bytes << 3));
>>>> +}
>>>
>>> This should probably go somewhere more generic.
>>
>> Yes, in wma.c, because wmapro & wma1/2 also use it. I was hoping
>> nobody would notice and I could do that after the initial commit, but
>> I guess I'm doomed. Damn it. Pretty please?
>
> I see nothing wma-specific about that. ?I think it should go right
> next to ff_copy_bits with a suitable name.

OK, you want that before or after the patch itself hits SVN?

I'll first work on doing some serious float vs. double testing for the
various formats, as Michael & you suggested, to see if float
significantly changes output of g729, amr, sipr or wmavoice. If not,
we'll convert everything to float and I'll use float everywhere here.
If output does change, then I'll see how many doubles we can convert
to float (or float-functions, such as cosf()) in wmavoice.c at the
very least.

Vitor also asked me to look into ff_acelp_interpolatef(), so I'm doing
that right now and will send a new patch once that's done.

Ronald