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

2015-12-10 Thread Andreas Cadhalpun 
On 02.12.2015 20:58, Andreas Cadhalpun wrote:
> On 19.11.2015 01:01, Andreas Cadhalpun wrote:
>> Subject: [PATCH] sbrdsp_fixed: assert that input values for sbr_sum_square_c
>>  are valid
>>
>> Larger values can cause overflows, leading to this function returning a
>> negative value.
>>
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  libavcodec/sbrdsp_fixed.c | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/libavcodec/sbrdsp_fixed.c b/libavcodec/sbrdsp_fixed.c
>> index 5b7b7a6..f4e3de0 100644
>> --- a/libavcodec/sbrdsp_fixed.c
>> +++ b/libavcodec/sbrdsp_fixed.c
>> @@ -38,9 +38,14 @@ static SoftFloat sbr_sum_square_c(int (*x)[2], int n)
>>  int i, nz, round;
>>  
>>  for (i = 0; i < n; i += 2) {
>> +// Larger values are inavlid and could cause overflows of accu.
>> +av_assert2(FFABS(x[i + 0][0]) >> 29 == 0);
>>  accu += (int64_t)x[i + 0][0] * x[i + 0][0];
>> +av_assert2(FFABS(x[i + 0][1]) >> 29 == 0);
>>  accu += (int64_t)x[i + 0][1] * x[i + 0][1];
>> +av_assert2(FFABS(x[i + 1][0]) >> 29 == 0);
>>  accu += (int64_t)x[i + 1][0] * x[i + 1][0];
>> +av_assert2(FFABS(x[i + 1][1]) >> 29 == 0);
>>  accu += (int64_t)x[i + 1][1] * x[i + 1][1];
>>  }
>>  
> 
> Ping.

I pushed this now.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-12-02 Thread Andreas Cadhalpun 
On 19.11.2015 01:01, Andreas Cadhalpun wrote:
> Subject: [PATCH] sbrdsp_fixed: assert that input values for sbr_sum_square_c
>  are valid
> 
> Larger values can cause overflows, leading to this function returning a
> negative value.
> 
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/sbrdsp_fixed.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavcodec/sbrdsp_fixed.c b/libavcodec/sbrdsp_fixed.c
> index 5b7b7a6..f4e3de0 100644
> --- a/libavcodec/sbrdsp_fixed.c
> +++ b/libavcodec/sbrdsp_fixed.c
> @@ -38,9 +38,14 @@ static SoftFloat sbr_sum_square_c(int (*x)[2], int n)
>  int i, nz, round;
>  
>  for (i = 0; i < n; i += 2) {
> +// Larger values are inavlid and could cause overflows of accu.
> +av_assert2(FFABS(x[i + 0][0]) >> 29 == 0);
>  accu += (int64_t)x[i + 0][0] * x[i + 0][0];
> +av_assert2(FFABS(x[i + 0][1]) >> 29 == 0);
>  accu += (int64_t)x[i + 0][1] * x[i + 0][1];
> +av_assert2(FFABS(x[i + 1][0]) >> 29 == 0);
>  accu += (int64_t)x[i + 1][0] * x[i + 1][0];
> +av_assert2(FFABS(x[i + 1][1]) >> 29 == 0);
>  accu += (int64_t)x[i + 1][1] * x[i + 1][1];
>  }
>  

Ping.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-18 Thread Andreas Cadhalpun 
On 16.11.2015 02:59, Michael Niedermayer wrote:
> On Fri, Nov 13, 2015 at 10:32:47PM +0100, Andreas Cadhalpun wrote:
>> Because these samples aren't tested with the aac_fixed decoder:
>>  * aac/ct_faac-adts.aac is only used to test the aac demuxer.
>>  * aac/al07_96.mp4 is for some reason only tested with the aac decoder.
>>
>> There the overflow happens on lines like:
>> che->ch[0].ret[j] = 
>> (int32_t)av_clipl_int32((int64_t)che->ch[0].ret[j]<<7)+0x8000;
>>
> 
>> I guess the +0x8000 was meant to be inside av_clipl_int32.
> 
> could be
> also there was a different patch about a overflow in that
> 0729 11:58 Nedeljko Babic  (1.8K) [FFmpeg-devel] [PATCH] 
> avcodec/aacdec_fixed: Fix integer overflow
> that case was a bug elsewhere though
> 
> in that light, my first question, is, is the overflowing value used
> (aka affects the decoder output) ?

