[FFmpeg-devel] [PATCH] all: use M_SQRT1_2, M_SQRT2, M_PI

[Ganesh Ajjanagadde]

On Thu, Nov 19, 2015 at 10:53 AM, James Darnley <james.darnley at gmail.com> wrote:
> On 2015-11-19 13:52, Ganesh Ajjanagadde wrote:
>> diff --git a/libavfilter/af_dynaudnorm.c b/libavfilter/af_dynaudnorm.c
>>> index 8f0c2d0..62a2653 100644
>>> --- a/libavfilter/af_dynaudnorm.c
>>> +++ b/libavfilter/af_dynaudnorm.c
>>> @@ -227,8 +227,6 @@ static int cqueue_pop(cqueue *q)
>>>      return 0;
>>>  }
>>>
>>> -static const double s_pi = 3.1415926535897932384626433832795028841971693993751058209749445923078164062862089986280348253421170679;
>>> -
>>>  static void init_gaussian_filter(DynamicAudioNormalizerContext *s)
>>>  {
>>>      double total_weight = 0.0;
>>> @@ -238,7 +236,7 @@ static void init_gaussian_filter(DynamicAudioNormalizerContext *s)
>>>
>>>      // Pre-compute constants
>>>      const int offset = s->filter_size / 2;
>>> -    const double c1 = 1.0 / (sigma * sqrt(2.0 * s_pi));
>>> +    const double c1 = 1.0 / (sigma * sqrt(2.0 * M_PI));
>>>      const double c2 = 2.0 * pow(sigma, 2.0);
>>>
>
> Paul asked you to drop this.  I didn't see whether he replied that time
> but when I asked why he didn't use M_PI when he first created the
> filter, I got this reply.

I never got the reply since unless I am mistaken, such a reply never
came on the ML. It was in the absence of this info that I gave an
additional day to give some benefit of doubt. Such replies belong on
the ML IMHO, since they affect the review of the patch.

>
>>>> +static const double s_pi =
>>>> 3.1415926535897932384626433832795028841971693993751058209749445923078164062862089986280348253421170679;
>>>
>>> Why not use the standard M_PI define?
>>
>> Its too short. And I try hard to produce exact same output as
>> reference implementation as possible, even if doubles are used.

If it is too short, it is a bug with libm which is highly unlikely and
needs to be substantiated with concrete evidence. Such evidence is
easy to obtain via e.g av_double2int, or the hexadecimal format
specifier (C99) for floats. The expansion here is of a ludicrous
length and gives no value whatsoever.

>>
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>