[FFmpeg-devel] [PATCH 0/6] avcodec/vc1: Arm optimisations

[Martin Storsjö]

Hi Ben,

On Thu, 17 Mar 2022, Ben Avison wrote:

> The VC1 decoder was missing lots of important fast paths for Arm, especially
> for 64-bit Arm. This submission fills in implementations for all functions
> where a fast path already existed and the fallback C implementation was
> taking 1% or more of the runtime, and adds a new fast path to permit
> vc1_unescape_buffer() to be overridden.
>
> I've measured the playback speed on a 1.5 GHz Cortex-A72 (Raspberry Pi 4)
> using `ffmpeg -i <bitstream> -f null -` for a couple of example streams:
>
> Architecture:  AArch32    AArch32    AArch64    AArch64
> Stream:        1          2          1          2
> Before speed:  1.22x      0.82x      1.00x      0.67x
> After speed:   1.31x      0.98x      1.39x      1.06x
> Improvement:   7.4%       20%        39%        58%
>
> `make fate` passes on both AArch32 and AArch64.

Thanks for the patches! I have looked briefly at the patches (I haven't 
started reading the implementation in detail yet though).

As you are writing assembly for these functions, I would very much 
appreciate if you could add checkasm tests for all the functions you're 
implementing. I see that there exists a test for the blockdsp functions, 
but all the other ones are missing a test.

I try to request such tests for all new assembly. Such a test allows 
testing all interesting cornercases of the DSP functions with one concise 
test, instead of having to run the full fate testsuite. It also allows 
catching a number of other possible lingering issues, like using the full 
64 bit register when the argument only set 32 bits where the upper bits 
are undefined, or missing to restore callee saved registers, etc. It also 
allows for easy benchmarking of the functions on their own, which is very 
useful for tuning of the implementation. And it finally allows easily 
checking that the assembly works correctly when built with a different 
toolchain for a different platform - without needing to run the full 
decoding tests.

Especially as you've been implementing the functions, you're probably more 
familiar with the expectations and behaviours (and potential cornercases 
that are worth testing) of the functions than most other developers in the 
community at the moment, which is good for writing useful testcases.

There's plenty of existing examples of such tests - the h264dsp, vp8dsp 
and vp9dsp cases might be relevant.


The other main issue I'd like to request is to indent the assembly 
similarly to the rest of the existing assembly. For the 32 bit assembly, 
your patches do match the surrounding code, but for the 64 bit assembly, 
your patches align the operands column differently than the rest. (I think 
your code aligns the operands with 16 chars to the left of the operands, 
while our code aligns it with 24 chars to the left, both in 32 and 64 bit 
arm assembly.)


Finally, the 32 bit assembly fails to build for me both with (recent) 
clang and old binutils, with errors like these:

src/libavcodec/arm/vc1dsp_neon.S: Assembler messages:
src/libavcodec/arm/vc1dsp_neon.S:1579: Error: bad type for scalar -- `vmov r0,d4[1]'
src/libavcodec/arm/vc1dsp_neon.S:1582: Error: bad type for scalar -- `vmov r2,d5[1]'
src/libavcodec/arm/vc1dsp_neon.S:1592: Error: bad type for scalar -- `vmov r2,d8[1]'
src/libavcodec/arm/vc1dsp_neon.S:1595: Error: bad type for scalar -- `vmov r12,d9[1]'

Qualifying the "vmov" into "vmov.32" seems to fix it.

// Martin