Simply removing the '+0x8000' doesn't change the framecrc output for these two 
samples,
so I guess the answer is no.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-18 Thread Andreas Cadhalpun 
On 16.11.2015 15:39, Nedeljko Babic wrote:
>>> On 11.11.2015 13:46, Michael Niedermayer wrote:
>> 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.

I would be very interested in details about this analysis of yours.
My investigation of this code leads me to believe that actually the potential
input range for sbr_sum_square is several orders of magnitude larger than what
fits into an int32_t, but since that type is used, lots of overflows are 
happening,
most during the imdct calculation.

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

Well, the input range should be 29-bits, because otherwise this function can
overflow.

So if you say that larger values are invalid, I suggest to assert that they
don't happen. See attached patch.

To prevent triggering these asserts, one can force the input to be small enough.
Doing that early enough also avoids overflows along the way.
I'll send a separate patch for that.

Best regards,
Andreas
>From 0555a4aa4a9398e7abc787760b67032fbb00e7e6 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun 
Date: Thu, 19 Nov 2015 00:37:52 +0100
Subject: [PATCH] sbrdsp_fixed: assert that input values for sbr_sum_square_c
 are valid

Larger values can cause overflows, leading to this function returning a
negative value.

Signed-off-by: Andreas Cadhalpun 
---
 libavcodec/sbrdsp_fixed.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/libavcodec/sbrdsp_fixed.c b/libavcodec/sbrdsp_fixed.c
index 5b7b7a6..f4e3de0 100644
--- a/libavcodec/sbrdsp_fixed.c
+++ b/libavcodec/sbrdsp_fixed.c
@@ -38,9 +38,14 @@ static SoftFloat sbr_sum_square_c(int (*x)[2], int n)
 int i, nz, round;
 
 for (i = 0; i < n; i += 2) {
+// Larger values are inavlid and could cause overflows of accu.
+av_assert2(FFABS(x[i + 0][0]) >> 29 == 0);
 accu += (int64_t)x[i + 0][0] * x[i + 0][0];
+av_assert2(FFABS(x[i + 0][1]) >> 29 == 0);
 accu += (int64_t)x[i + 0][1] * x[i + 0][1];
+av_assert2(FFABS(x[i + 1][0]) >> 29 == 0);
 accu += (int64_t)x[i + 1][0] * x[i + 1][0];
+av_assert2(FFABS(x[i + 1][1]) >> 29 == 0);
 accu += (int64_t)x[i + 1][1] * x[i + 1][1];
 }
 
-- 
2.6.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-18 Thread Ganesh Ajjanagadde 
On Wed, Nov 18, 2015 at 7:01 PM, Andreas Cadhalpun
 wrote:
> On 16.11.2015 15:39, Nedeljko Babic wrote:
 On 11.11.2015 13:46, Michael Niedermayer wrote:
>>> 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.
>
> I would be very interested in details about this analysis of yours.
> My investigation of this code leads me to believe that actually the potential
> input range for sbr_sum_square is several orders of magnitude larger than what
> fits into an int32_t, but since that type is used, lots of overflows are 
> happening,
> most during the imdct calculation.
>
>> And regarding valid range, if I remember correctly it should be 29, not 32.
>
> Well, the input range should be 29-bits, because otherwise this function can
> overflow.
>
> So if you say that larger values are invalid, I suggest to assert that they
> don't happen. See attached patch.
>
> To prevent triggering these asserts, one can force the input to be small 
> enough.
> Doing that early enough also avoids overflows along the way.
> I'll send a separate patch for that.

I have not analyzed any of this stuff, but just a general suggestion
to all here based on these aac related threads:
Please document any weird ranges like the 29 bits above in the code
either as asserts or as comments. It helps a lot in future analysis
for an unfamiliar reader in verifying lack of overflow, etc.
I mention this due to some back and forth I had regarding apedec:
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-October/180990.html.
There it is entirely possible that I was just dumb and could not see
the 24 bit thing easily, but here clearly in spite of a lot of thought
the analysis is hard.

