[FFmpeg-devel] [PATCHv2 2/2] avcodec/sinewin_tablegen: use sin instead of sinf for fixed point to improve precision

[Ganesh Ajjanagadde]

On Sun, Mar 13, 2016 at 8:09 PM, Marton Balint <cus at passwd.hu> wrote:
>
> On Sun, 13 Mar 2016, Ganesh Ajjanagadde wrote:
>
>> On Sun, Mar 13, 2016 at 6:50 PM, Marton Balint <cus at passwd.hu> wrote:
>>>
>>> Signed-off-by: Marton Balint <cus at passwd.hu>
>>> ---
>>>  libavcodec/sinewin_tablegen.h | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libavcodec/sinewin_tablegen.h
>>> b/libavcodec/sinewin_tablegen.h
>>> index 4432135..9c912aa 100644
>>> --- a/libavcodec/sinewin_tablegen.h
>>> +++ b/libavcodec/sinewin_tablegen.h
>>> @@ -50,9 +50,9 @@ SINETABLE(8192);
>>>  #endif
>>>
>>>  #if USE_FIXED
>>> -#define SIN_FIX(a) (int)floor((a) * 0x80000000 + 0.5)
>>> +#define SIN_FIX(a) (int)floor(sin(a) * 0x80000000 + 0.5)
>>
>>
>> Note that lrint may be preferred, it is faster and better rounded.
>> Hosts/hardcoded tables can use lavu/tablegen.h, which fall back to
>> floor based hacks.
>
>
> Ok.
>
>>
>>>  #else
>>> -#define SIN_FIX(a) a
>>> +#define SIN_FIX(a) sinf(a)
>>>  #endif
>>
>>
>> Also, why is it essential that SIN_FIX is redefined, can't you just
>> change sinf to sin on line 69?
>
>
> I just didn't want to touch existing floating point code, sin may be slower
> then sinf (?), and if the result is rounded to float from double anyway,
> what is the point.

Keep in mind that: double x; float y = sinf(x) and double x; float y =
sin(x) do not necessarily give the same result on a fixed platform,
often results in a few ulp differences due to lack of composability of
rounding with functions. This in 99.99% of the cases won't matter; I
believe our platforms libm's anyway have that much variation. In this
case, it seems like you anyway tested this, so it should be fine to
ignore.

>
>>>
>>>  SINETABLE_CONST INTFLOAT * const AAC_RENAME(ff_sine_windows)[] = {
>>> @@ -66,7 +66,7 @@ SINETABLE_CONST INTFLOAT * const
>>> AAC_RENAME(ff_sine_windows)[] = {
>>>  av_cold void AAC_RENAME(ff_sine_window_init)(INTFLOAT *window, int n) {
>>>      int i;
>>>      for(i = 0; i < n; i++)
>>> -        window[i] = SIN_FIX(sinf((i + 0.5) * (M_PI / (2.0 * n))));
>>> +        window[i] = SIN_FIX((i + 0.5) * (M_PI / (2.0 * n)));
>>>  }
>>
>>
>> Could you please add a link in the commit message describing more
>> verbosely why you need this (e.g which platform)?
>
>
> Ok, as far as I recall the fixed point table was different on x86_32 and
> x86_64, x86_32 was precise, x86_64 wasn't which is strange.
>
> Regards,
> Marton
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel