[FFmpeg-devel] [PATCH 1/6] avcodec/g723_1enc: Remove unnecessary av_clipl_int32()

[FFmpeg-devel] [PATCH 1/6] avcodec/g723_1enc: Remove unnecessary av_clipl_int32()

[Andreas Rheinhardt]

partial_corr is an int16_t and so the av_clipl_int32()
never clips and can be removed. This also avoids
undefined left-shifts of negative numbers.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/g723_1enc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libavcodec/g723_1enc.c b/libavcodec/g723_1enc.c
index f3baf7b4ec..a22985ca44 100644
--- a/libavcodec/g723_1enc.c
+++ b/libavcodec/g723_1enc.c
@@ -200,8 +200,7 @@ static void levinson_durbin(int16_t *lpc, int16_t *autocorr, int16_t error)
 
         partial_corr = temp / (error << 1);
 
-        lpc[i] = av_clipl_int32((int64_t) (partial_corr << 14) +
-                                (1 << 15)) >> 16;
+        lpc[i] = (partial_corr + (1 << 1)) >> 2;
 
         /* Update the prediction error */
         temp  = MULL2(temp, partial_corr);
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/6] avcodec/g723_1enc: Fix undefined left-shifts of negative numbers

[Andreas Rheinhardt]

Affected the acodec-g723_1 FATE-test.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/g723_1enc.c | 71 +++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/libavcodec/g723_1enc.c b/libavcodec/g723_1enc.c
index a22985ca44..8466067185 100644
--- a/libavcodec/g723_1enc.c
+++ b/libavcodec/g723_1enc.c
@@ -126,7 +126,7 @@ static void highpass_filter(int16_t *buf, int16_t *fir, int *iir)
 {
     int i;
     for (i = 0; i < FRAME_LEN; i++) {
-        *iir   = (buf[i] << 15) + ((-*fir) << 15) + MULL2(*iir, 0x7f00);
+        *iir   = (buf[i] - *fir) * (1 << 15) + MULL2(*iir, 0x7f00);
         *fir   = buf[i];
         buf[i] = av_clipl_int32((int64_t)*iir + (1 << 15)) >> 16;
     }
@@ -166,7 +166,7 @@ static void comp_autocorr(int16_t *buf, int16_t *autocorr)
     } else {
         for (i = 1; i <= LPC_ORDER; i++) {
             temp        = ff_dot_product(vector, vector + i, LPC_FRAME - i);
-            temp        = MULL2((temp << scale), binomial_window[i - 1]);
+            temp        = MULL2(temp * (1 << scale), binomial_window[i - 1]);
             autocorr[i] = av_clipl_int32((int64_t) temp + (1 << 15)) >> 16;
         }
     }
@@ -193,7 +193,7 @@ static void levinson_durbin(int16_t *lpc, int16_t *autocorr, int16_t error)
         temp = 0;
         for (j = 0; j < i; j++)
             temp -= lpc[j] * autocorr[i - j - 1];
-        temp = ((autocorr[i] << 13) + temp) << 3;
+        temp = (autocorr[i] * (1 << 13) + temp) * (1 << 3);
 
         if (FFABS(temp) >= (error << 16))
             break;
@@ -209,8 +209,8 @@ static void levinson_durbin(int16_t *lpc, int16_t *autocorr, int16_t error)
 
         memcpy(vector, lpc, i * sizeof(int16_t));
         for (j = 0; j < i; j++) {
-            temp   = partial_corr * vector[i - j - 1] << 1;
-            lpc[j] = av_clipl_int32((int64_t) (lpc[j] << 16) - temp +
+            temp   = partial_corr * vector[i - j - 1] * 2;
+            lpc[j] = av_clipl_int32((int64_t) (lpc[j] * (1 << 16)) - temp +
                                     (1 << 15)) >> 16;
         }
     }
@@ -259,9 +259,9 @@ static void lpc2lsp(int16_t *lpc, int16_t *prev_lsp, int16_t *lsp)
     /* Compute the remaining coefficients */
     for (i = 0; i < LPC_ORDER / 2; i++) {
         /* f1 */
-        f[2 * i + 2] = -f[2 * i] - ((lsp[i] + lsp[LPC_ORDER - 1 - i]) << 12);
+        f[2 * i + 2] = -f[2 * i]    - (lsp[i] + lsp[LPC_ORDER - 1 - i]) * (1 << 12);
         /* f2 */
-        f[2 * i + 3] = f[2 * i + 1] - ((lsp[i] - lsp[LPC_ORDER - 1 - i]) << 12);
+        f[2 * i + 3] = f[2 * i + 1] - (lsp[i] - lsp[LPC_ORDER - 1 - i]) * (1 << 12);
     }
 
     /* Divide f1[5] and f2[5] by 2 for use in polynomial evaluation */
@@ -276,7 +276,7 @@ static void lpc2lsp(int16_t *lpc, int16_t *prev_lsp, int16_t *lsp)
     shift = ff_g723_1_normalize_bits(max, 31);
 
     for (i = 0; i < LPC_ORDER + 2; i++)
-        f[i] = av_clipl_int32((int64_t) (f[i] << shift) + (1 << 15)) >> 16;
+        f[i] = av_clipl_int32((int64_t) (f[i] * (1 << shift)) + (1 << 15)) >> 16;
 
     /**
      * Evaluate F1 and F2 at uniform intervals of pi/256 along the
@@ -293,7 +293,7 @@ static void lpc2lsp(int16_t *lpc, int16_t *prev_lsp, int16_t *lsp)
         temp = 0;
         for (j = 0; j <= LPC_ORDER / 2; j++)
             temp += f[LPC_ORDER - 2 * j + p] * ff_g723_1_cos_tab[i * j % COS_TBL_SIZE];
-        cur_val = av_clipl_int32(temp << 1);
+        cur_val = av_clipl_int32(temp * 2);
 
         /* Check for sign change, indicating a zero crossing */
         if ((cur_val ^ prev_val) < 0) {
@@ -317,7 +317,7 @@ static void lpc2lsp(int16_t *lpc, int16_t *prev_lsp, int16_t *lsp)
             for (j = 0; j <= LPC_ORDER / 2; j++)
                 temp += f[LPC_ORDER - 2 * j + p] *
                         ff_g723_1_cos_tab[i * j % COS_TBL_SIZE];
-            cur_val = av_clipl_int32(temp << 1);
+            cur_val = av_clipl_int32(temp * 2);
         }
         prev_val = cur_val;
     }
@@ -344,7 +344,7 @@ static void lpc2lsp(int16_t *lpc, int16_t *prev_lsp, int16_t *lsp)
             temp[j] = (weight[j + (offset)] * ff_g723_1_lsp_band##num[i][j] + \
                       (1 << 14)) >> 15;                                       \
         }                                                                     \
-        error  = ff_g723_1_dot_product(lsp + (offset), temp, size) << 1;      \
+        error  = ff_g723_1_dot_product(lsp + (offset), temp, size) * 2;       \
         error -= ff_g723_1_dot_product(ff_g723_1_lsp_band##num[i], temp, size); \
         if (error > max) {                                                    \
             max = error;                                                      \
@@ -419,7 +419,7 @@ static void iir_filter(int16_t *fir_coef, int16_t *iir_coef,
                       iir_coef[n - 1] * dest[m - n];
         }
 
-        dest[m] = av_clipl_int32((src[m] << 16) + (filter << 3) +
+        dest[m] = av_clipl_int32(src[m] * (1 << 16) + filter * (1 << 3) +
                                  (1 << 15)) >> 16;
     }
 }
@@ -559,7 +559,7 @@ static void comp_harmonic_coeff(int16_t *buf, int16_t pitch_lag, HFParam *hf)
 
     exp = ff_g723_1_normalize_bits(max, 31);
     for (i = 0; i < 15; i++) {
-        energy[i] = av_clipl_int32((int64_t)(energy[i] << exp) +
+        energy[i] = av_clipl_int32((int64_t)(energy[i] * (1 << exp)) +
                                    (1 << 15)) >> 16;
     }
 
@@ -613,8 +613,8 @@ static void harmonic_filter(HFParam *hf, const int16_t *src, int16_t *dest)
     int i;
 
     for (i = 0; i < SUBFRAME_LEN; i++) {
-        int64_t temp = hf->gain * src[i - hf->index] << 1;
-        dest[i] = av_clipl_int32((src[i] << 16) - temp + (1 << 15)) >> 16;
+        int64_t temp = hf->gain * src[i - hf->index] * 2;
+        dest[i] = av_clipl_int32(src[i] * (1 << 16) - temp + (1 << 15)) >> 16;
     }
 }
 
@@ -622,8 +622,8 @@ static void harmonic_noise_sub(HFParam *hf, const int16_t *src, int16_t *dest)
 {
     int i;
     for (i = 0; i < SUBFRAME_LEN; i++) {
-        int64_t temp = hf->gain * src[i - hf->index] << 1;
-        dest[i] = av_clipl_int32(((dest[i] - src[i]) << 16) + temp +
+        int64_t temp = hf->gain * src[i - hf->index] * 2;
+        dest[i] = av_clipl_int32((dest[i] - src[i]) * (1 << 16) + temp +
                                  (1 << 15)) >> 16;
     }
 }
@@ -655,7 +655,7 @@ static void synth_percept_filter(int16_t *qnt_lpc, int16_t *perf_lpc,
         for (j = 1; j <= LPC_ORDER; j++)
             temp -= qnt_lpc[j - 1] * bptr_16[i - j];
 
-        buf[i]     = (src[i] << 15) + (temp << 3);
+        buf[i]     = src[i] * (1 << 15) + temp * (1 << 3);
         bptr_16[i] = av_clipl_int32(buf[i] + (1 << 15)) >> 16;
     }
 
@@ -665,7 +665,7 @@ static void synth_percept_filter(int16_t *qnt_lpc, int16_t *perf_lpc,
             fir -= perf_lpc[j - 1] * bptr_16[i - j];
             iir += perf_lpc[j + LPC_ORDER - 1] * dest[i - j];
         }
-        dest[i] = av_clipl_int32(((buf[i] + (fir << 3)) << scale) + (iir << 3) +
+        dest[i] = av_clipl_int32((buf[i] + fir * (1 << 3)) * (1 << scale) + iir * (1 << 3) +
                                  (1 << 15)) >> 16;
     }
     memcpy(perf_fir, buf_16 + SUBFRAME_LEN, sizeof(int16_t) * LPC_ORDER);
@@ -714,23 +714,22 @@ static void acb_search(G723_1_ChannelContext *p, int16_t *residual,
             temp = 0;
             for (k = 0; k <= j; k++)
                 temp += residual[PITCH_ORDER - 1 + k] * impulse_resp[j - k];
-            flt_buf[PITCH_ORDER - 1][j] = av_clipl_int32((temp << 1) +
-                                                         (1 << 15)) >> 16;
+            flt_buf[PITCH_ORDER - 1][j] = av_clipl_int32(temp * 2 + (1 << 15)) >> 16;
         }
 
         for (j = PITCH_ORDER - 2; j >= 0; j--) {
-            flt_buf[j][0] = ((residual[j] << 13) + (1 << 14)) >> 15;
+            flt_buf[j][0] = (residual[j] + (1 << 1)) >> 2;
             for (k = 1; k < SUBFRAME_LEN; k++) {
-                temp = (flt_buf[j + 1][k - 1] << 15) +
+                temp = flt_buf[j + 1][k - 1] * (1 << 15) +
                        residual[j] * impulse_resp[k];
-                flt_buf[j][k] = av_clipl_int32((temp << 1) + (1 << 15)) >> 16;
+                flt_buf[j][k] = av_clipl_int32(temp * 2 + (1 << 15)) >> 16;
             }
         }
 
         /* Compute crosscorrelation with the signal */
         for (j = 0; j < PITCH_ORDER; j++) {
             temp             = ff_dot_product(buf, flt_buf[j], SUBFRAME_LEN);
-            ccr_buf[count++] = av_clipl_int32(temp << 1);
+            ccr_buf[count++] = av_clipl_int32(temp * 2);
         }
 
         /* Compute energies */
@@ -742,7 +741,7 @@ static void acb_search(G723_1_ChannelContext *p, int16_t *residual,
         for (j = 1; j < PITCH_ORDER; j++) {
             for (k = 0; k < j; k++) {
                 temp             = ff_dot_product(flt_buf[j], flt_buf[k], SUBFRAME_LEN);
-                ccr_buf[count++] = av_clipl_int32(temp << 2);
+                ccr_buf[count++] = av_clipl_int32(temp * (1 << 2));
             }
         }
     }
@@ -755,7 +754,7 @@ static void acb_search(G723_1_ChannelContext *p, int16_t *residual,
     temp = ff_g723_1_normalize_bits(max, 31);
 
     for (i = 0; i < 20 * iter; i++)
-        ccr_buf[i] = av_clipl_int32((int64_t) (ccr_buf[i] << temp) +
+        ccr_buf[i] = av_clipl_int32((int64_t) (ccr_buf[i] * (1 << temp)) +
                                     (1 << 15)) >> 16;
 
     max = 0;
@@ -803,11 +802,11 @@ static void sub_acb_contrib(const int16_t *residual, const int16_t *impulse_resp
     int i, j;
     /* Subtract adaptive CB contribution to obtain the residual */
     for (i = 0; i < SUBFRAME_LEN; i++) {
-        int64_t temp = buf[i] << 14;
+        int64_t temp = buf[i] * (1 << 14);
         for (j = 0; j <= i; j++)
             temp -= residual[j] * impulse_resp[i - j];
 
-        buf[i] = av_clipl_int32((temp << 2) + (1 << 15)) >> 16;
+        buf[i] = av_clipl_int32(temp * (1 << 2) + (1 << 15)) >> 16;
     }
 }
 
@@ -851,7 +850,7 @@ static void get_fcb_param(FCBParam *optim, int16_t *impulse_resp,
     for (i = 1; i < SUBFRAME_LEN; i++) {
         temp = ff_g723_1_dot_product(temp_corr + i, temp_corr,
                                      SUBFRAME_LEN - i);
-        impulse_corr[i] = av_clipl_int32((temp << scale) + (1 << 15)) >> 16;
+        impulse_corr[i] = av_clipl_int32(temp * (1 << scale) + (1 << 15)) >> 16;
     }
 
     /* Compute crosscorrelation of impulse response with residual signal */
@@ -861,7 +860,7 @@ static void get_fcb_param(FCBParam *optim, int16_t *impulse_resp,
         if (scale < 0)
             ccr1[i] = temp >> -scale;
         else
-            ccr1[i] = av_clipl_int32(temp << scale);
+            ccr1[i] = av_clipl_int32(temp * (1 << scale));
     }
 
     /* Search loop */
@@ -910,7 +909,7 @@ static void get_fcb_param(FCBParam *optim, int16_t *impulse_resp,
                         continue;
                     temp = impulse_corr[FFABS(l - param.pulse_pos[k - 1])];
                     temp = av_clipl_int32((int64_t) temp *
-                                          param.pulse_sign[k - 1] << 1);
+                                          param.pulse_sign[k - 1] * 2);
                     ccr2[l] -= temp;
                     temp     = FFABS(ccr2[l]);
                     if (temp > max) {
@@ -934,17 +933,17 @@ static void get_fcb_param(FCBParam *optim, int16_t *impulse_resp,
                 temp = 0;
                 for (l = 0; l <= k; l++) {
                     int prod = av_clipl_int32((int64_t) temp_corr[l] *
-                                              impulse_r[k - l] << 1);
+                                              impulse_r[k - l] * 2);
                     temp = av_clipl_int32(temp + prod);
                 }
-                temp_corr[k] = temp << 2 >> 16;
+                temp_corr[k] = temp >> 14;
             }
 
             /* Compute square of error */
             err = 0;
             for (k = 0; k < SUBFRAME_LEN; k++) {
                 int64_t prod;
-                prod = av_clipl_int32((int64_t) buf[k] * temp_corr[k] << 1);
+                prod = av_clipl_int32((int64_t) buf[k] * temp_corr[k] * 2);
                 err  = av_clipl_int32(err - prod);
                 prod = av_clipl_int32((int64_t) temp_corr[k] * temp_corr[k]);
                 err  = av_clipl_int32(err + prod);
@@ -1204,7 +1203,7 @@ static int g723_1_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
         memmove(p->prev_excitation, p->prev_excitation + SUBFRAME_LEN,
                 sizeof(int16_t) * (PITCH_MAX - SUBFRAME_LEN));
         for (j = 0; j < SUBFRAME_LEN; j++)
-            in[j] = av_clip_int16((in[j] << 1) + impulse_resp[j]);
+            in[j] = av_clip_int16(in[j] * 2 + impulse_resp[j]);
         memcpy(p->prev_excitation + PITCH_MAX - SUBFRAME_LEN, in,
                sizeof(int16_t) * SUBFRAME_LEN);
 
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/6] avformat/aviobuf: Don't use NULL as src for memcpy

[Andreas Rheinhardt]

This might happen in avio_write() if size == 0
when the direct codepath is taken. It is undefined behaviour
according to the spec although it happens to work in practice.
Fixes the webm-webvtt-remux FATE-test under UBSan.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/aviobuf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index b20b1a611a..5b6a42d7f4 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -231,6 +231,8 @@ void ffio_fill(AVIOContext *s, int b, int64_t count)
 
 void avio_write(AVIOContext *s, const unsigned char *buf, int size)
 {
+    if (size <= 0)
+        return;
     if (s->direct && !s->update_checksum) {
         avio_flush(s);
         writeout(s, buf, size);
@@ -246,7 +248,7 @@ void avio_write(AVIOContext *s, const unsigned char *buf, int size)
 
         buf += len;
         size -= len;
-    }
+    } while (size > 0);
 }
 
 void avio_flush(AVIOContext *s)
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/6] avformat/aviobuf: Don't use NULL as src for memcpy

[James Almer]

On 9/28/2022 3:58 PM, Andreas Rheinhardt wrote:
> This might happen in avio_write() if size == 0
> when the direct codepath is taken. It is undefined behaviour
> according to the spec although it happens to work in practice.
> Fixes the webm-webvtt-remux FATE-test under UBSan.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/aviobuf.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index b20b1a611a..5b6a42d7f4 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -231,6 +231,8 @@ void ffio_fill(AVIOContext *s, int b, int64_t count)
>   
>   void avio_write(AVIOContext *s, const unsigned char *buf, int size)
>   {
> +    if (size <= 0)
> +        return;
>       if (s->direct && !s->update_checksum) {
>           avio_flush(s);
>           writeout(s, buf, size);
> @@ -246,7 +248,7 @@ void avio_write(AVIOContext *s, const unsigned char *buf, int size)
>   
>           buf += len;
>           size -= len;
> -    }
> +    } while (size > 0);

Why are you adding this at the end of a similar while statement? Did you 
mean to replace the previous one with do()?

>   }
>   
>   void avio_flush(AVIOContext *s)
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/6] avformat/aviobuf: Don't use NULL as src for memcpy

[Andreas Rheinhardt]

James Almer:
> On 9/28/2022 3:58 PM, Andreas Rheinhardt wrote:
>> This might happen in avio_write() if size == 0
>> when the direct codepath is taken. It is undefined behaviour
>> according to the spec although it happens to work in practice.
>> Fixes the webm-webvtt-remux FATE-test under UBSan.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>   libavformat/aviobuf.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>> index b20b1a611a..5b6a42d7f4 100644
>> --- a/libavformat/aviobuf.c
>> +++ b/libavformat/aviobuf.c
>> @@ -231,6 +231,8 @@ void ffio_fill(AVIOContext *s, int b, int64_t count)
>>     void avio_write(AVIOContext *s, const unsigned char *buf, int size)
>>   {
>> +    if (size <= 0)
>> +        return;
>>       if (s->direct && !s->update_checksum) {
>>           avio_flush(s);
>>           writeout(s, buf, size);
>> @@ -246,7 +248,7 @@ void avio_write(AVIOContext *s, const unsigned
>> char *buf, int size)
>>             buf += len;
>>           size -= len;
>> -    }
>> +    } while (size > 0);
> 
> Why are you adding this at the end of a similar while statement? Did you
> mean to replace the previous one with do()?
> 

Yes, exactly. Somehow I didn't. fate passed locally (apart from the
tdsc). Crazy that I forgot this. Thanks for spotting.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 4/6] avcodec/jrevdct: Fix UB left shifts of negative numbers

