[FFmpeg-devel] [PATCH 1/2] swscale/input: Use unsigned intermediates in rgb64ToUV_c_template

[FFmpeg-devel] [PATCH 1/2] swscale/input: Use unsigned intermediates in rgb64ToUV_c_template

[Michael Niedermayer]

From: Jeremy Dorfman <jdorfman@google.com>

Large rgb2yuv tables and high pixel values cause the intermediate
int32_t of ru*r + gu*g + bu*b to exceed INT_MAX, which is undefined
behavior. This causes libswscale built with LLVM -fsanitize=undefined to
assert. Using unsigned integers instead has defined behavior and
produces identical results, and makes rgb64ToUV_c_template match
rgb64ToY_c_template.

Fixes: signed integer overflow
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libswscale/input.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libswscale/input.c b/libswscale/input.c
index 7ff7bfaa01..d7dbedd82f 100644
--- a/libswscale/input.c
+++ b/libswscale/input.c
@@ -65,9 +65,9 @@ rgb64ToUV_c_template(uint16_t *dstU, uint16_t *dstV,
     int32_t rv = rgb2yuv[RV_IDX], gv = rgb2yuv[GV_IDX], bv = rgb2yuv[BV_IDX];
     av_assert1(src1==src2);
     for (i = 0; i < width; i++) {
-        int r_b = input_pixel(&src1[i*4+0]);
-        int   g = input_pixel(&src1[i*4+1]);
-        int b_r = input_pixel(&src1[i*4+2]);
+        unsigned int r_b = input_pixel(&src1[i*4+0]);
+        unsigned int   g = input_pixel(&src1[i*4+1]);
+        unsigned int b_r = input_pixel(&src1[i*4+2]);
 
         dstU[i] = (ru*r + gu*g + bu*b + (0x10001<<(RGB2YUV_SHIFT-1))) >> RGB2YUV_SHIFT;
         dstV[i] = (rv*r + gv*g + bv*b + (0x10001<<(RGB2YUV_SHIFT-1))) >> RGB2YUV_SHIFT;
-- 
2.17.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/2] swscale/input: Use more unsigned intermediates

[Michael Niedermayer]

Same principle as previous commit, with sufficiently huge rgb2yuv table
values this produces wrong results and undefined behavior.
The unsigned produces the same incorrect results. That is probably
ok as these cases with huge values seem not to occur in any real
use case.

There are more cases but someone is refactoring them, so i didnt yet change
them.

Fixes: signed integer overflow
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libswscale/input.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/libswscale/input.c b/libswscale/input.c
index d7dbedd82f..d5676062a2 100644
--- a/libswscale/input.c
+++ b/libswscale/input.c
@@ -84,9 +84,9 @@ rgb64ToUV_half_c_template(uint16_t *dstU, uint16_t *dstV,
     int32_t rv = rgb2yuv[RV_IDX], gv = rgb2yuv[GV_IDX], bv = rgb2yuv[BV_IDX];
     av_assert1(src1==src2);
     for (i = 0; i < width; i++) {
-        int r_b = (input_pixel(&src1[8 * i + 0]) + input_pixel(&src1[8 * i + 4]) + 1) >> 1;
-        int   g = (input_pixel(&src1[8 * i + 1]) + input_pixel(&src1[8 * i + 5]) + 1) >> 1;
-        int b_r = (input_pixel(&src1[8 * i + 2]) + input_pixel(&src1[8 * i + 6]) + 1) >> 1;
+        unsigned r_b = (input_pixel(&src1[8 * i + 0]) + input_pixel(&src1[8 * i + 4]) + 1) >> 1;
+        unsigned   g = (input_pixel(&src1[8 * i + 1]) + input_pixel(&src1[8 * i + 5]) + 1) >> 1;
+        unsigned b_r = (input_pixel(&src1[8 * i + 2]) + input_pixel(&src1[8 * i + 6]) + 1) >> 1;
 
         dstU[i]= (ru*r + gu*g + bu*b + (0x10001<<(RGB2YUV_SHIFT-1))) >> RGB2YUV_SHIFT;
         dstV[i]= (rv*r + gv*g + bv*b + (0x10001<<(RGB2YUV_SHIFT-1))) >> RGB2YUV_SHIFT;
@@ -158,9 +158,9 @@ static av_always_inline void rgb48ToUV_c_template(uint16_t *dstU,
     int32_t rv = rgb2yuv[RV_IDX], gv = rgb2yuv[GV_IDX], bv = rgb2yuv[BV_IDX];
     av_assert1(src1 == src2);
     for (i = 0; i < width; i++) {
-        int r_b = input_pixel(&src1[i * 3 + 0]);
-        int g   = input_pixel(&src1[i * 3 + 1]);
-        int b_r = input_pixel(&src1[i * 3 + 2]);
+        unsigned r_b = input_pixel(&src1[i * 3 + 0]);
+        unsigned g   = input_pixel(&src1[i * 3 + 1]);
+        unsigned b_r = input_pixel(&src1[i * 3 + 2]);
 
         dstU[i] = (ru*r + gu*g + bu*b + (0x10001 << (RGB2YUV_SHIFT - 1))) >> RGB2YUV_SHIFT;
         dstV[i] = (rv*r + gv*g + bv*b + (0x10001 << (RGB2YUV_SHIFT - 1))) >> RGB2YUV_SHIFT;
@@ -180,12 +180,12 @@ static av_always_inline void rgb48ToUV_half_c_template(uint16_t *dstU,
     int32_t rv = rgb2yuv[RV_IDX], gv = rgb2yuv[GV_IDX], bv = rgb2yuv[BV_IDX];
     av_assert1(src1 == src2);
     for (i = 0; i < width; i++) {
-        int r_b = (input_pixel(&src1[6 * i + 0]) +
-                   input_pixel(&src1[6 * i + 3]) + 1) >> 1;
-        int g   = (input_pixel(&src1[6 * i + 1]) +
-                   input_pixel(&src1[6 * i + 4]) + 1) >> 1;
-        int b_r = (input_pixel(&src1[6 * i + 2]) +
-                   input_pixel(&src1[6 * i + 5]) + 1) >> 1;
+        unsigned r_b = (input_pixel(&src1[6 * i + 0]) +
+                        input_pixel(&src1[6 * i + 3]) + 1) >> 1;
+        unsigned g   = (input_pixel(&src1[6 * i + 1]) +
+                        input_pixel(&src1[6 * i + 4]) + 1) >> 1;
+        unsigned b_r = (input_pixel(&src1[6 * i + 2]) +
+                        input_pixel(&src1[6 * i + 5]) + 1) >> 1;
 
         dstU[i] = (ru*r + gu*g + bu*b + (0x10001 << (RGB2YUV_SHIFT - 1))) >> RGB2YUV_SHIFT;
         dstV[i] = (rv*r + gv*g + bv*b + (0x10001 << (RGB2YUV_SHIFT - 1))) >> RGB2YUV_SHIFT;
-- 
2.17.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 2/2] swscale/input: Use more unsigned intermediates

[Michael Niedermayer]

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

On Tue, Nov 15, 2022 at 11:16:57PM +0100, Michael Niedermayer wrote:
> Same principle as previous commit, with sufficiently huge rgb2yuv table
> values this produces wrong results and undefined behavior.
> The unsigned produces the same incorrect results. That is probably
> ok as these cases with huge values seem not to occur in any real
> use case.
> 
> There are more cases but someone is refactoring them, so i didnt yet change
> them.
> 
> Fixes: signed integer overflow
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libswscale/input.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

I can merge these 2 if people prefer

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch

[-- 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 2/2] swscale/input: Use more unsigned intermediates

[Michael Niedermayer]

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

On Tue, Nov 15, 2022 at 11:16:57PM +0100, Michael Niedermayer wrote:
> Same principle as previous commit, with sufficiently huge rgb2yuv table
> values this produces wrong results and undefined behavior.
> The unsigned produces the same incorrect results. That is probably
> ok as these cases with huge values seem not to occur in any real
> use case.
> 
> There are more cases but someone is refactoring them, so i didnt yet change
> them.
> 
> Fixes: signed integer overflow
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libswscale/input.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

will apply patch set

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

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.

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