>
> Best regards,
> Andreas
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-16 Thread 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-15 Thread Michael Niedermayer 
On Fri, Nov 13, 2015 at 10:32:47PM +0100, Andreas Cadhalpun wrote:
> On 13.11.2015 04:15, Michael Niedermayer wrote:
> > On Thu, Nov 12, 2015 at 08:43:42PM +0100, Andreas Cadhalpun wrote:
> >> 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.
> > 
> > IMO this reasoning is flawed
> > 
> > if you write a float mpeg2 decoder and feed it a fuzzed file and
> > that decoder skips all checks then you can get  >16bit coefficients
> > as input to the IDCT.
> > Changing the fixed point IDCTs to work with 32x32 bit mutiplies
> > would not fix anything it would just make everything much slower
> > 
> > either way the authors of the fixed point aac decoder should
> > decide on what needs to be done, they implemented it and know this
> > code much better and why it scales the values as they are scaled ...
> 
> Yes, it would be great if they could comment on this.
> 
> >> 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.
> > 
> > i think this is something the authors of the decoder should look
> > into 
> 
> OK.
> 
> > also if there are overflows with fate samples why does this not
> > show up on any test on fate.ffmpeg.org ?
> 
> Because these samples aren't tested with the aac_fixed decoder:
>  * aac/ct_faac-adts.aac is only used to test the aac demuxer.
>  * aac/al07_96.mp4 is for some reason only tested with the aac decoder.
> 
> There the overflow happens on lines like:
> che->ch[0].ret[j] = 
> (int32_t)av_clipl_int32((int64_t)che->ch[0].ret[j]<<7)+0x8000;
> 

> I guess the +0x8000 was meant to be inside av_clipl_int32.

could be
also there was a different patch about a overflow in that
0729 11:58 Nedeljko Babic  (1.8K) [FFmpeg-devel] [PATCH] avcodec/aacdec_fixed: 
Fix integer overflow
that case was a bug elsewhere though

in that light, my first question, is, is the overflowing value used
(aka affects the decoder output) ?

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

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-13 Thread Andreas Cadhalpun 
On 13.11.2015 04:15, Michael Niedermayer wrote:
> On Thu, Nov 12, 2015 at 08:43:42PM +0100, Andreas Cadhalpun wrote:
>> 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.
> 
> IMO this reasoning is flawed
> 
> if you write a float mpeg2 decoder and feed it a fuzzed file and
> that decoder skips all checks then you can get  >16bit coefficients
> as input to the IDCT.
> Changing the fixed point IDCTs to work with 32x32 bit mutiplies
> would not fix anything it would just make everything much slower
> 
> either way the authors of the fixed point aac decoder should
> decide on what needs to be done, they implemented it and know this
> code much better and why it scales the values as they are scaled ...

Yes, it would be great if they could comment on this.

>> 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.
> 
> i think this is something the authors of the decoder should look
> into 

OK.

> also if there are overflows with fate samples why does this not
> show up on any test on fate.ffmpeg.org ?

Because these samples aren't tested with the aac_fixed decoder:
 * aac/ct_faac-adts.aac is only used to test the aac demuxer.
 * aac/al07_96.mp4 is for some reason only tested with the aac decoder.

There the overflow happens on lines like:
che->ch[0].ret[j] = 
(int32_t)av_clipl_int32((int64_t)che->ch[0].ret[j]<<7)+0x8000;

I guess the +0x8000 was meant to be inside av_clipl_int32.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-12 Thread 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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-12 Thread Michael Niedermayer 
On Thu, Nov 12, 2015 at 08:43:42PM +0100, Andreas Cadhalpun wrote:
> 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.

IMO this reasoning is flawed

if you write a float mpeg2 decoder and feed it a fuzzed file and
that decoder skips all checks then you can get  >16bit coefficients
as input to the IDCT.
Changing the fixed point IDCTs to work with 32x32 bit mutiplies
would not fix anything it would just make everything much slower

either way the authors of the fixed point aac decoder should
decide on what needs to be done, they implemented it and know this
code much better and why it scales the values as they are scaled ...


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

i think this is something the authors of the decoder should look
into 
also if there are overflows with fate samples why does this not
show up on any test on fate.ffmpeg.org ?


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

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-11 Thread 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 
> >> ---
> >>  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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

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

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

2015-11-11 Thread Michael Niedermayer 
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, ...)


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

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

During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-08 Thread Andreas Cadhalpun 
If accu overflows, a negative value can be returned.

