[FFmpeg-devel] [PATCH] aac_fixed: fix overflow in sbr_sum_square_c

[Michael Niedermayer]

On Wed, Nov 11, 2015 at 09:09:51PM +0100, Andreas Cadhalpun wrote:
> On 11.11.2015 13:46, Michael Niedermayer wrote:
> > On Sun, Nov 08, 2015 at 09:26:21PM +0100, Andreas Cadhalpun wrote:
> >> On 08.11.2015 20:17, Michael Niedermayer wrote:
> >>> but the patch does not look like an optimal solution
> >>
> >> It's certainly not pretty, but it fixes the crashes/assertion failures.
> > 
> > at the expense of making the code rather slow and hard to implement in
> > SIMD.
> > This would be perfectly ok if that is neccessary but is it ?
> > (i dont know)
> 
> That depends how you define necessary. It's possible to avoid the problem
> with simpler, faster means, but those are less correct mathematically.

I define neccessary as what is neccessary for a well working fixed
point aac decoder.

There are 2 possible cases i think
A. The large input values are invalid then the correct solution is
   to detect that and error out / do error concealment
B. The large values are valid, in this case the implementation is
   broken. And the question would arrise how to best support the valid
   range, can the values or their products be scaled down by a fixed
   shift? or does this affect quality, do we need a variable shift
   if so how to implement this best (like 2pass, find max, choose
   shift at a earlier stage possibly, some kind of floats, ...)


> 
> > do we know the valid input range?
> 
> I don't know about valid, but the possible input range currently seems
> to be any 32-bit integer.

yes, and neither do i know the valid range but
SBR builds the high frequency signal and adds that to the base AAC
low frequency signal. These cannot be arbitrary large as the output
cannot be arbitrary large so there is a practical limit on how large
these values can be unless its possible and allowed to have much
larger intermediates at some point.


[...]
> > or maybe "valid" is not the correct word (in case the spec does not
> > specify that directly or indirectly ...)
> > if thats the case then it could be a question if any encoder could
> > ever have a reason to use values in a specific range
> 
> Anyway, the decoder has to deal with such values somehow.
> It could also error out, but I don't know which error check to use.
> And it would be a bit strange if the aac_fixed decoder errors out,
> while the float aac decoder can handle such samples.

can you check if there are any overflows happening in valid aac files
?
if none of them overflows anywhere then i would guess the supported
ranges are not that far off from what is used by actual encoders
and just erroring out with a request for a sample might be an easy
solution and much faster than converting all to some float emulation

Comments fro AAC and SBR experts very welcome!

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell
-------------- 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/20151111/21044a4c/attachment.sig>