[FFmpeg-devel] [PATCH] lavc/h264chroma: RISC-V V add motion compensation for 8x8 chroma blocks

[Lynne]

May 17, 2023, 09:13 by arnie.chang at sifive.com:

> Optimize the put and avg filtering for 8x8 chroma blocks
>
> Signed-off-by: Arnie Chang <arnie.chang at sifive.com>
> ---
> libavcodec/h264chroma.c                   |   2 +
> libavcodec/h264chroma.h                   |   1 +
> libavcodec/riscv/Makefile                 |   3 +
> libavcodec/riscv/h264_chroma_init_riscv.c |  39 ++
> libavcodec/riscv/h264_mc_chroma.S         | 492 ++++++++++++++++++++++
> libavcodec/riscv/h264_mc_chroma.h         |  34 ++
> 6 files changed, 571 insertions(+)
> create mode 100644 libavcodec/riscv/h264_chroma_init_riscv.c
> create mode 100644 libavcodec/riscv/h264_mc_chroma.S
> create mode 100644 libavcodec/riscv/h264_mc_chroma.h
>
> +#include <stdint.h>
> +
> +#include "libavutil/attributes.h"
> +#include "libavutil/cpu.h"
> +#include "libavcodec/h264chroma.h"
> +#include "config.h"
> +#include "h264_mc_chroma.h"
> +
> +av_cold void ff_h264chroma_init_riscv(H264ChromaContext *c, int bit_depth)
> +{
> +#if HAVE_RVV
> +    const int high_bit_depth = bit_depth > 8;
>

You don't need this constant.


> +
> +    if (!high_bit_depth) {
> +        c->put_h264_chroma_pixels_tab[0] = h264_put_chroma_mc8_rvv;
> +        c->avg_h264_chroma_pixels_tab[0] = h264_avg_chroma_mc8_rvv;
> +    }
> +#endif
>

You have to check if RVV is supported:
> int flags = av_get_cpu_flags();
>
> if (flags & AV_CPU_FLAG_RVV_F32) {
>     if (bit_depth > 8) {


> +    .text
> +
> +    .globl    h264_put_chroma_mc8_rvv
> +    .p2align    1
> +    .type    h264_put_chroma_mc8_rvv, at function
> +h264_put_chroma_mc8_rvv:
>

You don't need any of this. We already have macros to
handle this - take a look at libavcodec/riscv/opusdsp_rvv.S:

> func ff_opus_postfilter_rvv_256, zve32f
>         lvtypei a5, e32, m1, ta, ma // function instructions start here

Make sure to change zve32f to whatever instruction extension you use
to initialize the assembler to handle it.


> +    slliw    t2, a5, 3
> +    mulw    t1, a5, a4
> +    sh3add    a5, a4, t2
> +    slliw    a4, a4, 3
> +    subw    a5, t1, a5
> +    subw    a7, a4, t1
> +    addiw    a6, a5, 64
> +    subw    t0, t2, t1
>

Coding style issue - we style our RISC-V assembly the same way
we style our AArch64 assembly:

<8 spaces><instruction><spaces until the 24th character on the line><arguments,registers .etc>

For example:
>         vsetvl          zero, a4, a5
>         lw              t2, 20(a1)
>         vfmul.vv        v8, v24, v16
>         addi            a0, a0, 4
>         vslide1down.vx  v16, v16, t2
>         MACRO           arg1, arg2


> +.LBB0_8:                                # if ((x8 - xy) == 0 && (y8 -xy) != 0)
> +    add    a5, a1, a4
> +    vsetvli    zero, zero, e8, m1, ta, ma
> +    addiw    t1, t1, 4
> +    vle8.v    v8, (a5)
> +    add    a5, a5, a2
> +    add    t2, a5, a2
> +    vwmulu.vx    v10, v8, a6
>

This branch looks very similar to
> .LBB1_16:                               # the final else, none of the above conditions are met
>     add    a5, a1, a4
>     vsetvli    zero, zero, e8, m1, ta, ma
>     addiw    t0, t0, 4
>     vle8.v    v8, (a5)
>     add    a5, a5, a2
>     add    t1, a5, a2
>     vwmulu.vx    v10, v8, a6

Consider using a macro.

In fact, a lot of the branches look similar to each other. Looking at other
implementations, they only consider 3 possible variants, the same ones
that the C function has.


> +    .size    h264_avg_chroma_mc8_rvv, .Lfunc_end1-h264_avg_chroma_mc8_rvv
> diff --git a/libavcodec/riscv/h264_mc_chroma.h b/libavcodec/riscv/h264_mc_chroma.h
> new file mode 100644
> index 0000000000..cb350d0e4a
> --- /dev/null
> +++ b/libavcodec/riscv/h264_mc_chroma.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright (c) 2023 SiFive, Inc. All rights reserved.
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVCODEC_RISCV_H264_MC_CHROMA_H
> +#define AVCODEC_RISCV_H264_MC_CHROMA_H
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <stddef.h>
> +#include "config.h"
>

You don't need all of these includes. Just config.h and stdint.h would be enough.


> +#if HAVE_RVV
> +void h264_put_chroma_mc8_rvv(uint8_t *p_dst, const uint8_t *p_src, ptrdiff_t stride, int h, int x, int y);
> +void h264_avg_chroma_mc8_rvv(uint8_t *p_dst, const uint8_t *p_src, ptrdiff_t stride, int h, int x, int y);
> +#endif
> +#endif
> \ No newline at end of file
>

You need your file to end in a newline. Git already warns you if you don't.

Finally, run:
make checkasm && ./tests/checkasm/checkasm --bench
and report on the timings for both the C and assembly versions.
If you've made a mistake somewhere, (forgot to restore stack, or a callee-saved register,
or your function produces an incorrect result), checkasm will fail.