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

[Andreas Cadhalpun]

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.

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

>>> also does anyone known if values large enough to cause overflows
>>> are alowed in valid AAC ? (didnt investigate yet, just asking as
>>> someone might know ...)
>>
>> I don't know either, but it would be strange if that's invalid,
>> as e.g. the float aac decoder handles this just fine.
> 
> Are you sure that the computations that fill these arrays do not
> overflow ?

As I already mentioned, there are lots of other overflows happening
in this decoder, e.g. in autocorrelate, which I think is involved
in calculating these arrays.

> it just seems strange to me that it all works out to be exactly
> a hair too large at this point but fine in prior calculations

I haven't said prior calculations were fine...
But if they were done correctly, the input range would probably
be even larger.

> or in more abstract terms, this patch goes to great lengths to
> make the function work with any 32 bit input where before it worked
> with any 29bit value or whatever but theres no explanation about why
> values in the 30.32 range are valid while values in the >32bit range
> are not

I'm not sure if input values larger than the 32-bit range are invalid,
they just can't happen in the current code, because the input is a
32-bit integer.
Considering that the float aac decoder uses a float as input for
the corresponding function, I think it would be more correct to
use a SoftFloat in the aac_fixed decoder.
But that would probably be even slower.

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

Best regards,
Andreas