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

[Michael Niedermayer]

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

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


[...]
-- 
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.
-------------- 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/03181a57/attachment.sig>