Re: [FFmpeg-devel] [PATCH] avcodec/aarch64/hevc: add transform_luma_4x4_neon note: run_count=1000, CPU=Cortex A53 transform_4x4_luma_neon: 45 transform_4x4_luma_c: 103

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

On Sat, 11 Mar 2023, xufuji456 wrote:

> ---
> libavcodec/aarch64/hevcdsp_idct_neon.S    | 52 ++++++++++++++++++++++-
> libavcodec/aarch64/hevcdsp_init_aarch64.c |  2 +
> 2 files changed, 53 insertions(+), 1 deletion(-)

Overall the code seems fine, but there's a couple more minor issues:

- There's no checkasm test, so we don't have any continuous tracking that 
this function doesn't break ABI details subtly. From reading the code it 
seems fine, but we really want to have checkasm coverage for all new 
assembly. Can you please add one? It should hopefully not be very 
complicated given the existing tests for other idct parts.

- The commit message is a bit garbled - not all that text belongs in the 
subject line.

- It was hard to get the patch applied:

> diff --git a/libavcodec/aarch64/hevcdsp_idct_neon.S b/libavcodec/aarch64/hevcdsp_idct_neon.S
> index b11f56862a..00d9690466 100644
> --- a/libavcodec/aarch64/hevcdsp_idct_neon.S
> +++ b/libavcodec/aarch64/hevcdsp_idct_neon.S
> @@ -665,4 +667,52 @@ idct_dc 16, 8
> idct_dc 16, 10
> 
> idct_dc 32, 8
> -idct_dc 32, 10
> \ No newline at end of file
> +idct_dc 32, 10

The file upstream actually did have the proper newline at the end of the 
file (some earlier patch of yours, I think, were missing this but I fixed 
it up when pushing it), but the patch expected a file without a trailing 
newline. It would be much less hassle for me to apply the patch if it was 
properly rebased on the actually pushed git master version.

// 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".