[FFmpeg-devel] [PATCH v2 1/3] aarch64/vvc: Add w_avg

[Martin Storsjö]

On Thu, 26 Sep 2024, Zhao Zhili wrote:

>       --- a/libavcodec/aarch64/vvc/dsp_init.c
>       +++ b/libavcodec/aarch64/vvc/dsp_init.c
>       @@ -52,6 +52,37 @@ void ff_vvc_avg_12_neon(uint8_t *dst,
>       ptrdiff_t dst_stride,
>                              const int16_t *src0, const int16_t
>       *src1, int width,
>                              int height);
>
>       +void ff_vvc_w_avg_8_neon(uint8_t *_dst, const ptrdiff_t
>       _dst_stride,
>       +                         const int16_t *src0, const
>       int16_t *src1,
>       +                         const int width, const int
>       height,
>       +                         uintptr_t w0_w1, uintptr_t
>       offset_shift);
> 
> 
> Including "const" on scalar parameters is entirely redundant, and we
> don't prescribe use of that elsewhere in ffmpeg, and just makes the
> whole declaration more noisy.
> 
> 
> I see these “const” make clang-tidy not happy. They are here to keep
> consistent with the prototypes
> in vvc/dsp.h.

Hmm, I don't quite understand this comment - so you say that clang-tidy, 
in addition to me, also complain about them? But they are added manually 
to keep the prototypes exactly in sync? Or does clang-tidy complain about 
differences here, if we differ on the constness here?

> There are three options:
> 
> 1. Keep “const” as current state
> 2. Drop “const” only for these new functions
> 3. Remove “const” from vvc/dsp.h and all implementations
> 
> I can’t decide which way to go.

I would go for 3, at least long term.

If you need to keep the const within the function prototypes here for now 
to please some tool (I think most compilers wouldn't complain about 
differences in const on scalar parameters, although I think old MSVC did 
that), that's ok, but I would remove it from the unnecessary places (the 
local variables in the function, the parameter/register mappings in 
assembly).

Then we can try to do 3 as a later step.

// Martin