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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is what and why we do it that matters, not just one of them.
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
