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

[Michael Niedermayer]

On Thu, Nov 19, 2015 at 01:31:19AM +0100, Michael Niedermayer wrote:
> On Thu, Nov 19, 2015 at 12:31:17AM +0100, Andreas Cadhalpun wrote:
> > 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.
> 
> yes
> 
> 
> > Instead this code needs a function like av_exp2_sf.
> 
> no, thats not the problem
> this code is missing error checks and only adding error checks will
> fix that
> 
> there is read_sbr_noise() which reads data
> there is sbr_dequant() which converts the data from "read data" to
> lets call it "dequantized data"
> 
> what you describe sounds like that sbr_dequant() is called on top of
> the output from sbr_dequant(), that sounds like a error condition
> for both fixed and float

ive split the pre dequant noise_fac tables from the post dequant
and also added some range checks for noise_fac
if you really had a case where sbr_dequant was run on top of
sbr_dequant, that should likely still be checked for and fixed

i dont know the valid range for env_facs currently so i cant write a
similar check for that atm

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/20151119/8aadff1f/attachment.sig>