Signed-off-by: Andreas Cadhalpun 
---
 libavcodec/sbrdsp_fixed.c | 28 ++--
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/libavcodec/sbrdsp_fixed.c b/libavcodec/sbrdsp_fixed.c
index 5b7b7a6..7ab6cc6 100644
--- a/libavcodec/sbrdsp_fixed.c
+++ b/libavcodec/sbrdsp_fixed.c
@@ -35,13 +35,29 @@ static SoftFloat sbr_sum_square_c(int (*x)[2], int n)
 {
 SoftFloat ret;
 int64_t accu = 0;
-int i, nz, round;
+int i, nz, round, exp_offset = 0;
 
 for (i = 0; i < n; i += 2) {
-accu += (int64_t)x[i + 0][0] * x[i + 0][0];
-accu += (int64_t)x[i + 0][1] * x[i + 0][1];
-accu += (int64_t)x[i + 1][0] * x[i + 1][0];
-accu += (int64_t)x[i + 1][1] * x[i + 1][1];
+accu += ((int64_t)x[i + 0][0] * x[i + 0][0]) >> exp_offset;
+if (accu > 0x3FFF) {
+exp_offset += 1;
+accu >>= 1;
+}
+accu += ((int64_t)x[i + 0][1] * x[i + 0][1]) >> exp_offset;
+if (accu > 0x3FFF) {
+exp_offset += 1;
+accu >>= 1;
+}
+accu += ((int64_t)x[i + 1][0] * x[i + 1][0]) >> exp_offset;
+if (accu > 0x3FFF) {
+exp_offset += 1;
+accu >>= 1;
+}
+accu += ((int64_t)x[i + 1][1] * x[i + 1][1]) >> exp_offset;
+if (accu > 0x3FFF) {
+exp_offset += 1;
+accu >>= 1;
+}
 }
 
 i = (int)(accu >> 32);
@@ -59,7 +75,7 @@ static SoftFloat sbr_sum_square_c(int (*x)[2], int n)
 round = 1 << (nz-1);
 i = (int)((accu + round) >> nz);
 i >>= 1;
-ret = av_int2sf(i, 15 - nz);
+ret = av_int2sf(i, 15 - nz + exp_offset);
 
 return ret;
 }
-- 
2.6.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-08 Thread Andreas Cadhalpun 
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 
>> ---
>>  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.

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

> also an additional bit of precission can be use by making the
> variable unsigned

One bit is probably not sufficient, as dozens of arbitrary integers
are squared and added together.
Making the variable unsigned would effectively hide the overflows,
though.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-08 Thread Michael Niedermayer 
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 
> ---
>  libavcodec/sbrdsp_fixed.c | 28 ++--
>  1 file changed, 22 insertions(+), 6 deletions(-)

CCing the authors of this, they are probably interrested in
commenting

but the patch does not look like an optimal solution
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 ...)

also an additional bit of precission can be use by making the
variable unsigned


> 
> diff --git a/libavcodec/sbrdsp_fixed.c b/libavcodec/sbrdsp_fixed.c
> index 5b7b7a6..7ab6cc6 100644
> --- a/libavcodec/sbrdsp_fixed.c
> +++ b/libavcodec/sbrdsp_fixed.c
> @@ -35,13 +35,29 @@ static SoftFloat sbr_sum_square_c(int (*x)[2], int n)
>  {
>  SoftFloat ret;
>  int64_t accu = 0;
> -int i, nz, round;
> +int i, nz, round, exp_offset = 0;
>  
>  for (i = 0; i < n; i += 2) {
> -accu += (int64_t)x[i + 0][0] * x[i + 0][0];
> -accu += (int64_t)x[i + 0][1] * x[i + 0][1];
> -accu += (int64_t)x[i + 1][0] * x[i + 1][0];
> -accu += (int64_t)x[i + 1][1] * x[i + 1][1];
> +accu += ((int64_t)x[i + 0][0] * x[i + 0][0]) >> exp_offset;
> +if (accu > 0x3FFF) {
> +exp_offset += 1;
> +accu >>= 1;
> +}
> +accu += ((int64_t)x[i + 0][1] * x[i + 0][1]) >> exp_offset;
> +if (accu > 0x3FFF) {
> +exp_offset += 1;
> +accu >>= 1;
> +}
> +accu += ((int64_t)x[i + 1][0] * x[i + 1][0]) >> exp_offset;
> +if (accu > 0x3FFF) {
> +exp_offset += 1;
> +accu >>= 1;
> +}
> +accu += ((int64_t)x[i + 1][1] * x[i + 1][1]) >> exp_offset;
> +if (accu > 0x3FFF) {
> +exp_offset += 1;
> +accu >>= 1;
> +}
>  }
>  
>  i = (int)(accu >> 32);
> @@ -59,7 +75,7 @@ static SoftFloat sbr_sum_square_c(int (*x)[2], int n)
>  round = 1 << (nz-1);
>  i = (int)((accu + round) >> nz);
>  i >>= 1;
> -ret = av_int2sf(i, 15 - nz);
> +ret = av_int2sf(i, 15 - nz + exp_offset);
>  
>  return ret;
>  }
> -- 
> 2.6.2
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel