diff mbox series

[FFmpeg-devel] yuv2rgb_neon: fix return value

Message ID MBdnfNj--F-2@lynne.ee
State Accepted
Commit 3e098cca6e51db0f19928c12d0348deaa17137b3
Headers show
Series [FFmpeg-devel] yuv2rgb_neon: fix return value
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Lynne July 7, 2020, 3:04 p.m. UTC
We return 0 for this particular architecture but should instead be 
returning the number of lines.
Fixes users who check the return value matches what they expect.
Subject: [PATCH] yuv2rgb_neon: fix return value

We return 0 for this particular architecture but should instead be
returning the number of lines.
Fixes users who check the return value matches what they expect.
---
 libswscale/aarch64/swscale_unscaled.c | 31 ++++++++++++---------------
 libswscale/aarch64/yuv2rgb_neon.S     |  2 ++
 2 files changed, 16 insertions(+), 17 deletions(-)

Comments

Martin Storsjö July 7, 2020, 7:58 p.m. UTC | #1
On Tue, 7 Jul 2020, Lynne wrote:

> We return 0 for this particular architecture but should instead be
> returning the number of lines.
> Fixes users who check the return value matches what they expect.

The change looks good in itself, but it also looks like we have the same 
issue in the arm version of the same assembly, right?

I presume we don't have a preexisting checkasm test for this function, 
where we could add a check for the return value (which would expose all 
other instances of the same issue)?

// Martin
Lynne July 7, 2020, 9:22 p.m. UTC | #2
Jul 7, 2020, 20:58 by martin@martin.st:

> On Tue, 7 Jul 2020, Lynne wrote:
>
>> We return 0 for this particular architecture but should instead be
>> returning the number of lines.
>> Fixes users who check the return value matches what they expect.
>>
>
> The change looks good in itself, but it also looks like we have the same issue in the arm version of the same assembly, right?
>
> I presume we don't have a preexisting checkasm test for this function, where we could add a check for the return value (which would expose all other instances of the same issue)?
>

I've checked (there's only PPC) and the only other place we return 0 is in the 32bit ARM asm.
I've never done 32bit ARM asm and it looks more involved since the asm functions return a void,
and I've no idea how that arch handles return values.

Submitted a patch to check sws_scale return value in its API tests. As the scale functions are
directly called, I don't think there's a need to run checkasm on this.

Since patch looks good to you I'll push it tomorrow unless someone else has any comments.
diff mbox series

Patch

diff --git a/libswscale/aarch64/swscale_unscaled.c b/libswscale/aarch64/swscale_unscaled.c
index 551daad9e3..c7a2a1037d 100644
--- a/libswscale/aarch64/swscale_unscaled.c
+++ b/libswscale/aarch64/swscale_unscaled.c
@@ -42,15 +42,14 @@  static int ifmt##_to_##ofmt##_neon_wrapper(SwsContext *c, const uint8_t *src[],
                                            uint8_t *dst[], int dstStride[]) {               \
     const int16_t yuv2rgb_table[] = { YUV_TO_RGB_TABLE };                                   \
                                                                                             \
-    ff_##ifmt##_to_##ofmt##_neon(c->srcW, srcSliceH,                                        \
-                                 dst[0] + srcSliceY * dstStride[0], dstStride[0],           \
-                                 src[0], srcStride[0],                                      \
-                                 src[1], srcStride[1],                                      \
-                                 src[2], srcStride[2],                                      \
-                                 yuv2rgb_table,                                             \
-                                 c->yuv2rgb_y_offset >> 6,                                  \
-                                 c->yuv2rgb_y_coeff);                                       \
-    return 0;                                                                               \
+    return ff_##ifmt##_to_##ofmt##_neon(c->srcW, srcSliceH,                                 \
+                                        dst[0] + srcSliceY * dstStride[0], dstStride[0],    \
+                                        src[0], srcStride[0],                               \
+                                        src[1], srcStride[1],                               \
+                                        src[2], srcStride[2],                               \
+                                        yuv2rgb_table,                                      \
+                                        c->yuv2rgb_y_offset >> 6,                           \
+                                        c->yuv2rgb_y_coeff);                                \
 }                                                                                           \
 
 #define DECLARE_FF_YUVX_TO_ALL_RGBX_FUNCS(yuvx)                                             \
@@ -76,14 +75,12 @@  static int ifmt##_to_##ofmt##_neon_wrapper(SwsContext *c, const uint8_t *src[],
                                            uint8_t *dst[], int dstStride[]) {               \
     const int16_t yuv2rgb_table[] = { YUV_TO_RGB_TABLE };                                   \
                                                                                             \
-    ff_##ifmt##_to_##ofmt##_neon(c->srcW, srcSliceH,                                        \
-                                 dst[0] + srcSliceY * dstStride[0], dstStride[0],           \
-                                 src[0], srcStride[0], src[1], srcStride[1],                \
-                                 yuv2rgb_table,                                             \
-                                 c->yuv2rgb_y_offset >> 6,                                  \
-                                 c->yuv2rgb_y_coeff);                                       \
-                                                                                            \
-    return 0;                                                                               \
+    return ff_##ifmt##_to_##ofmt##_neon(c->srcW, srcSliceH,                                 \
+                                        dst[0] + srcSliceY * dstStride[0], dstStride[0],    \
+                                        src[0], srcStride[0], src[1], srcStride[1],         \
+                                        yuv2rgb_table,                                      \
+                                        c->yuv2rgb_y_offset >> 6,                           \
+                                        c->yuv2rgb_y_coeff);                                \
 }                                                                                           \
 
 #define DECLARE_FF_NVX_TO_ALL_RGBX_FUNCS(nvx)                                               \
diff --git a/libswscale/aarch64/yuv2rgb_neon.S b/libswscale/aarch64/yuv2rgb_neon.S
index b7446aa105..f4b220fb60 100644
--- a/libswscale/aarch64/yuv2rgb_neon.S
+++ b/libswscale/aarch64/yuv2rgb_neon.S
@@ -142,6 +142,7 @@ 
 .macro declare_func ifmt ofmt
 function ff_\ifmt\()_to_\ofmt\()_neon, export=1
     load_args_\ifmt
+    mov                 w9, w1
 1:
     mov                 w8, w0                                          // w8 = width
 2:
@@ -193,6 +194,7 @@  function ff_\ifmt\()_to_\ofmt\()_neon, export=1
     increment_\ifmt
     subs                w1, w1, #1                                      // height -= 1
     b.gt                1b
+    mov                 w0, w9
     ret
 endfunc
 .endm