[FFmpeg-devel] [PATCH 1/3] lavc/aarch64: Add neon implementation for pix_median_abs16
Martin Storsjö
martin at martin.st
Sat Sep 17 00:08:55 EEST 2022
On Tue, 13 Sep 2022, Hubert Mazur wrote:
> Provide optimized implementation for pix_median_abs16 function.
>
> Performance comparison tests are shown below.
> - median_sad_0_c: 722.0
> - median_sad_0_neon: 144.7
>
> Benchmarks and tests run with checkasm tool on AWS Graviton 3.
>
> Signed-off-by: Hubert Mazur <hum at semihalf.com>
> ---
> libavcodec/aarch64/me_cmp_init_aarch64.c | 4 ++
> libavcodec/aarch64/me_cmp_neon.S | 81 ++++++++++++++++++++++++
> libavcodec/me_cmp.c | 5 +-
> 3 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/aarch64/me_cmp_init_aarch64.c b/libavcodec/aarch64/me_cmp_init_aarch64.c
> index ade3e9a4c1..fb51a833be 100644
> --- a/libavcodec/aarch64/me_cmp_init_aarch64.c
> +++ b/libavcodec/aarch64/me_cmp_init_aarch64.c
> @@ -53,6 +53,8 @@ int nsse16_neon(int multiplier, const uint8_t *s, const uint8_t *s2,
> ptrdiff_t stride, int h);
> int nsse16_neon_wrapper(MpegEncContext *c, const uint8_t *s1, const uint8_t *s2,
> ptrdiff_t stride, int h);
> +int pix_median_abs16_neon(MpegEncContext *v, const uint8_t *pix1, const uint8_t *pix2,
> + ptrdiff_t stride, int h);
>
> av_cold void ff_me_cmp_init_aarch64(MECmpContext *c, AVCodecContext *avctx)
> {
> @@ -78,6 +80,8 @@ av_cold void ff_me_cmp_init_aarch64(MECmpContext *c, AVCodecContext *avctx)
> c->vsse[4] = vsse_intra16_neon;
>
> c->nsse[0] = nsse16_neon_wrapper;
> +
> + c->median_sad[0] = pix_median_abs16_neon;
> }
> }
>
> diff --git a/libavcodec/aarch64/me_cmp_neon.S b/libavcodec/aarch64/me_cmp_neon.S
> index f8998749a5..a4a4344f42 100644
> --- a/libavcodec/aarch64/me_cmp_neon.S
> +++ b/libavcodec/aarch64/me_cmp_neon.S
> @@ -969,3 +969,84 @@ function nsse16_neon, export=1
>
> ret
> endfunc
> +
> +function pix_median_abs16_neon, export=1
> + // x0 unused
> + // x1 uint8_t *pix1
> + // x2 uint8_t *pix2
> + // x3 ptrdiff_t stride
> + // w4 int h
> +
> + ld1 {v2.16b}, [x1], x3
> + ld1 {v3.16b}, [x2], x3
> + movi v31.8h, #0
> + movi v16.8h, #0
> + ext v0.16b, v2.16b, v2.16b, #1
> + ext v1.16b, v3.16b, v3.16b, #1
> + usubl v28.8h, v2.8b, v3.8b
> + usubl2 v27.8h, v2.16b, v3.16b
> + usubl v26.8h, v0.8b, v1.8b
> + usubl2 v25.8h, v0.16b, v1.16b
> + sub w4, w4, #1 // we need to make h-1 iterations
> + saba v31.8h, v26.8h, v28.8h
> + saba v16.8h, v25.8h, v27.8h
> + mov h18, v28.h[0]
> + cmp w4, #1
> + sqabs h18, h18
> +
> + b.lt 2f
> +1:
> +
> + ld1 {v6.16b}, [x1], x3 // pix1 vector for V(j-1)
> + ld1 {v7.16b}, [x2], x3 // pix2 vector for V(j-1)
> + subs w4, w4, #1
> + mov v2.16b, v6.16b
> + mov v3.16b, v7.16b
These two mov instructions seem unnecessary?
> + ext v4.16b, v6.16b, v6.16b, #1 // pix1 vector for V(j)
> + ext v5.16b, v7.16b, v7.16b, #1 // pix2 vector for V(j)
> +
> + // protected registers: v30, v29, v28, v27, v26, v25, v24, v23
> + // scratch registers: v22, v21, v20, v19, v17
> +
> + // To find median of three values, calculate sum of them
> + // and subtract max and min value from it.
> + usubl v30.8h, v6.8b, v7.8b // V(j-1)
> + usubl2 v29.8h, v6.16b, v7.16b // V(j-1)
> + usubl v24.8h, v4.8b, v5.8b // V(j)
> + usubl2 v23.8h, v4.16b, v5.16b // V(j)
> + mov v0.16b, v4.16b
> + mov v1.16b, v5.16b
These two movs are unused, too, right?
> + sabd v20.8h, v30.8h, v28.8h
> + mov h17, v20.h[0]
> + add d18, d18, d17
These are quite suboptimally scheduled here. However, we shouldn't need
them.
In general, try to avoid these single-element calculations if not strictly
necessary. You can just keep using both the input (here, v20) and the
accumulator (v18) as a .4h vector, where you only care about the first
element. Then at the very end you can extract the individual first
element from it, instead of doing it every round in the loop. Then you can
potentially change sabd into saba too, unless the non-accumulated result
is needed too.
> + add v22.8h, v26.8h, v30.8h
> + smin v20.8h, v26.8h, v30.8h
> + add v21.8h, v25.8h, v29.8h
> + smax v19.8h, v26.8h, v30.8h
> + sub v22.8h, v22.8h, v28.8h
> + sub v21.8h, v21.8h, v27.8h
> + smin v17.8h, v19.8h, v22.8h
> + smin v22.8h, v25.8h, v29.8h
> + mov v28.16b, v30.16b
> + smax v20.8h, v20.8h, v17.8h // median values lower half
> + smax v19.8h, v25.8h, v29.8h
> + saba v31.8h, v24.8h, v20.8h
> + mov v27.16b, v29.16b
> + smin v19.8h, v19.8h, v21.8h
> + mov v26.16b, v24.16b
> + smax v17.8h, v22.8h, v19.8h // median values upper half
> + mov v25.16b, v23.16b
> + saba v16.8h, v23.8h, v17.8h
> +
> + b.ne 1b
> +
> +2:
> + ins v16.h[7], wzr
> + add v31.8h, v31.8h, v16.8h
> + uaddlv s17, v31.8h
> + add d18, d18, d17
> + fmov w0, s18
> +
> + ret
> +
> +endfunc
> diff --git a/libavcodec/me_cmp.c b/libavcodec/me_cmp.c
> index 4242fbc6e4..230e7ea54a 100644
> --- a/libavcodec/me_cmp.c
> +++ b/libavcodec/me_cmp.c
> @@ -1048,6 +1048,9 @@ av_cold void ff_me_cmp_init(MECmpContext *c, AVCodecContext *avctx)
> ff_dsputil_init_dwt(c);
> #endif
>
> +c->median_sad[0] = pix_median_abs16_c;
> +c->median_sad[1] = pix_median_abs8_c;
> +
These are incorrectly indented.
Other than that, this seems reasonable I think.
// Martin
More information about the ffmpeg-devel
mailing list