[FFmpeg-devel] [PATCH] WMA Voice postfilter

[Vitor Sessak]

Ronald S. Bultje wrote:
> Hi Vitor,
> 
> On Thu, Mar 18, 2010 at 6:52 PM, Vitor Sessak <vitor1001 at gmail.com> wrote:
>> Ronald S. Bultje wrote:
>>> On Thu, Mar 18, 2010 at 4:33 PM, Vitor Sessak <vitor1001 at gmail.com> wrote:
>>>>> +    /* calculate the Hilbert transform of the gains, which we do (since
>>>>> this
>>>>> +     * is a sinus input) by doing a phase shift (in theory,
>>>>> H(sin())=cos()).
>>>>> +     * Because input is symmetric (mirror above), every im[n] is zero.
>>>>> */
>>>>> +    ff_rdft_calc(&s->rdft, &lpcs[1]);
>>>>> +    lpcs[1] = lpcs[2];
>>>>> +    lpcs[2] = lpcs[0] = 0;
>>>>> +    ff_rdft_calc(&s->irdft, lpcs);
>>>> I think this deserve to be in a separate function (and that would include
>>>> the mirroring), it could be reused in case we need a Hilbert transform in
>>>> another codec. Also I think it should be possible to do it with a half as
>>>> big FFT...
>>> Hm. That's a good idea. Now, I can't assume input to be aligned or
>>> padded, so that'd add an extra memcpy, is that OK?
>> I didn't mean to make it a shared function, but just move the code in its
>> own function, so when any other codec need it will be trivial to make it
>> public. So IMHO you can just assume whatever alignment/padding is needed
>> (but document it).
>>
>> Also, you do both
>>
>>> +    ff_rdft_calc(&s->rdft, &lpcs[1]);
>>> +    ff_rdft_calc(&s->irdft, lpcs);
>> Since either &lpcs[1] or lpcs will be unaligned this code will segfault when
>> compiled with YASM assembly enabled. But there is no point in fixing this
>> before looking in a way to do the Hilbert transform with a buffer half the
>> size. I'll give a look this weekend if I have time.
> 
> The *_calc() on array + 1 is gone now (that was relatively easy),
> leaving the Hilbert transform as per your new functions (and thank to
> your patch) as:
> 
>     /* calculate the Hilbert transform of the gains, which we do (since this
>      * is a sinus input) by doing a phase shift (in theory, H(sin())=cos()). */
>     ff_dct_calc(&s->dct, lpcs);
>     ff_dct_calc(&s->dst, lpcs);

Nice!

> Should that still be its own function? Seems like desperate overkill
> to me, so I didn't yet. If you really want me to, I'll do it.

I don't see the point any more to separate it either.

> New patch with my current version attached, the calc_coeffs() might be
> a bit rough because I basically just changed it to work and didn't pay
> much attention to the comments yet... Comments welcome.

It very likely still crashes with YASM enabled. You are still passing 
buffers to {D,RD}FT that has whatever random alignment the compiler 
decided to give them. You should move some buffers to the context and 
use DECLARE_ALIGNED(16, ...).

> +    for (n = 0x3F; n > 0; n--) {
> +        float sin, cos;
> +
> +        idx = av_clip((n & 1 ? -lpcs[64] : +lpcs[64]) - 2 * lpcs[n - 1], -255, 255);
> +        if (idx >= 0) {
> +            sin =  ff_sine_256[      idx];
> +            cos =  ff_sine_256[255 - idx];
> +        } else {
> +            sin = -ff_sine_256[     -idx];
> +            cos =  ff_sine_256[255 + idx];
> +        }
> +        coeffs[n * 2 + 1] = coeffs[n + 1] * sin;
> +        coeffs[n * 2]     = coeffs[n + 1] * cos;
> +    }

The branches are expensive and can be easily avoided with a bigger table 
and unrolling the loop once.

-Vitor