[FFmpeg-devel] [PATCH 1/3] aacsbr_fixed: avoid division by zero in sbr_gain_calc

[Andreas Cadhalpun]

On 16.11.2015 13:46, Michael Niedermayer wrote:
> On Fri, Nov 13, 2015 at 11:19:44PM +0100, Andreas Cadhalpun wrote:
>> Well, unfortunately just rejecting 0 in sbr_dequant is no solution,
>> because, as you noticed, that only happens via underflow.
> 
> a value that has underflowed should be 0, so underflow affecting
> anything implies 0 as result and so a check for 0 would cover all
> underflows
> I think i misunderstand somehow

The problem is that this code manipulates SoftFloat.exp directly.

>> One could check for exponents smaller than MIN_EXP, but since
> 
> exponents must not be smaller than MIN_EXP.
> no *_sf function should set such an exponent. code directly writing
> exponents has to check for exp < MIN_EXP

This code doesn't...

> that could in principle be done by using a _sf function which allows
> such exponents on input and clears it up on output

A function av_exp2_sf properly calculating 2^v for a Softfloat v could
be used to fix this problem.

>> the exponent can get smaller during the calculations in sbr_gain_calc,
>> that wouldn't necessarily avoid the division by 0.
>>
> 
>> Additionally both sbr_dequant and sbr_gain_calc are void functions,
>> so can't easily return errors.
> 
> iam not sure i understand your concern ?
> the resturn type is easy changeable or  flag could be added to the
> context indicating an error or a simpler hack could be used to
> fix this in the releases with a more complete cleanup in master

Changing the return type means changing also the return types of the
functions calling this function and also for the aac float decoder,
which does not fail in this case... and that gives a clue for the
proper solution, see below.

> but maybe iam missing something why you consider this to be a bad
> solution ?

I guess what we both missed is that the actual problem is that the
calculation of noise_facs in the aac_fixed decoder is utterly broken:

First they are set in read_sbr_noise, which only sets mant and not exp,
so for example:
noise_facs[1][0] = {mant = 29, exp = 0}

Then in sbr_dequant we have (comments mine):
for (e = 1; e <= sbr->data[ch].bs_num_noise; e++)
    for (k = 0; k < sbr->n_q; k++){
        // This should calculate the same as the aac float decoder:
        // sbr->data[ch].noise_facs[e][k] =
        //     exp2f(NOISE_FLOOR_OFFSET - sbr->data[ch].noise_facs[e][k]);
        sbr->data[ch].noise_facs[e][k].exp = NOISE_FLOOR_OFFSET - \
            sbr->data[ch].noise_facs[e][k].mant + 1;
        sbr->data[ch].noise_facs[e][k].mant = 0x20000000;
    }

Thus we get:
noise_facs[1][0].exp = 6 - 29 + 1 = -22;
noise_facs[1][0].mant = 0x20000000;
Together:
noise_facs[1][0] = {mant = 536870912, exp = -22}

So far so good. However, the next time sbr_dequant is called this breaks:
noise_facs[1][0].exp = 6 - 536870912 + 1 = -536870905;

This is obviously completely bogus.
Instead this code needs a function like av_exp2_sf.

Best regards,
Andreas