| Message ID | MBdnfNj--F-2@lynne.ee |
|---|---|
| State | Accepted |
| Commit | 3e098cca6e51db0f19928c12d0348deaa17137b3 |
| Headers | show |
| Series | [FFmpeg-devel] yuv2rgb_neon: fix return value | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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
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 --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