| Message ID | tencent_4805C285B69868E200516A6E7E1EAD523A0A@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] swscale/aarch64: Fix rgb24toyv12 only works with aligned width | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Wed, 18 Sep 2024, Zhao Zhili wrote: > From: Zhao Zhili <zhilizhao@tencent.com> > > Since c0666d8b, rgb24toyv12 is broken for width non-aligned to 16. > Add a simple wrapper to handle the non-aligned part. > > Signed-off-by: Zhao Zhili <zhilizhao@tencent.com> > Co-authored-by: johzzy <hellojinqiang@gmail.com> > --- > libswscale/aarch64/rgb2rgb.c | 23 ++++++++++++++++++++++- > tests/checkasm/sw_rgb.c | 2 +- > 2 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/libswscale/aarch64/rgb2rgb.c b/libswscale/aarch64/rgb2rgb.c > index d978a6f173..20a25033cb 100644 > --- a/libswscale/aarch64/rgb2rgb.c > +++ b/libswscale/aarch64/rgb2rgb.c > @@ -27,9 +27,30 @@ > #include "libswscale/swscale.h" > #include "libswscale/swscale_internal.h" > > +// Only handle width aligned to 16 > void ff_rgb24toyv12_neon(const uint8_t *src, uint8_t *ydst, uint8_t *udst, > uint8_t *vdst, int width, int height, int lumStride, > int chromStride, int srcStride, int32_t *rgb2yuv); > + > +static void rgb24toyv12(const uint8_t *src, uint8_t *ydst, uint8_t *udst, > + uint8_t *vdst, int width, int height, int lumStride, > + int chromStride, int srcStride, int32_t *rgb2yuv) > +{ > + int width_align = width & (~15); > + > + if (width_align > 0) > + ff_rgb24toyv12_neon(src, ydst, udst, vdst, width_align, height, > + lumStride, chromStride, srcStride, rgb2yuv); > + if (width_align < width) { > + src += width_align * 3; > + ydst += width_align; > + udst += width_align / 2; > + vdst += width_align / 2; > + ff_rgb24toyv12_c(src, ydst, udst, vdst, width - width_align, height, > + lumStride, chromStride, srcStride, rgb2yuv); > + } > +} > + > void ff_interleave_bytes_neon(const uint8_t *src1, const uint8_t *src2, > uint8_t *dest, int width, int height, > int src1Stride, int src2Stride, int dstStride); > @@ -42,7 +63,7 @@ av_cold void rgb2rgb_init_aarch64(void) > int cpu_flags = av_get_cpu_flags(); > > if (have_neon(cpu_flags)) { > - ff_rgb24toyv12 = ff_rgb24toyv12_neon; > + ff_rgb24toyv12 = rgb24toyv12; > interleaveBytes = ff_interleave_bytes_neon; > deinterleaveBytes = ff_deinterleave_bytes_neon; > } > diff --git a/tests/checkasm/sw_rgb.c b/tests/checkasm/sw_rgb.c > index af9434073a..a57c471e3b 100644 > --- a/tests/checkasm/sw_rgb.c > +++ b/tests/checkasm/sw_rgb.c > @@ -129,7 +129,7 @@ static int cmp_off_by_n(const uint8_t *ref, const uint8_t *test, size_t n, int a > > static void check_rgb24toyv12(struct SwsContext *ctx) > { > - static const int input_sizes[] = {16, 128, 512, MAX_LINE_SIZE, -MAX_LINE_SIZE}; > + static const int input_sizes[] = {4, 16, 128, 512, MAX_LINE_SIZE, -MAX_LINE_SIZE}; > I think it would be good to test a case which isn't mod16, but bigger than 16 as well. Other than that, I think this change looks reasonable. // Martin
> On Sep 18, 2024, at 20:51, Martin Storsjö <martin@martin.st> wrote: > > On Wed, 18 Sep 2024, Zhao Zhili wrote: > >> From: Zhao Zhili <zhilizhao@tencent.com> >> >> Since c0666d8b, rgb24toyv12 is broken for width non-aligned to 16. >> Add a simple wrapper to handle the non-aligned part. >> >> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com> >> Co-authored-by: johzzy <hellojinqiang@gmail.com> >> --- >> libswscale/aarch64/rgb2rgb.c | 23 ++++++++++++++++++++++- >> tests/checkasm/sw_rgb.c | 2 +- >> 2 files changed, 23 insertions(+), 2 deletions(-) >> >> diff --git a/libswscale/aarch64/rgb2rgb.c b/libswscale/aarch64/rgb2rgb.c >> index d978a6f173..20a25033cb 100644 >> --- a/libswscale/aarch64/rgb2rgb.c >> +++ b/libswscale/aarch64/rgb2rgb.c >> @@ -27,9 +27,30 @@ >> #include "libswscale/swscale.h" >> #include "libswscale/swscale_internal.h" >> >> +// Only handle width aligned to 16 >> void ff_rgb24toyv12_neon(const uint8_t *src, uint8_t *ydst, uint8_t *udst, >> uint8_t *vdst, int width, int height, int lumStride, >> int chromStride, int srcStride, int32_t *rgb2yuv); >> + >> +static void rgb24toyv12(const uint8_t *src, uint8_t *ydst, uint8_t *udst, >> + uint8_t *vdst, int width, int height, int lumStride, >> + int chromStride, int srcStride, int32_t *rgb2yuv) >> +{ >> + int width_align = width & (~15); >> + >> + if (width_align > 0) >> + ff_rgb24toyv12_neon(src, ydst, udst, vdst, width_align, height, >> + lumStride, chromStride, srcStride, rgb2yuv); >> + if (width_align < width) { >> + src += width_align * 3; >> + ydst += width_align; >> + udst += width_align / 2; >> + vdst += width_align / 2; >> + ff_rgb24toyv12_c(src, ydst, udst, vdst, width - width_align, height, >> + lumStride, chromStride, srcStride, rgb2yuv); >> + } >> +} >> + >> void ff_interleave_bytes_neon(const uint8_t *src1, const uint8_t *src2, >> uint8_t *dest, int width, int height, >> int src1Stride, int src2Stride, int dstStride); >> @@ -42,7 +63,7 @@ av_cold void rgb2rgb_init_aarch64(void) >> int cpu_flags = av_get_cpu_flags(); >> >> if (have_neon(cpu_flags)) { >> - ff_rgb24toyv12 = ff_rgb24toyv12_neon; >> + ff_rgb24toyv12 = rgb24toyv12; >> interleaveBytes = ff_interleave_bytes_neon; >> deinterleaveBytes = ff_deinterleave_bytes_neon; >> } >> diff --git a/tests/checkasm/sw_rgb.c b/tests/checkasm/sw_rgb.c >> index af9434073a..a57c471e3b 100644 >> --- a/tests/checkasm/sw_rgb.c >> +++ b/tests/checkasm/sw_rgb.c >> @@ -129,7 +129,7 @@ static int cmp_off_by_n(const uint8_t *ref, const uint8_t *test, size_t n, int a >> >> static void check_rgb24toyv12(struct SwsContext *ctx) >> { >> - static const int input_sizes[] = {16, 128, 512, MAX_LINE_SIZE, -MAX_LINE_SIZE}; >> + static const int input_sizes[] = {4, 16, 128, 512, MAX_LINE_SIZE, -MAX_LINE_SIZE}; >> > > I think it would be good to test a case which isn't mod16, but bigger than 16 as well. Patch v2 changed width 512 to 540. https://ffmpeg.org/pipermail/ffmpeg-devel/2024-September/333636.html I have thought about handle the non-aligned width in asm. It’s cache friendly than current C wrapper. But current implementation is simple, and the lose of performance may be negligible. > > Other than that, I think this change looks reasonable. > > // Martin > > _______________________________________________ > 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".
diff --git a/libswscale/aarch64/rgb2rgb.c b/libswscale/aarch64/rgb2rgb.c index d978a6f173..20a25033cb 100644 --- a/libswscale/aarch64/rgb2rgb.c +++ b/libswscale/aarch64/rgb2rgb.c @@ -27,9 +27,30 @@ #include "libswscale/swscale.h" #include "libswscale/swscale_internal.h" +// Only handle width aligned to 16 void ff_rgb24toyv12_neon(const uint8_t *src, uint8_t *ydst, uint8_t *udst, uint8_t *vdst, int width, int height, int lumStride, int chromStride, int srcStride, int32_t *rgb2yuv); + +static void rgb24toyv12(const uint8_t *src, uint8_t *ydst, uint8_t *udst, + uint8_t *vdst, int width, int height, int lumStride, + int chromStride, int srcStride, int32_t *rgb2yuv) +{ + int width_align = width & (~15); + + if (width_align > 0) + ff_rgb24toyv12_neon(src, ydst, udst, vdst, width_align, height, + lumStride, chromStride, srcStride, rgb2yuv); + if (width_align < width) { + src += width_align * 3; + ydst += width_align; + udst += width_align / 2; + vdst += width_align / 2; + ff_rgb24toyv12_c(src, ydst, udst, vdst, width - width_align, height, + lumStride, chromStride, srcStride, rgb2yuv); + } +} + void ff_interleave_bytes_neon(const uint8_t *src1, const uint8_t *src2, uint8_t *dest, int width, int height, int src1Stride, int src2Stride, int dstStride); @@ -42,7 +63,7 @@ av_cold void rgb2rgb_init_aarch64(void) int cpu_flags = av_get_cpu_flags(); if (have_neon(cpu_flags)) { - ff_rgb24toyv12 = ff_rgb24toyv12_neon; + ff_rgb24toyv12 = rgb24toyv12; interleaveBytes = ff_interleave_bytes_neon; deinterleaveBytes = ff_deinterleave_bytes_neon; } diff --git a/tests/checkasm/sw_rgb.c b/tests/checkasm/sw_rgb.c index af9434073a..a57c471e3b 100644 --- a/tests/checkasm/sw_rgb.c +++ b/tests/checkasm/sw_rgb.c @@ -129,7 +129,7 @@ static int cmp_off_by_n(const uint8_t *ref, const uint8_t *test, size_t n, int a static void check_rgb24toyv12(struct SwsContext *ctx) { - static const int input_sizes[] = {16, 128, 512, MAX_LINE_SIZE, -MAX_LINE_SIZE}; + static const int input_sizes[] = {4, 16, 128, 512, MAX_LINE_SIZE, -MAX_LINE_SIZE}; LOCAL_ALIGNED_32(uint8_t, src, [BUFSIZE * 3]); LOCAL_ALIGNED_32(uint8_t, buf_y_0, [BUFSIZE]);