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

[Nedeljko Babic]

>> 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!

This code was developed a while ago, but based on informations that I have
this part of code was analysed regarding possibility of overflow and conclusion
was that there is no valid way for causing overflow here.

And regarding valid range, if I remember correctly it should be 29, not 32.

Regards,
Nedeljko