[FFmpeg-devel] [PATCH 3/3] lavc/aarch64: add hevc sao edge 8x8

[Martin Storsjö]

On Thu, 28 Apr 2022, J. Dekker wrote:

> bench on AWS Graviton:
>
> hevc_sao_edge_8x8_8_c: 516.0
> hevc_sao_edge_8x8_8_neon: 81.0
>
> Signed-off-by: J. Dekker <jdek at itanimul.li>
> ---
> libavcodec/aarch64/hevcdsp_init_aarch64.c |  3 ++
> libavcodec/aarch64/hevcdsp_sao_neon.S     | 51 +++++++++++++++++++++++
> 2 files changed, 54 insertions(+)
>
> diff --git a/libavcodec/aarch64/hevcdsp_init_aarch64.c b/libavcodec/aarch64/hevcdsp_init_aarch64.c
> index df521bb083..2002530266 100644
> --- a/libavcodec/aarch64/hevcdsp_init_aarch64.c
> +++ b/libavcodec/aarch64/hevcdsp_init_aarch64.c
> @@ -59,6 +59,8 @@ void ff_hevc_sao_band_filter_8x8_8_neon(uint8_t *_dst, uint8_t *_src,
>                                   int width, int height);
> void ff_hevc_sao_edge_filter_16x16_8_neon(uint8_t *dst, uint8_t *src, ptrdiff_t stride_dst,
>                                           int16_t *sao_offset_val, int eo, int width, int height);
> +void ff_hevc_sao_edge_filter_8x8_8_neon(uint8_t *dst, uint8_t *src, ptrdiff_t stride_dst,
> +                                          int16_t *sao_offset_val, int eo, int width, int height);
>
> av_cold void ff_hevc_dsp_init_aarch64(HEVCDSPContext *c, const int bit_depth)
> {
> @@ -80,6 +82,7 @@ av_cold void ff_hevc_dsp_init_aarch64(HEVCDSPContext *c, const int bit_depth)
>         c->sao_band_filter[2]          =
>         c->sao_band_filter[3]          =
>         c->sao_band_filter[4]          = ff_hevc_sao_band_filter_8x8_8_neon;
> +        c->sao_edge_filter[0]          = ff_hevc_sao_edge_filter_8x8_8_neon;
>         c->sao_edge_filter[1]          =
>         c->sao_edge_filter[2]          =
>         c->sao_edge_filter[3]          =
> diff --git a/libavcodec/aarch64/hevcdsp_sao_neon.S b/libavcodec/aarch64/hevcdsp_sao_neon.S
> index 0315c479df..efd8112af4 100644
> --- a/libavcodec/aarch64/hevcdsp_sao_neon.S
> +++ b/libavcodec/aarch64/hevcdsp_sao_neon.S
> @@ -140,3 +140,54 @@ function ff_hevc_sao_edge_filter_16x16_8_neon, export=1
>         // no lines to filter
>         ret
> endfunc
> +
> +// ff_hevc_sao_edge_filter_8x8_8_neon(char *dst, char *src, ptrdiff stride_dst,
> +//                                    int16 *sao_offset_val, int eo, int width, int height)
> +function ff_hevc_sao_edge_filter_8x8_8_neon, export=1
> +        adr             x7, .Lsao_edge_pos
> +        ldr             w4, [x7, w4, uxtw #2]
> +        ld1             {v3.8h}, [x3]
> +        mov             v3.h[7], v3.h[0]
> +        mov             v3.h[0], v3.h[1]
> +        mov             v3.h[1], v3.h[2]
> +        mov             v3.h[2], v3.h[7]
> +        uzp2            v1.16b, v3.16b, v3.16b
> +        uzp1            v0.16b, v3.16b, v3.16b
> +        movi            v2.16b, #2
> +        add             x16, x0, x2
> +        lsl             x2,  x2, #1
> +        mov             x15, #192
> +        mov             x8,  x1
> +        sub             x9,  x1, x4
> +        add             x10, x1, x4
> +        lsr             w17, w6, #1

Compared with the previously applied (and reverted) patch, here, you 
previously had "mov x17, #4". I guess that'd mean the function only ever 
produced 8 output rows, while it now uses the real height parameter? Was 
this change a no-op (height is always 8?) or was this another hidden bug 
in the previous implementation?

> +1:      ld1             {v3.d}[0], [ x8], x15
> +        ld1             {v4.d}[0], [ x9], x15
> +        ld1             {v5.d}[0], [x10], x15
> +        ld1             {v3.d}[1], [ x8], x15
> +        ld1             {v4.d}[1], [ x9], x15
> +        ld1             {v5.d}[1], [x10], x15
> +        cmhi            v16.16b, v4.16b, v3.16b
> +        cmhi            v17.16b, v3.16b, v4.16b
> +        cmhi            v18.16b, v5.16b, v3.16b
> +        cmhi            v19.16b, v3.16b, v5.16b
> +        sub             v20.16b, v16.16b, v17.16b
> +        sub             v21.16b, v18.16b, v19.16b
> +        add             v20.16b, v20.16b, v21.16b
> +        add             v20.16b, v20.16b, v2.16b
> +        tbl             v16.16b, {v0.16b}, v20.16b
> +        tbl             v17.16b, {v1.16b}, v20.16b
> +        uxtl            v20.8h, v3.8b
> +        uxtl2           v21.8h, v3.16b
> +        zip1            v18.16b, v16.16b, v17.16b
> +        zip2            v19.16b, v16.16b, v17.16b
> +        sqadd           v20.8h, v18.8h, v20.8h
> +        sqadd           v21.8h, v19.8h, v21.8h
> +        sqxtun          v6.8b, v20.8h
> +        sqxtun          v7.8b, v21.8h
> +        st1             {v6.8b}, [ x0], x2
> +        st1             {v7.8b}, [x16], x2
> +        subs            x17, x17, #1

This could be "subs w6, w6, #2" and you wouldn't need the lsr instruction 
at all. And you could place the subs before the two st1 instructions to 
reduce latency between them a little. (The same thing goes for moving subs 
further away from the branch that uses its outcome in the previous patch 
too.) But as this is just a reapply of a previously committed and reverted 
patch, I guess it's fine this way too...

The patchset otherwise looks good to me, modulo the question about the 
difference to the previous patchset above.

// Martin