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

[Michael Niedermayer]

On Sat, Dec 26, 2015 at 06:37:31PM -0800, Ganesh Ajjanagadde wrote:
> 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.

patch posted

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151227/dfd9e08c/attachment.sig>