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 floative 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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