[Andreas Rheinhardt]

Affected the rv20-1239 FATE test.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/jrevdct.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/libavcodec/jrevdct.c b/libavcodec/jrevdct.c
index 36160cb663..7f1863515f 100644
--- a/libavcodec/jrevdct.c
+++ b/libavcodec/jrevdct.c
@@ -255,7 +255,7 @@ void ff_j_rev_dct(DCTBLOCK data)
       if (d0) {
           /* Compute a 32 bit value to assign. */
           int16_t dcval = (int16_t) (d0 * (1 << PASS1_BITS));
-          register int v = (dcval & 0xffff) | ((dcval * (1 << 16)) & 0xffff0000);
+          register unsigned v = (dcval & 0xffff) | ((uint32_t)dcval << 16);
 
           AV_WN32A(&idataptr[ 0], v);
           AV_WN32A(&idataptr[ 4], v);
@@ -988,8 +988,8 @@ void ff_j_rev_dct4(DCTBLOCK data)
       /* AC terms all zero */
       if (d0) {
           /* Compute a 32 bit value to assign. */
-          int16_t dcval = (int16_t) (d0 << PASS1_BITS);
-          register int v = (dcval & 0xffff) | ((dcval << 16) & 0xffff0000);
+          int16_t dcval = (int16_t) (d0 * (1 << PASS1_BITS));
+          register unsigned v = (dcval & 0xffff) | ((uint32_t)dcval << 16);
 
           AV_WN32A(&idataptr[0], v);
           AV_WN32A(&idataptr[4], v);
@@ -1008,8 +1008,8 @@ void ff_j_rev_dct4(DCTBLOCK data)
                     tmp2 = z1 + MULTIPLY(-d6, FIX_1_847759065);
                     tmp3 = z1 + MULTIPLY(d2, FIX_0_765366865);
 
-                    tmp0 = (d0 + d4) << CONST_BITS;
-                    tmp1 = (d0 - d4) << CONST_BITS;
+                    tmp0 = (d0 + d4) * (1 << CONST_BITS);
+                    tmp1 = (d0 - d4) * (1 << CONST_BITS);
 
                     tmp10 = tmp0 + tmp3;
                     tmp13 = tmp0 - tmp3;
@@ -1020,8 +1020,8 @@ void ff_j_rev_dct4(DCTBLOCK data)
                     tmp2 = MULTIPLY(-d6, FIX_1_306562965);
                     tmp3 = MULTIPLY(d6, FIX_0_541196100);
 
-                    tmp0 = (d0 + d4) << CONST_BITS;
-                    tmp1 = (d0 - d4) << CONST_BITS;
+                    tmp0 = (d0 + d4) * (1 << CONST_BITS);
+                    tmp1 = (d0 - d4) * (1 << CONST_BITS);
 
                     tmp10 = tmp0 + tmp3;
                     tmp13 = tmp0 - tmp3;
@@ -1034,8 +1034,8 @@ void ff_j_rev_dct4(DCTBLOCK data)
                     tmp2 = MULTIPLY(d2, FIX_0_541196100);
                     tmp3 = MULTIPLY(d2, FIX_1_306562965);
 
-                    tmp0 = (d0 + d4) << CONST_BITS;
-                    tmp1 = (d0 - d4) << CONST_BITS;
+                    tmp0 = (d0 + d4) * (1 << CONST_BITS);
+                    tmp1 = (d0 - d4) * (1 << CONST_BITS);
 
                     tmp10 = tmp0 + tmp3;
                     tmp13 = tmp0 - tmp3;
@@ -1043,8 +1043,8 @@ void ff_j_rev_dct4(DCTBLOCK data)
                     tmp12 = tmp1 - tmp2;
             } else {
                     /* d0 != 0, d2 == 0, d4 != 0, d6 == 0 */
-                    tmp10 = tmp13 = (d0 + d4) << CONST_BITS;
-                    tmp11 = tmp12 = (d0 - d4) << CONST_BITS;
+                    tmp10 = tmp13 = (d0 + d4) * (1 << CONST_BITS);
+                    tmp11 = tmp12 = (d0 - d4) * (1 << CONST_BITS);
             }
       }
 
