diff mbox series

[FFmpeg-devel,1/2] avcodec/adxenc: Avoid undefined left shift of negative numbers

Message ID 20200120192043.4288-1-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/2] avcodec/adxenc: Avoid undefined left shift of negative numbers
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt Jan. 20, 2020, 7:20 p.m. UTC
Replace "((a << shift) + b) >> shift" by "a + (b >> shift)". This avoids
a left shift which also happens to trigger undefined behaviour in case "a"
is negative. This affected the FATE-tests acodec-adpcm-adx and
acodec-adpcm-adx-trellis; it also fixes ticket #8008.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
The equivalence of the old code and the new code in the first two
changes is clear from the type of wav[i] (int16_t) and the fact that
COEFF_BITS is 12 (i.e. no high bits are lost when left shifting); for
the last change the equivalence depends upon scale being representable
in an uint16_t. I don't know whether this is indeed so (the direct
estimate gives an upper bound of (2^19 + 2^15 + 1) / 7), but it better
should be so because scale is written via AV_WB16. Maybe it should be
clipped if it is outside of this range?

 libavcodec/adxenc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Michael Niedermayer Jan. 21, 2020, 8:59 a.m. UTC | #1
On Mon, Jan 20, 2020 at 08:20:42PM +0100, Andreas Rheinhardt wrote:
> Replace "((a << shift) + b) >> shift" by "a + (b >> shift)". This avoids
> a left shift which also happens to trigger undefined behaviour in case "a"
> is negative. This affected the FATE-tests acodec-adpcm-adx and
> acodec-adpcm-adx-trellis; it also fixes ticket #8008.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> The equivalence of the old code and the new code in the first two
> changes is clear from the type of wav[i] (int16_t) and the fact that
> COEFF_BITS is 12 (i.e. no high bits are lost when left shifting); for
> the last change the equivalence depends upon scale being representable
> in an uint16_t. I don't know whether this is indeed so (the direct
> estimate gives an upper bound of (2^19 + 2^15 + 1) / 7), but it better
> should be so because scale is written via AV_WB16. Maybe it should be
> clipped if it is outside of this range?
> 
>  libavcodec/adxenc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

will apply patchset

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/adxenc.c b/libavcodec/adxenc.c
index a781151b44..93b902b0e1 100644
--- a/libavcodec/adxenc.c
+++ b/libavcodec/adxenc.c
@@ -48,7 +48,7 @@  static void adx_encode(ADXContext *c, uint8_t *adx, const int16_t *wav,
     s2 = prev->s2;
     for (i = 0, j = 0; j < 32; i += channels, j++) {
         s0 = wav[i];
-        d = ((s0 << COEFF_BITS) - c->coeff[0] * s1 - c->coeff[1] * s2) >> COEFF_BITS;
+        d = s0 + ((-c->coeff[0] * s1 - c->coeff[1] * s2) >> COEFF_BITS);
         if (max < d)
             max = d;
         if (min > d)
@@ -79,13 +79,13 @@  static void adx_encode(ADXContext *c, uint8_t *adx, const int16_t *wav,
     s1 = prev->s1;
     s2 = prev->s2;
     for (i = 0, j = 0; j < 32; i += channels, j++) {
-        d = ((wav[i] << COEFF_BITS) - c->coeff[0] * s1 - c->coeff[1] * s2) >> COEFF_BITS;
+        d = wav[i] + ((-c->coeff[0] * s1 - c->coeff[1] * s2) >> COEFF_BITS);
 
         d = av_clip_intp2(ROUNDED_DIV(d, scale), 3);
 
         put_sbits(&pb, 4, d);
 
-        s0 = ((d << COEFF_BITS) * scale + c->coeff[0] * s1 + c->coeff[1] * s2) >> COEFF_BITS;
+        s0 = d * scale + ((c->coeff[0] * s1 + c->coeff[1] * s2) >> COEFF_BITS);
         s2 = s1;
         s1 = s0;
     }