[FFmpeg-devel] [PATCH] avcodec/apv_dsp: Fix left-shift of negative value

[FFmpeg-devel] [PATCH] avcodec/apv_dsp: Fix left-shift of negative value

[Andreas Rheinhardt]

[-- Attachment #1: Type: text/plain, Size: 27 bytes --]

Patch attached.

- Andreas

[-- Attachment #2: 0001-avcodec-apv_dsp-Fix-left-shift-of-negative-value.patch --]
[-- Type: text/x-patch, Size: 954 bytes --]

From a33080ab9e500432e816808fbdc42d15b47e7237 Mon Sep 17 00:00:00 2001
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Date: Wed, 30 Apr 2025 20:07:35 +0200
Subject: [PATCH] avcodec/apv_dsp: Fix left-shift of negative value

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

diff --git a/libavcodec/apv_dsp.c b/libavcodec/apv_dsp.c
index fd814ef900..07bb34ec0b 100644
--- a/libavcodec/apv_dsp.c
+++ b/libavcodec/apv_dsp.c
@@ -58,7 +58,7 @@ static void apv_decode_transquant_c(void *output,
 
         for (int y = 0; y < 8; y++) {
             for (int x = 0; x < 8; x++) {
-                int coeff = (((input[y][x] * qmatrix[y][x]) << qp_shift) +
+                int coeff = (input[y][x] * qmatrix[y][x] * (1 << qp_shift) +
                              (1 << (bd_shift - 1))) >> bd_shift;
 
                 scaled_coeff[y][x] =
-- 
2.45.2


[-- Attachment #3: 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] avcodec/apv_dsp: Fix left-shift of negative value

[Mark Thompson]

On 30/04/2025 19:26, Andreas Rheinhardt wrote:
> Patch attached.
> 
> - Andreas

Hmm, the spec does write exactly this but also defines << as arithmetic left shift of a two's complement representation so it works.

Seems fair, LGTM.

More generally this line is not robust to extreme values (can integer overflow 32 bits), but the cases which do this are not useful and won't be in real streams (if calculated at full precision they would always be far outside the possible coefficient range from a forward transform, so it takes a crafted encoder to make them).

Possibly a pre-clip to a calculated range which ensures it doesn't go too far outside the following clip range is the right answer to avoid the overflow?  I will think about this a bit more.

Thanks,

- Mark

_______________________________________________
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] avcodec/apv_dsp: Fix left-shift of negative value

[Andreas Rheinhardt]

Mark Thompson:
> On 30/04/2025 19:26, Andreas Rheinhardt wrote:
>> Patch attached.
>>
>> - Andreas
> 
> Hmm, the spec does write exactly this but also defines << as arithmetic left shift of a two's complement representation so it works.
> 
> Seems fair, LGTM.
> 
> More generally this line is not robust to extreme values (can integer overflow 32 bits), but the cases which do this are not useful and won't be in real streams (if calculated at full precision they would always be far outside the possible coefficient range from a forward transform, so it takes a crafted encoder to make them).
> 
> Possibly a pre-clip to a calculated range which ensures it doesn't go too far outside the following clip range is the right answer to avoid the overflow?  I will think about this a bit more.
> 

If it does not happen for real files, our usual answer is to use
unsigned types where overflow is defined.

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