diff mbox

[FFmpeg-devel] aacsbr_fixed: prevent sbr gain from being zero

Message ID 411427cc-6dec-62cc-4795-0b7c57b47004@googlemail.com
State Accepted
Headers show

Commit Message

Andreas Cadhalpun Nov. 24, 2016, 11:31 p.m. UTC
On 24.11.2016 18:29, Michael Niedermayer wrote:
> On Thu, Nov 24, 2016 at 01:15:54AM +0100, Andreas Cadhalpun wrote:
>> On 23.11.2016 03:22, Michael Niedermayer wrote:
>>> On Sun, Nov 13, 2016 at 09:29:11PM +0100, Andreas Cadhalpun wrote:
>>>> It causes division by zero crashes.
>>>>
>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>>>> ---
>>>>  libavcodec/aacsbr_fixed.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/libavcodec/aacsbr_fixed.c b/libavcodec/aacsbr_fixed.c
>>>> index b26314a..d2a7027 100644
>>>> --- a/libavcodec/aacsbr_fixed.c
>>>> +++ b/libavcodec/aacsbr_fixed.c
>>>> @@ -429,6 +429,10 @@ static void sbr_gain_calc(AACContext *ac, SpectralBandReplication *sbr,
>>>>                                                  av_add_sf(FLOAT_1, sbr->e_curr[e][m]),
>>>>                                                  av_add_sf(FLOAT_1, sbr->q_mapped[e][m]))));
>>>>                  }
>>>> +                if (sbr->gain[e][m].mant == 0) {
>>>> +                    sbr->gain[e][m] = FLOAT_1;
>>>> +                    sbr->gain[e][m].exp = MIN_EXP;
>>>> +                }
>>>
>>> Why is this not not needed for the float code ?
>>
>> Because float can represent smaller absolute values than normalized SoftFloat.
>>
>>> is there a shortcomming in the SoftFloat code ?
>>
>> In a way, because the SoftFloat gets normalized, e.g.:
>>  * float: 2.16840434e-19 * 2.77555756e-17 / 10337.5293 = 5.82201677e-40
>>  * SoftFloat: av_div_sf(av_mul_sf({mant = 536870912, exp = -61}, {mant = 536870912, exp = -54}),
>>                         {mant = 677439305, exp = 14}) = {mant = 850858499, exp = -130} = FLOAT_0
> 
> hmm
> does increasing the limits for SoftFloats exponent solve this ?

Yes, patch is attached.

> If that works and doesnt break anything it would be good to have
> SoftFloat cover the range float has

I'm not entirely sure about not breaking anything. For example the output of
the softfloat test changes, but for some reason that test is not enabled anyway.

Best regards,
Andreas

Comments

