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

[Andreas Cadhalpun]

On 11.11.2015 22:52, Michael Niedermayer wrote:
> 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, ...)

Considering that the aac float decoder can decode such samples, I tend
to think that the aac fixed decoder should be able to do that, too.

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

Well, in this case the intermediates can be much larger, because the
further calculation involves something like the square root of the inverse
of the value calculated by the function in question.
(And now we're back to the cause of this investigation: calculating
the square root of a negative value.)

> [...]
>>> 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 the FFmpeg aac encoder produces valid aac files then, yes, lots of
overflows can happen with valid aac files.
The decoder overflows even with a FATE sample.
Though it seems the FFmpeg aac encoder doesn't use sbr, so I don't know if
this particular overflow can happen with valid aac sbr 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

It seems that would be too simple...

> Comments fro AAC and SBR experts very welcome!

Indeed.

Best regards,
Andreas