[FFmpeg-devel] [PATCH 11/15] lavc/on2avc: replace pow(10, x) by exp10(x)

[Ganesh Ajjanagadde]

On Sun, Dec 27, 2015 at 8:14 AM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Sun, Dec 27, 2015 at 08:00:52AM -0800, Ganesh Ajjanagadde wrote:
>> On Sun, Dec 27, 2015 at 4:31 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
>> > Hi,
>> >
>> > On Sat, Dec 26, 2015 at 8:23 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>> > wrote:
>> >>
>> >> On Sat, Dec 26, 2015 at 4:20 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>> >> wrote:
>> >> > On Sat, Dec 26, 2015 at 4:08 PM, James Almer <jamrial at gmail.com> wrote:
>> >> >> On 12/23/2015 3:47 PM, Ganesh Ajjanagadde wrote:
>> >> >>> exp10, introduced recently, is superior for the purpose.
>> >> >>>
>> >> >>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> >> >>> ---
>> >> >>>  libavcodec/on2avc.c | 5 +++--
>> >> >>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> >> >>>
>> >> >>> diff --git a/libavcodec/on2avc.c b/libavcodec/on2avc.c
>> >> >>> index 04c8e41..0409b3e 100644
>> >> >>> --- a/libavcodec/on2avc.c
>> >> >>> +++ b/libavcodec/on2avc.c
>> >> >>> @@ -22,6 +22,7 @@
>> >> >>>
>> >> >>>  #include "libavutil/channel_layout.h"
>> >> >>>  #include "libavutil/float_dsp.h"
>> >> >>> +#include "libavutil/libm.h"
>> >> >>>  #include "avcodec.h"
>> >> >>>  #include "bytestream.h"
>> >> >>>  #include "fft.h"
>> >> >>> @@ -934,9 +935,9 @@ static av_cold int
>> >> >>> on2avc_decode_init(AVCodecContext *avctx)
>> >> >>>                 "Stereo mode support is not good, patch is
>> >> >>> welcome\n");
>> >> >>>
>> >> >>>      for (i = 0; i < 20; i++)
>> >> >>> -        c->scale_tab[i] = ceil(pow(10.0, i * 0.1) * 16) / 32;
>> >> >>> +        c->scale_tab[i] = ceil(exp10(i * 0.1) * 16) / 32;
>> >> >>>      for (; i < 128; i++)
>> >> >>> -        c->scale_tab[i] = ceil(pow(10.0, i * 0.1) * 0.5);
>> >> >>> +        c->scale_tab[i] = ceil(exp10(i * 0.1) * 0.5);
>> >> >>>
>> >> >>>      if (avctx->sample_rate < 32000 || avctx->channels == 1)
>> >> >>>          memcpy(c->long_win, ff_on2avc_window_long_24000,
>> >> >>
>> >> >> This apparently broke ICC
>> >> >>
>> >> >>
>> >> >> http://fate.ffmpeg.org/report.cgi?time=20151226215846&slot=x86_64-linux-gnu-icc-2011.4.191
>> >> >>
>> >> >> http://fate.ffmpeg.org/report.cgi?time=20151226235348&slot=x86_64-linux-gnu-icc-2011_sp1.13.367
>> >> >>
>> >> >> http://fate.ffmpeg.org/report.cgi?time=20151226203729&slot=x86_64-archlinux-icc-2013
>> >> >
>> >> > Thanks for the report. A couple of questions:
>> >> > 1. Is there an easy way to acquire icc so that I can reproduce?
>> >> > GCC/Clang are perfectly fine with it.
>> >> > 2. Do you know what the ICC platforms use for exp2?
>> >> >
>> >> > In the absence of remarks and my own inability to fix this, I will
>> >> > revert tonight.
>> >> > BTW, please let me know the general policy for this kind of breakage:
>> >> > i.e, how quickly do such regressions need to be fixed.
>> >>
>> >> Different fix pushed that also speeds up things.
>> >
>> >
>> > You shouldn't push things without review...
>>
>> I assumed this does not apply to things that were broken by commits
>> made by one self, and also assumed that regressions should be fixed
>> asap, proper fixes coming in later as Michael posted now.
>>
>> Guess I was wrong here, noted this for the future. Thanks.
>
> IMO its ok to revert ones own commits if they break something

thanks for this statement: what I did was not a simple revert, so I
guess I should have asked.

>
> its also ok to commit things that everyone is happy with
> like for example if you forget a ";" adding it is perfectly ok without
> patch, noone will be unhappy about that.
> the more complex a patch is and the more close it is to other peoples
> authorship/maitaince the more likely someone else will be unhappy
>
> sending patches vs. directly commiting so that work and latency is
> minimized and everyone is happy is not always trivial

There is only one thing I am a bit unhappy about here: the patch you
pushed: since it was related to code I write, and I am active on the
ML here, I would have appreciated at least obtaining my opinion on the
patch before pushing.

It would have still been a nack, but as it is not technically wrong, I
would not have opposed it if someone else acks it, like Ronald in this
case.

I guess you and Ronald are unhappy with what I did to fix the broken
change. Overall, seems like simple miscommunication, and everything is
back to normal here.

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I have never wished to cater to the crowd; for what I know they do not
> approve, and what they approve I do not know. -- Epicurus
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>