diff mbox series

[FFmpeg-devel,1/2] lavc/arm: dont assign hevc_qpel non-multiple of 8 width stubs

Message ID 20211016173552.49482-1-jdek@itanimul.li
State New
Headers show
Series [FFmpeg-devel,1/2] lavc/arm: dont assign hevc_qpel non-multiple of 8 width stubs
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

J. Dekker Oct. 16, 2021, 5:35 p.m. UTC
Signed-off-by: J. Dekker <jdek@itanimul.li>
---
 libavcodec/arm/hevcdsp_init_neon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Martin Storsjö Oct. 18, 2021, 12:31 p.m. UTC | #1
On Sat, 16 Oct 2021, J. Dekker wrote:

> Signed-off-by: J. Dekker <jdek@itanimul.li>
> ---
> libavcodec/arm/hevcdsp_init_neon.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/arm/hevcdsp_init_neon.c b/libavcodec/arm/hevcdsp_init_neon.c
> index 201a088dac..112edb5edd 100644
> --- a/libavcodec/arm/hevcdsp_init_neon.c
> +++ b/libavcodec/arm/hevcdsp_init_neon.c
> @@ -270,7 +270,8 @@ av_cold void ff_hevc_dsp_init_neon(HEVCDSPContext *c, const int bit_depth)
>         put_hevc_qpel_uw_neon[3][1]      = ff_hevc_put_qpel_uw_h1v3_neon_8;
>         put_hevc_qpel_uw_neon[3][2]      = ff_hevc_put_qpel_uw_h2v3_neon_8;
>         put_hevc_qpel_uw_neon[3][3]      = ff_hevc_put_qpel_uw_h3v3_neon_8;
> -        for (x = 0; x < 10; x++) {
> +        for (x = 3; x < 10; x++) {
> +            if (x == 4) continue;

The mapping between indices and sizes is quite non-obvious, would it be 
good to have a comment here explaining why? E.g. "The NEON functions 
assume that the width is a multiple of 8", and e.g. "x==4 -> width = 12" 
as comment for the continue statement?

This still doesn't explain why this hasn't been an issue in practice 
before the checkasm test. Do none of the fate samples ever trigger calling 
these functions with x<3 or x==4? Is that a limitation in our samples, or 
is it at all possible to make the decoder call those cases? I.e. should we 
try to find a sample to add that triggers it?

The commit message is a bit hard to read too. What about this?

---8<---
lavc/arm: dont assign hevc_qpel functions for non-multiple of 8 widths

The assembly is written assuming that the width is a multiple of 8.
---8<---

// Martin
diff mbox series

Patch

diff --git a/libavcodec/arm/hevcdsp_init_neon.c b/libavcodec/arm/hevcdsp_init_neon.c
index 201a088dac..112edb5edd 100644
--- a/libavcodec/arm/hevcdsp_init_neon.c
+++ b/libavcodec/arm/hevcdsp_init_neon.c
@@ -270,7 +270,8 @@  av_cold void ff_hevc_dsp_init_neon(HEVCDSPContext *c, const int bit_depth)
         put_hevc_qpel_uw_neon[3][1]      = ff_hevc_put_qpel_uw_h1v3_neon_8;
         put_hevc_qpel_uw_neon[3][2]      = ff_hevc_put_qpel_uw_h2v3_neon_8;
         put_hevc_qpel_uw_neon[3][3]      = ff_hevc_put_qpel_uw_h3v3_neon_8;
-        for (x = 0; x < 10; x++) {
+        for (x = 3; x < 10; x++) {
+            if (x == 4) continue;
             c->put_hevc_qpel[x][1][0]         = ff_hevc_put_qpel_neon_wrapper;
             c->put_hevc_qpel[x][0][1]         = ff_hevc_put_qpel_neon_wrapper;
             c->put_hevc_qpel[x][1][1]         = ff_hevc_put_qpel_neon_wrapper;