@@ -1086,8 +1086,8 @@ void ff_j_rev_dct4(DCTBLOCK data)
                     tmp2 = z1 + MULTIPLY(-d6, FIX_1_847759065);
                     tmp3 = z1 + MULTIPLY(d2, FIX_0_765366865);
 
-                    tmp0 = (d0 + d4) << CONST_BITS;
-                    tmp1 = (d0 - d4) << CONST_BITS;
+                    tmp0 = (d0 + d4) * (1 << CONST_BITS);
+                    tmp1 = (d0 - d4) * (1 << CONST_BITS);
 
                     tmp10 = tmp0 + tmp3;
                     tmp13 = tmp0 - tmp3;
@@ -1098,8 +1098,8 @@ void ff_j_rev_dct4(DCTBLOCK data)
                     tmp2 = MULTIPLY(-d6, FIX_1_306562965);
                     tmp3 = MULTIPLY(d6, FIX_0_541196100);
 
-                    tmp0 = (d0 + d4) << CONST_BITS;
-                    tmp1 = (d0 - d4) << CONST_BITS;
+                    tmp0 = (d0 + d4) * (1 << CONST_BITS);
+                    tmp1 = (d0 - d4) * (1 << CONST_BITS);
 
                     tmp10 = tmp0 + tmp3;
                     tmp13 = tmp0 - tmp3;
