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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel