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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
