[FFmpeg-devel] [PATCH] avcodec/on2avc: Fix stability issues with scale_tab generation

[Michael Niedermayer]

From: Michael Niedermayer 

This also simplifies the code
the resulting values are binary identical to what pow(10, i/10.0) produces
---
 libavcodec/on2avc.c |   37 -
 1 file changed, 4 insertions(+), 33 deletions(-)

diff --git a/libavcodec/on2avc.c b/libavcodec/on2avc.c
index 3309d99..e6ccd88 100644
--- a/libavcodec/on2avc.c
+++ b/libavcodec/on2avc.c
@@ -912,23 +912,7 @@ static av_cold void on2avc_free_vlcs(On2AVCContext *c)
 static av_cold int on2avc_decode_init(AVCodecContext *avctx)
 {
 On2AVCContext *c = avctx->priv_data;
-int i, ph;
-/* 10^(i*0.1) for 0 <= i < 10 */
-/* TODO: possibly statically allocate scale_tab; this may help with FATE
- * and reproducibility if the binary size is not impacted much */
-static const double exp10_lut[] = {
-1,
-1.2589254117941673,
-1.5848931924611136,
-1.9952623149688795,
-2.5118864315095806,
-3.1622776601683795,
-3.9810717055349727,
-5.0118723362727229,
-6.3095734448019334,
-7.9432823472428158,
-};
-int64_t exp10_base = 10;
+int i;
 
 if (avctx->channels > 2U) {
 avpriv_request_sample(avctx, "Decoding more than 2 channels");
@@ -950,23 +934,10 @@ static av_cold int on2avc_decode_init(AVCodecContext 
*avctx)
 av_log(avctx, AV_LOG_WARNING,
"Stereo mode support is not good, patch is welcome\n");
 
-
-/* Fast and more accurate way of doing for (i = 0; i < 20; i++)
-c->scale_tab[i] = ceil(ff_exp10(i * 0.1) * 16) / 32;
+for (i = 0; i < 20; i++)
+c->scale_tab[i] = ceil(ff_exp10(i * 0.1) * 16 - 0.01) / 32;
 for (; i < 128; i++)
-c->scale_tab[i] = ceil(ff_exp10(i * 0.1) * 0.5); */
-for (i = 0; i < 10; i++) {
-c->scale_tab[i] = ceil(exp10_lut[i] * 16) / 32;
-c->scale_tab[i+10] = ceil(exp10_lut[i] * 160) / 32;
-}
-
-for (i = 20, ph = 0; i < 128; i++, ph++) {
-if (i % 10 == 0) {
-exp10_base *= 10;
-ph = 0;
-}
-c->scale_tab[i] = ceil(exp10_base * exp10_lut[ph] * 0.5);
-}
+c->scale_tab[i] = ceil(ff_exp10(i * 0.1) * 0.5 - 0.01);
 
 if (avctx->sample_rate < 32000 || avctx->channels == 1)
 memcpy(c->long_win, ff_on2avc_window_long_24000,
-- 
1.7.9.5

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/on2avc: Fix stability issues with scale_tab generation

[Michael Niedermayer]

On Sun, Dec 27, 2015 at 07:40:53AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Sun, Dec 27, 2015 at 6:02 AM, Michael Niedermayer 
> wrote:
> 
> > +for (i = 0; i < 20; i++)
> > +c->scale_tab[i] = ceil(ff_exp10(i * 0.1) * 16 - 0.01) / 32;
> >  for (; i < 128; i++)
> > +c->scale_tab[i] = ceil(ff_exp10(i * 0.1) * 0.5 - 0.01);
> >
> 
> To innocent bystanders, this is hard to understand. Let's keep it a habit
> to document things (what and why), where the "what" portion is probably
> "this code emulates pow(10, x) with ff_pow10(x - 0.01)". As for why (most
> specifically, why the 0.01?), I'm going to assume that here, you're trying
> to get unit integers to not go to unit.000[..]001, so you subtract 0.01
> before the ceil so it works fine again to get the exact unit integer output
> number. If that's correct, please add a comment saying that, and then lgtm.

yes
changed
applied
thx


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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/on2avc: Fix stability issues with scale_tab generation

[Ganesh Ajjanagadde]

On Sun, Dec 27, 2015 at 7:44 AM, Michael Niedermayer
 wrote:
> On Sun, Dec 27, 2015 at 07:40:53AM -0500, Ronald S. Bultje wrote:
>> Hi,
>>
>> On Sun, Dec 27, 2015 at 6:02 AM, Michael Niedermayer 
>> wrote:
>>
>> > +for (i = 0; i < 20; i++)
>> > +c->scale_tab[i] = ceil(ff_exp10(i * 0.1) * 16 - 0.01) / 32;
>> >  for (; i < 128; i++)
>> > +c->scale_tab[i] = ceil(ff_exp10(i * 0.1) * 0.5 - 0.01);
>> >
>>
>> To innocent bystanders, this is hard to understand. Let's keep it a habit
>> to document things (what and why), where the "what" portion is probably
>> "this code emulates pow(10, x) with ff_pow10(x - 0.01)". As for why (most
>> specifically, why the 0.01?), I'm going to assume that here, you're trying
>> to get unit integers to not go to unit.000[..]001, so you subtract 0.01
>> before the ceil so it works fine again to get the exact unit integer output
>> number. If that's correct, please add a comment saying that, and then lgtm.

In my view this change is not good for the exact reasons above. It is
a "magic constant" that is a pure hack. The commit I made made
accuracy more consistent across platforms (instead of relying on the
whims of libm's pow/exp2), and was commented so that intent was clear.
Nack from me.

>
> yes
> changed
> applied
> thx
>
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Concerning the gods, I have no means of knowing whether they exist or not
> or of what sort they may be, because of the obscurity of the subject, and
> the brevity of human life -- Protagoras
>
> ___
> 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