On Sat, Dec 26, 2015 at 6:19 PM, Michael Niedermayer <mich...@niedermayer.cc> wrote: > On Sat, Dec 26, 2015 at 05:23:55PM -0800, Ganesh Ajjanagadde wrote: >> On Sat, Dec 26, 2015 at 4:20 PM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: >> > On Sat, Dec 26, 2015 at 4:08 PM, James Almer <jamr...@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 <gajjanaga...@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. > > but speed doesnt really matter for this code while maintainability > IMHO matters more
Definitely, I did not do it for speed actually, speed was a side effect - this is also why the first line of the message does not mention speed effect. I did this because it improved accuracy on clang/gcc (as also mentioned in the commit message). In any case, I was looking for a quick fix since I assumed ICC regression is serious, and wanted to do so in a way that improves numerical accuracy. > > either way, the code is numerically unstable as some of the arguments > to ceil() fall exactly on the mid point between different outputs > this is still so after the change and would be in case of a revert too Yes. I think something needs to be done to FATE to fix this, but I have no idea. > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Awnsering whenever a program halts or runs forever is > On a turing machine, in general impossible (turings halting problem). > On any real computer, always possible as a real computer has a finite number > of states N, and will either halt in less than N cycles or never halt. > > _______________________________________________ > 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
