On Thu, Nov 19, 2015 at 11:12 AM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: > On Thu, Nov 19, 2015 at 11:03 AM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: >> On Thu, Nov 19, 2015 at 10:53 AM, James Darnley <james.darn...@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@ffmpeg.org >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel