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

[Ganesh Ajjanagadde]

On Sat, Dec 26, 2015 at 6:19 PM, Michael Niedermayer
<michael at 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 <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.
>
> 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>