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

[Ganesh Ajjanagadde]

On Thu, Nov 19, 2015 at 11:12 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> On Thu, Nov 19, 2015 at 11:03 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>> 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.
>
> Confirmed just now bit-exactness via the hex print:
> 0x1.921fb54442d18p+1
> 0x1.921fb54442d18p+1.

Dug up old ML, now understood where this comes from:
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-July/175768.html. I
also addressed precision concerns in a brief reply to Paul for the
recent patch I sent and asked for more info:
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-November/183111.html.

@Paul: can you please be a little more verbose in future? Instead of a
"drop this" with no explanation whatsoever, if you provided the old ML
link, I could have addressed those concerns without resorting to this
rather unnatural and not pleasant "one day else push" behavior. I try
to address all concerns, but my hands are tied unless concerns are
expressed explicitly.

I apologize for all misunderstanding.

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