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

[Michael Niedermayer]

On Sun, Nov 08, 2015 at 09:26:21PM +0100, Andreas Cadhalpun wrote:
> On 08.11.2015 20:17, Michael Niedermayer wrote:
> > On Sun, Nov 08, 2015 at 05:14:10PM +0100, Andreas Cadhalpun wrote:
> >> If accu overflows, a negative value can be returned.
> >>
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >> ---
> >>  libavcodec/sbrdsp_fixed.c | 28 ++++++++++++++++++++++------
> >>  1 file changed, 22 insertions(+), 6 deletions(-)
> > 
> > CCing the authors of this, they are probably interrested in
> > commenting
> 
> Thanks.
> 
> > 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)

do we know the valid input range?


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

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

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


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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/57657ed4/attachment.sig>