Michael Niedermayer Nov. 25, 2016, 12:12 a.m. UTC | #1
On Fri, Nov 25, 2016 at 12:31:28AM +0100, Andreas Cadhalpun wrote:
> On 24.11.2016 18:29, Michael Niedermayer wrote:
> > On Thu, Nov 24, 2016 at 01:15:54AM +0100, Andreas Cadhalpun wrote:
> >> On 23.11.2016 03:22, Michael Niedermayer wrote:
> >>> On Sun, Nov 13, 2016 at 09:29:11PM +0100, Andreas Cadhalpun wrote:
> >>>> It causes division by zero crashes.
> >>>>
> >>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> >>>> ---
> >>>>  libavcodec/aacsbr_fixed.c | 4 ++++
> >>>>  1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/libavcodec/aacsbr_fixed.c b/libavcodec/aacsbr_fixed.c
> >>>> index b26314a..d2a7027 100644
> >>>> --- a/libavcodec/aacsbr_fixed.c
> >>>> +++ b/libavcodec/aacsbr_fixed.c
> >>>> @@ -429,6 +429,10 @@ static void sbr_gain_calc(AACContext *ac, SpectralBandReplication *sbr,
> >>>>                                                  av_add_sf(FLOAT_1, sbr->e_curr[e][m]),
> >>>>                                                  av_add_sf(FLOAT_1, sbr->q_mapped[e][m]))));
> >>>>                  }
> >>>> +                if (sbr->gain[e][m].mant == 0) {
> >>>> +                    sbr->gain[e][m] = FLOAT_1;
> >>>> +                    sbr->gain[e][m].exp = MIN_EXP;
> >>>> +                }
> >>>
> >>> Why is this not not needed for the float code ?
> >>
> >> Because float can represent smaller absolute values than normalized SoftFloat.
> >>
> >>> is there a shortcomming in the SoftFloat code ?
> >>
> >> In a way, because the SoftFloat gets normalized, e.g.:
> >>  * float: 2.16840434e-19 * 2.77555756e-17 / 10337.5293 = 5.82201677e-40
> >>  * SoftFloat: av_div_sf(av_mul_sf({mant = 536870912, exp = -61}, {mant = 536870912, exp = -54}),
> >>                         {mant = 677439305, exp = 14}) = {mant = 850858499, exp = -130} = FLOAT_0
> > 
> > hmm
> > does increasing the limits for SoftFloats exponent solve this ?
> 
> Yes, patch is attached.
> 
> > If that works and doesnt break anything it would be good to have
> > SoftFloat cover the range float has
> 
> I'm not entirely sure about not breaking anything. For example the output of
> the softfloat test changes, but for some reason that test is not enabled anyway.
> 
> Best regards,
> Andreas

>  softfloat.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 17b9929a3b2f3b064cc1f96c7ed48dfe377f191a  0001-softfloat-decrease-MIN_EXP-to-cover-full-float-range.patch
> From e28c5828a62c583152834f661f9a15b572b07eee Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> Date: Fri, 25 Nov 2016 00:26:51 +0100
> Subject: [PATCH] softfloat: decrease MIN_EXP to cover full float range
> 
> floats are not necessarily normalized, so a normalized softfloat needs
> MIN_EXP lowered by 23 to cover that range.
> 
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavutil/softfloat.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

LGTM

thx

[...]
Andreas Cadhalpun Nov. 25, 2016, 12:25 a.m. UTC | #2
On 25.11.2016 01:12, Michael Niedermayer wrote:
> On Fri, Nov 25, 2016 at 12:31:28AM +0100, Andreas Cadhalpun wrote:
>>  softfloat.h |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 17b9929a3b2f3b064cc1f96c7ed48dfe377f191a  0001-softfloat-decrease-MIN_EXP-to-cover-full-float-range.patch
>> From e28c5828a62c583152834f661f9a15b572b07eee Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> Date: Fri, 25 Nov 2016 00:26:51 +0100
>> Subject: [PATCH] softfloat: decrease MIN_EXP to cover full float range
>>
>> floats are not necessarily normalized, so a normalized softfloat needs
>> MIN_EXP lowered by 23 to cover that range.
>>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavutil/softfloat.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> LGTM

Pushed.

Best regards,
Andreas
diff mbox

Patch

From e28c5828a62c583152834f661f9a15b572b07eee Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
Date: Fri, 25 Nov 2016 00:26:51 +0100
Subject: [PATCH] softfloat: decrease MIN_EXP to cover full float range

floats are not necessarily normalized, so a normalized softfloat needs
MIN_EXP lowered by 23 to cover that range.

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavutil/softfloat.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/softfloat.h b/libavutil/softfloat.h
index 48d0d59..fa91d1e 100644
--- a/libavutil/softfloat.h
+++ b/libavutil/softfloat.h
@@ -27,7 +27,7 @@ 
 #include "avassert.h"
 #include "softfloat_tables.h"
 
-#define MIN_EXP -126
+#define MIN_EXP -149
 #define MAX_EXP  126
 #define ONE_BITS 29
 
-- 
2.10.2