| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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 --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; }
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(-)