@@ -1112,8 +1112,8 @@ void ff_j_rev_dct4(DCTBLOCK data)
                     tmp2 = MULTIPLY(d2, FIX_0_541196100);
                     tmp3 = MULTIPLY(d2, FIX_1_306562965);
 
-                    tmp0 = (d0 + d4) << CONST_BITS;
-                    tmp1 = (d0 - d4) << CONST_BITS;
+                    tmp0 = (d0 + d4) * (1 << CONST_BITS);
+                    tmp1 = (d0 - d4) * (1 << CONST_BITS);
 
                     tmp10 = tmp0 + tmp3;
                     tmp13 = tmp0 - tmp3;
@@ -1121,8 +1121,8 @@ void ff_j_rev_dct4(DCTBLOCK data)
                     tmp12 = tmp1 - tmp2;
             } else {
                     /* d0 != 0, d2 == 0, d4 != 0, d6 == 0 */
-                    tmp10 = tmp13 = (d0 + d4) << CONST_BITS;
-                    tmp11 = tmp12 = (d0 - d4) << CONST_BITS;
+                    tmp10 = tmp13 = (d0 + d4) * (1 << CONST_BITS);
+                    tmp11 = tmp12 = (d0 - d4) * (1 << CONST_BITS);
             }
     }
 
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6] avcodec/jrevdct: Fix UB left shifts of negative numbers

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 659 bytes --]

