[FFmpeg-devel] [PATCH] WMA Voice decoder

[Vitor Sessak]

Ronald S. Bultje wrote:
> Hi,
> 
> On Sat, Jan 30, 2010 at 8:06 PM, Vitor Sessak <vitor1001 at gmail.com> wrote:
>> Ronald S. Bultje wrote:
>>> +    if (frame_desc->acb_type == 1) {
>>> +        for (n = 0; n < size; n++) {
>>> +            int pitch_sh8 = (s->last_pitch_val << 8) +
>>> +                ((s->pitch_diff_sh16 * (block_idx * size + n)) >> 8);
>>> +            int pitch = (pitch_sh8 + 0x6F) >> 8;
>>> +            idx = (((pitch << 8) - pitch_sh8) * 8 + 0x80) >> 8;
>>> +            //FIXME don't call this with len=1
>>> +            ff_acelp_interpolatef(&excitation[n], &excitation[n - pitch],
>>> +                                  ff_wmavoice_ipol1_coeffs, 17,
>>> +                                  (idx >= 0) | (abs(idx) << 1), 9, 1);
>> I think you can at least call this with len=s->pitch_diff_sh16 or something
>> like that (since pitch_sh8 seems to change only after a certain fixed number
>> of iterations). Also it will not stay in the way for SIMD optimizations.
> 
> How about this?
> 
>         int len;
>         for (n = 0; n < size; n += len) {
>             int next_idx_sh16;
>             int abs_idx    = block_idx * size + n;
>             int pitch_sh16 = (s->last_pitch_val << 16) +
>                              s->pitch_diff_sh16 * abs_idx;
>             int pitch      = (pitch_sh16 + 0x6FFF) >> 16;
>             int idx_sh16   = ((pitch << 16) - pitch_sh16) * 8 + 0x8000;
>             idx            = idx_sh16 >> 16;
>             if (s->pitch_diff_sh16) {
>                 if (s->pitch_diff_sh16 > 0) {
>                     next_idx_sh16 = (idx_sh16) &~ 0xFFFF;
>                 } else
>                     next_idx_sh16 = (idx_sh16 + 0x10000) &~ 0xFFFF;

If you could get rid of a few of the shifts, readability would certainly 
improve.

>                 len = av_clip((idx_sh16 - next_idx_sh16) /
> s->pitch_diff_sh16 / 8,
>                               1, size - n);
>             } else
>                 len = size;
> 
>             ff_acelp_interpolatef(&excitation[n], &excitation[n - pitch],
>                                   wmavoice_ipol1_coeffs, 17,
>                                   (idx >= 0) + abs(idx) * 2, 9, len);
>         }
> 
> Little bit ugly, but this is my first try which actually works.
> Sometimes len is one too small, not sure why, so then
> ff_acelp_interpolatef() is called twice with the same value, once with
> len = one too short and once with len=1. That shouldn't be too bad. If
> someone has good ideas on how to de-uglify this (or just outright
> rejects it), let me know. Otherwise, it'll be part of the next patch.

IMO it looks much better than previous version. What is the typical 
value of len? I hope that if the calculation of pitch_* above gets 
simplified, we can see a better way of doing it.

-Vitor