Re: [FFmpeg-devel] [PATCH] lavc/aarch64: new optimization for 8-bit hevc_pel_uni_w_pixels, qpel_uni_w_h, qpel_uni_w_v, qpel_uni_w_hv and qpel_h

["Martin Storsjö" <martin@martin.st>]

On Wed, 3 May 2023, myais wrote:

> Hello,
>
> - I splited this patch, Do I need to resubmit or just attach them as 
> attachments? (I attached those patches.  If I need to resummit, please let me 
> know.)

The attached form here is fine with me.

I didn't review it in detail yet, but I think it roughly looks ok, but 
there's a few issues.

The "fate-hevc" tests as part of our testsuite fails with these patches 
applied - please test that and make sure it passes.

The assembly fails to build with MSVC, with errors like these:

libavcodec\aarch64\hevcdsp_qpel_neon.o.asm(2278) 
: error A2173: syntax error in expression
         add     x2, x2, x3, lsl 1

The immediate constant 1 here should be prefixed with #, the same thing 
goes in a lot of other places in the same file.

If compiling only patch 1, it fails to build due to a mismatched #endif in 
the c file.

The second patch adds code that git flags as "No newline at end of file"; 
please don't do that, please make sure your editor saves the file as usual 
with a trailing newline.

The patches have some cases of trailing whitespace, please make sure you 
don't have time.

In the second patch, you're inconsistently using "#if 
__ARM_FEATURE_DOTPROD" and "#if defined(__ARM_FEATURE_DOTPROD)".

Dot product is a new feature we haven't taken advantage of on aarch64 
before. None of my toolchains/environments have this enabled by default. 
It would be good if you'd provide examples of where you're testing it and 
how you configure the build to enable it.

Because right now, most of this assembly runs untested unless building in 
a specific configuration that explicitly enables that extension.

For such features, we generally would want to always compile/assemble the 
feature (as long as the toolchain supports assembling it, with some extra 
flag), and use runtime checks for detecting whether the cpu feature is 
supported. But I guess adding support for that is a bigger separate 
project, and this approach with build time ifdefs if the toolchain 
mandates support for it, is tolerable for now.

If there's not a huge benefit from the dot product instructions, maybe it 
would be best to just not use them, so the whole wide audience can benefit 
from the optimizations?

// 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-request@ffmpeg.org with subject "unsubscribe".