On Wed, Sep 28, 2022 at 08:58:16PM +0200, Andreas Rheinhardt wrote:
> Affected the rv20-1239 FATE test.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/jrevdct.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)

iam a bit surprised this was not spotted long ago but LGTM

thx

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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6] avcodec/jrevdct: Fix UB left shifts of negative numbers

[Andreas Rheinhardt]

Michael Niedermayer:
> On Wed, Sep 28, 2022 at 08:58:16PM +0200, Andreas Rheinhardt wrote:
>> Affected the rv20-1239 FATE test.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavcodec/jrevdct.c | 38 +++++++++++++++++++-------------------
>>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> iam a bit surprised this was not spotted long ago but LGTM
> 
> thx
> 

Most of these changes are in ff_j_rev_dct4 which is only used if lowres
== 1; I guess the fuzzer doesn't use lowres which explains why this has
not been found before.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 5/6] avcodec/mpegvideo: Fix undefined left shift of negative numbers

[Andreas Rheinhardt]

Fixes the rv20-1239 FATE-test.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/mpegvideo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
index d8c7bc687d..5095149eaa 100644
--- a/libavcodec/mpegvideo.c
+++ b/libavcodec/mpegvideo.c
@@ -843,7 +843,7 @@ static inline int hpel_motion_lowres(MpegEncContext *s,
         s->vdsp.emulated_edge_mc(s->sc.edge_emu_buffer, src,
                                  s->linesize, s->linesize,
                                  w + 1, (h + 1) << field_based,
-                                 src_x, src_y   << field_based,
+                                 src_x, src_y * (1 << field_based),
                                  h_edge_pos, v_edge_pos);
         src = s->sc.edge_emu_buffer;
         emu = 1;
@@ -945,7 +945,7 @@ static av_always_inline void mpeg_motion_lowres(MpegEncContext *s,
         s->vdsp.emulated_edge_mc(s->sc.edge_emu_buffer, ptr_y,
                                  linesize >> field_based, linesize >> field_based,
                                  17, 17 + field_based,
-                                src_x, src_y << field_based, h_edge_pos,
+                                src_x, src_y * (1 << field_based), h_edge_pos,
                                 v_edge_pos);
         ptr_y = s->sc.edge_emu_buffer;
         if (!CONFIG_GRAY || !(s->avctx->flags & AV_CODEC_FLAG_GRAY)) {
@@ -956,12 +956,12 @@ static av_always_inline void mpeg_motion_lowres(MpegEncContext *s,
             s->vdsp.emulated_edge_mc(ubuf,  ptr_cb,
                                      uvlinesize >> field_based, uvlinesize >> field_based,
                                      9, 9 + field_based,
-                                    uvsrc_x, uvsrc_y << field_based,
+                                    uvsrc_x, uvsrc_y * (1 << field_based),
                                     h_edge_pos >> 1, v_edge_pos >> 1);
             s->vdsp.emulated_edge_mc(vbuf,  ptr_cr,
                                      uvlinesize >> field_based,uvlinesize >> field_based,
                                      9, 9 + field_based,
-                                    uvsrc_x, uvsrc_y << field_based,
+                                    uvsrc_x, uvsrc_y * (1 << field_based),
                                     h_edge_pos >> 1, v_edge_pos >> 1);
             ptr_cb = ubuf;
             ptr_cr = vbuf;
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 5/6] avcodec/mpegvideo: Fix undefined left shift of negative numbers

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 745 bytes --]

On Wed, Sep 28, 2022 at 08:58:17PM +0200, Andreas Rheinhardt wrote:
> Fixes the rv20-1239 FATE-test.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/mpegvideo.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

LGTM

thx

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

Modern terrorism, a quick summary: Need oil, start war with country that
has oil, kill hundread thousand in war. Let country fall into chaos,
be surprised about raise of fundamantalists. Drop more bombs, kill more
people, be surprised about them taking revenge and drop even more bombs
and strip your own citizens of their rights and freedoms. to be continued

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 6/6] avcodec/mpegvideo_dec: Fix UB NULL + 0

[Andreas Rheinhardt]

Affected the mpeg2-field-enc FATE-test.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/mpegvideo_dec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavcodec/mpegvideo_dec.c b/libavcodec/mpegvideo_dec.c
index 5b37e79e36..6d1edc027a 100644
--- a/libavcodec/mpegvideo_dec.c
+++ b/libavcodec/mpegvideo_dec.c
@@ -446,8 +446,8 @@ int ff_mpv_frame_start(MpegEncContext *s, AVCodecContext *avctx)
     if (s->picture_structure != PICT_FRAME) {
         for (int i = 0; i < 4; i++) {
             if (s->picture_structure == PICT_BOTTOM_FIELD) {
-                s->current_picture.f->data[i] +=
-                    s->current_picture.f->linesize[i];
+                s->current_picture.f->data[i] = FF_PTR_ADD(s->current_picture.f->data[i],
+                                                           s->current_picture.f->linesize[i]);
             }
             s->current_picture.f->linesize[i] *= 2;
             s->last_picture.f->linesize[i]    *= 2;
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/6] avcodec/g723_1enc: Remove unnecessary av_clipl_int32()

[Andreas Rheinhardt]

Andreas Rheinhardt:
> partial_corr is an int16_t and so the av_clipl_int32()
> never clips and can be removed. This also avoids
> undefined left-shifts of negative numbers.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/g723_1enc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/libavcodec/g723_1enc.c b/libavcodec/g723_1enc.c
> index f3baf7b4ec..a22985ca44 100644
> --- a/libavcodec/g723_1enc.c
> +++ b/libavcodec/g723_1enc.c
> @@ -200,8 +200,7 @@ static void levinson_durbin(int16_t *lpc, int16_t *autocorr, int16_t error)
>  
>          partial_corr = temp / (error << 1);
>  
> -        lpc[i] = av_clipl_int32((int64_t) (partial_corr << 14) +
> -                                (1 << 15)) >> 16;
> +        lpc[i] = (partial_corr + (1 << 1)) >> 2;
>  
>          /* Update the prediction error */
>          temp  = MULL2(temp, partial_corr);

Will apply the remaining patches of this patchset tomorrow unless there
are objections.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".