[FFmpeg-devel] [PATCHv4 1/4] lavc/h263dsp: add DCT dequantisation functions

[FFmpeg-devel] [PATCHv4 1/4] lavc/h263dsp: add DCT dequantisation functions

[Rémi Denis-Courmont]

Note that optimised implementations of these functions will be taken
into actual use only if MpegEncContext.dct_unquantize_h263_{inter,intra}
are *not* overloaded by existing optimisations.
---
 libavcodec/h263dsp.c | 25 +++++++++++++++++++++++++
 libavcodec/h263dsp.h |  4 ++++
 2 files changed, 29 insertions(+)

diff --git a/libavcodec/h263dsp.c b/libavcodec/h263dsp.c
index 6a13353499..755d7077bf 100644
--- a/libavcodec/h263dsp.c
+++ b/libavcodec/h263dsp.c
@@ -23,6 +23,29 @@
 #include "config.h"
 #include "h263dsp.h"
 
+static void h263_dct_unquantize_inter_c(int16_t *block, size_t nCoeffs,
+                                        int qmul, int qadd)
+{
+    for (size_t i = 0; i <= nCoeffs; i++) {
+        int level = block[i];
+
+        if (level) {
+            if (level < 0)
+                level = level * qmul - qadd;
+            else
+                level = level * qmul + qadd;
+            block[i] = level;
+        }
+    }
+}
+
+static void h263_dct_unquantize_intra_c(int16_t *block, size_t nCoeffs,
+                                        int qmul, int qadd)
+{
+    if (nCoeffs > 0)
+        h263_dct_unquantize_inter_c(block + 1, nCoeffs - 1, qmul, qadd);
+}
+
 const uint8_t ff_h263_loop_filter_strength[32] = {
     0, 1, 1, 2, 2, 3, 3,  4,  4,  4,  5,  5,  6,  6,  7, 7,
     7, 8, 8, 8, 9, 9, 9, 10, 10, 10, 11, 11, 11, 12, 12, 12
@@ -116,6 +139,8 @@ static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale)
 
 av_cold void ff_h263dsp_init(H263DSPContext *ctx)
 {
+    ctx->h263_dct_unquantize_intra = h263_dct_unquantize_intra_c;
+    ctx->h263_dct_unquantize_inter = h263_dct_unquantize_inter_c;
     ctx->h263_h_loop_filter = h263_h_loop_filter_c;
     ctx->h263_v_loop_filter = h263_v_loop_filter_c;
 
diff --git a/libavcodec/h263dsp.h b/libavcodec/h263dsp.h
index 2dccd23392..0ecbe83314 100644
--- a/libavcodec/h263dsp.h
+++ b/libavcodec/h263dsp.h
@@ -24,6 +24,10 @@
 extern const uint8_t ff_h263_loop_filter_strength[32];
 
 typedef struct H263DSPContext {
+    void (*h263_dct_unquantize_intra)(int16_t *block /* align 16 */,
+                                      size_t len, int mul, int add);
+    void (*h263_dct_unquantize_inter)(int16_t *block /* align 16 */,
+                                      size_t len, int mul, int add);
     void (*h263_h_loop_filter)(uint8_t *src, int stride, int qscale);
     void (*h263_v_loop_filter)(uint8_t *src, int stride, int qscale);
 } H263DSPContext;
-- 
2.45.1

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

[FFmpeg-devel] [PATCHv4 2/4] lavc/mpegvideo: use H263DSP dequant function

[Rémi Denis-Courmont]

---
 libavcodec/mpegvideo.c | 40 +++++++++-------------------------------
 1 file changed, 9 insertions(+), 31 deletions(-)

diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
index 7af823b8bd..810888fc47 100644
--- a/libavcodec/mpegvideo.c
+++ b/libavcodec/mpegvideo.c
@@ -201,13 +201,11 @@ static void dct_unquantize_mpeg2_inter_c(MpegEncContext *s,
 static void dct_unquantize_h263_intra_c(MpegEncContext *s,
                                   int16_t *block, int n, int qscale)
 {
-    int i, level, qmul, qadd;
-    int nCoeffs;
+    int qmul = qscale << 1;
+    int qadd, nCoeffs;
 
     av_assert2(s->block_last_index[n]>=0 || s->h263_aic);
 
-    qmul = qscale << 1;
-
     if (!s->h263_aic) {
         block[0] *= n < 4 ? s->y_dc_scale : s->c_dc_scale;
         qadd = (qscale - 1) | 1;
@@ -219,43 +217,20 @@ static void dct_unquantize_h263_intra_c(MpegEncContext *s,
     else
         nCoeffs= s->intra_scantable.raster_end[ s->block_last_index[n] ];
 
-    for(i=1; i<=nCoeffs; i++) {
-        level = block[i];
-        if (level) {
-            if (level < 0) {
-                level = level * qmul - qadd;
-            } else {
-                level = level * qmul + qadd;
-            }
-            block[i] = level;
-        }
-    }
+    s->h263dsp.h263_dct_unquantize_intra(block, nCoeffs, qmul, qadd);
 }
 
 static void dct_unquantize_h263_inter_c(MpegEncContext *s,
                                   int16_t *block, int n, int qscale)
 {
-    int i, level, qmul, qadd;
+    int qmul = qscale << 1;
+    int qadd = (qscale - 1) | 1;
     int nCoeffs;
 
     av_assert2(s->block_last_index[n]>=0);
 
-    qadd = (qscale - 1) | 1;
-    qmul = qscale << 1;
-
     nCoeffs= s->inter_scantable.raster_end[ s->block_last_index[n] ];
-
-    for(i=0; i<=nCoeffs; i++) {
-        level = block[i];
-        if (level) {
-            if (level < 0) {
-                level = level * qmul - qadd;
-            } else {
-                level = level * qmul + qadd;
-            }
-            block[i] = level;
-        }
-    }
+    s->h263dsp.h263_dct_unquantize_inter(block, nCoeffs, qmul, qadd);
 }
 
 
@@ -275,6 +250,9 @@ static void gray8(uint8_t *dst, const uint8_t *src, ptrdiff_t linesize, int h)
 static av_cold int dct_init(MpegEncContext *s)
 {
     ff_blockdsp_init(&s->bdsp);
+#if CONFIG_H263DSP
+    ff_h263dsp_init(&s->h263dsp);
+#endif
     ff_hpeldsp_init(&s->hdsp, s->avctx->flags);
     ff_videodsp_init(&s->vdsp, s->avctx->bits_per_raw_sample);
 
-- 
2.45.1

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

[FFmpeg-devel] [PATCHv4 3/4] checkasm/h263dsp: test dct_unquantize_{intra, inter}

[Rémi Denis-Courmont]

---
 tests/checkasm/h263dsp.c | 53 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/tests/checkasm/h263dsp.c b/tests/checkasm/h263dsp.c
index 2d0957a90b..a8530403ee 100644
--- a/tests/checkasm/h263dsp.c
+++ b/tests/checkasm/h263dsp.c
@@ -18,13 +18,61 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  */
 
+#include <stdbool.h>
 #include <string.h>
 
 #include "checkasm.h"
 
-#include "libavcodec/h263dsp.h"
+#include "libavutil/avassert.h"
 #include "libavutil/mem.h"
 #include "libavutil/mem_internal.h"
+#include "libavcodec/h263dsp.h"
+#include "libavcodec/mpegvideodata.h"
+
+static uint_fast8_t mpeg_qscale_rnd(void)
+{
+    int n = rnd(), q = (n >> 1) & 31;
+
+    if (n & 1)
+        return ff_mpeg2_non_linear_qscale[q];
+    else
+        return q << 1;
+}
+
+typedef void (*unquantizer)(int16_t *, size_t, int, int);
+
+static void check_dct_unquantize(unquantizer func, const char *name)
+{
+#define LEN 64
+    LOCAL_ALIGNED_16(int16_t, block0, [LEN]);
+    LOCAL_ALIGNED_16(int16_t, block1, [LEN]);
+    size_t coeffs = rnd() % LEN;
+    const int qscale = mpeg_qscale_rnd();
+    const int qmul = qscale << 1;
+    const int qadd = (rnd() & 1) ? (qscale - 1) | 1 : 0;
+
+    declare_func(void, int16_t *, size_t, int, int);
+
+    for (size_t i = 0; i < LEN; i++)
+        block1[i] = block0[i] = (rnd() & 1) ? rnd() : 0;
+
+    if (check_func(func, "h263dsp.dct_unquantize_%s", name)) {
+        call_ref(block0, 0, qmul, qadd);
+        call_new(block1, 0, qmul, qadd);
+
+        if (block0[0] != block1[0])
+            fail();
+
+        av_assert0(coeffs < LEN);
+        call_ref(block0, coeffs, qmul, qadd);
+        call_new(block1, coeffs, qmul, qadd);
+
+        if (memcmp(block0, block1, (coeffs + 1) * sizeof (int16_t)))
+            fail();
+
+        bench_new(block1, LEN - 1, qmul, qadd);
+    }
+}
 
 typedef void (*filter)(uint8_t *src, int stride, int qscale);
 
@@ -56,6 +104,9 @@ void checkasm_check_h263dsp(void)
     H263DSPContext ctx;
 
     ff_h263dsp_init(&ctx);
+    check_dct_unquantize(ctx.h263_dct_unquantize_intra, "intra");
+    check_dct_unquantize(ctx.h263_dct_unquantize_inter, "inter");
+    report("dct_unquantize");
     check_loop_filter('h', ctx.h263_h_loop_filter);
     check_loop_filter('v', ctx.h263_v_loop_filter);
     report("loop_filter");
-- 
2.45.1

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

[FFmpeg-devel] [PATCHv4 4/4] lavc/h263dsp: R-V V dct_unquantize_{intra, inter}

[Rémi Denis-Courmont]

T-Head C908:
h263dsp.dct_unquantize_inter_c:       3.7
h263dsp.dct_unquantize_inter_rvv_i32: 1.7
h263dsp.dct_unquantize_intra_c:       4.0
h263dsp.dct_unquantize_intra_rvv_i32: 2.0

SpacemiT X60:
h263dsp.dct_unquantize_inter_c:       3.5
h263dsp.dct_unquantize_inter_rvv_i32: 1.5
h263dsp.dct_unquantize_intra_c:       3.5
h263dsp.dct_unquantize_intra_rvv_i32: 1.5
---
 libavcodec/riscv/h263dsp_init.c | 15 ++++++++++++---
 libavcodec/riscv/h263dsp_rvv.S  | 26 ++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/libavcodec/riscv/h263dsp_init.c b/libavcodec/riscv/h263dsp_init.c
index 21b536366c..5fb12f360b 100644
--- a/libavcodec/riscv/h263dsp_init.c
+++ b/libavcodec/riscv/h263dsp_init.c
@@ -25,6 +25,8 @@
 #include "libavutil/riscv/cpu.h"
 #include "libavcodec/h263dsp.h"
 
+void ff_h263_dct_unquantize_intra_rvv(int16_t *, size_t len, int, int);
+void ff_h263_dct_unquantize_inter_rvv(int16_t *, size_t len, int, int);
 void ff_h263_h_loop_filter_rvv(uint8_t *src, int stride, int q);
 void ff_h263_v_loop_filter_rvv(uint8_t *src, int stride, int q);
 
@@ -33,9 +35,16 @@ av_cold void ff_h263dsp_init_riscv(H263DSPContext *c)
 #if HAVE_RVV
     int flags = av_get_cpu_flags();
 
-    if ((flags & AV_CPU_FLAG_RVV_I32) && ff_rv_vlen_least(128)) {
-        c->h263_h_loop_filter = ff_h263_h_loop_filter_rvv;
-        c->h263_v_loop_filter = ff_h263_v_loop_filter_rvv;
+    if (flags & AV_CPU_FLAG_RVV_I32) {
+        if (ff_rv_vlen_least(128) || (flags & AV_CPU_FLAG_RVB_ADDR)) {
+            c->h263_dct_unquantize_intra = ff_h263_dct_unquantize_intra_rvv;
+            c->h263_dct_unquantize_inter = ff_h263_dct_unquantize_inter_rvv;
+        }
+
+        if (ff_rv_vlen_least(128)) {
+            c->h263_h_loop_filter = ff_h263_h_loop_filter_rvv;
+            c->h263_v_loop_filter = ff_h263_v_loop_filter_rvv;
+        }
     }
 #endif
 }
diff --git a/libavcodec/riscv/h263dsp_rvv.S b/libavcodec/riscv/h263dsp_rvv.S
index 97503d527c..78847b9d44 100644
--- a/libavcodec/riscv/h263dsp_rvv.S
+++ b/libavcodec/riscv/h263dsp_rvv.S
@@ -20,6 +20,32 @@
 
 #include "libavutil/riscv/asm.S"
 
+func ff_h263_dct_unquantize_intra_rvv, zve32x
+        addi    a0, a0, 2
+        j       1f
+endfunc
+
+func ff_h263_dct_unquantize_inter_rvv, zve32x
+        addi    a1, a1, 1
+1:
+        vsetvli  t0, a1, e16, m8, ta, mu
+        vle16.v  v8, (a0)
+        sub      a1, a1, t0
+        vmv.v.x  v24, a3
+        vmslt.vi v0, v8, 0
+        vmul.vx  v16, v8, a2
+        vneg.v   v24, v24, v0.t
+        vmsne.vi v0, v8, 0
+        vadd.vv  v8, v16, v24, v0.t
+        vse16.v v8, (a0)
+#if defined(__riscv_v_min_vlen) && __riscv_v_min_vlen < 128
+        sh1add  a0, t0, a0
+        bnez    a1, 1b
+#endif
+
+        ret
+endfunc
+
         .option push
         .option norelax
 func ff_h263_h_loop_filter_rvv, zve32x
-- 
2.45.1

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

Re: [FFmpeg-devel] [PATCHv4 1/4] lavc/h263dsp: add DCT dequantisation functions

[Andreas Rheinhardt]

Rémi Denis-Courmont:
> Note that optimised implementations of these functions will be taken
> into actual use only if MpegEncContext.dct_unquantize_h263_{inter,intra}
> are *not* overloaded by existing optimisations.
> ---
>  libavcodec/h263dsp.c | 25 +++++++++++++++++++++++++
>  libavcodec/h263dsp.h |  4 ++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/libavcodec/h263dsp.c b/libavcodec/h263dsp.c
> index 6a13353499..755d7077bf 100644
> --- a/libavcodec/h263dsp.c
> +++ b/libavcodec/h263dsp.c
> @@ -23,6 +23,29 @@
>  #include "config.h"
>  #include "h263dsp.h"
>  
> +static void h263_dct_unquantize_inter_c(int16_t *block, size_t nCoeffs,
> +                                        int qmul, int qadd)
> +{
> +    for (size_t i = 0; i <= nCoeffs; i++) {
> +        int level = block[i];
> +
> +        if (level) {
> +            if (level < 0)
> +                level = level * qmul - qadd;
> +            else
> +                level = level * qmul + qadd;
> +            block[i] = level;
> +        }
> +    }
> +}
> +
> +static void h263_dct_unquantize_intra_c(int16_t *block, size_t nCoeffs,
> +                                        int qmul, int qadd)
> +{
> +    if (nCoeffs > 0)

Great, a branch. Why don't you just use a signed type?

> +        h263_dct_unquantize_inter_c(block + 1, nCoeffs - 1, qmul, qadd);
> +}
> +
>  const uint8_t ff_h263_loop_filter_strength[32] = {
>      0, 1, 1, 2, 2, 3, 3,  4,  4,  4,  5,  5,  6,  6,  7, 7,
>      7, 8, 8, 8, 9, 9, 9, 10, 10, 10, 11, 11, 11, 12, 12, 12
> @@ -116,6 +139,8 @@ static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale)
>  
>  av_cold void ff_h263dsp_init(H263DSPContext *ctx)
>  {
> +    ctx->h263_dct_unquantize_intra = h263_dct_unquantize_intra_c;
> +    ctx->h263_dct_unquantize_inter = h263_dct_unquantize_inter_c;
>      ctx->h263_h_loop_filter = h263_h_loop_filter_c;
>      ctx->h263_v_loop_filter = h263_v_loop_filter_c;
>  
> diff --git a/libavcodec/h263dsp.h b/libavcodec/h263dsp.h
> index 2dccd23392..0ecbe83314 100644
> --- a/libavcodec/h263dsp.h
> +++ b/libavcodec/h263dsp.h
> @@ -24,6 +24,10 @@
>  extern const uint8_t ff_h263_loop_filter_strength[32];
>  
>  typedef struct H263DSPContext {
> +    void (*h263_dct_unquantize_intra)(int16_t *block /* align 16 */,
> +                                      size_t len, int mul, int add);
> +    void (*h263_dct_unquantize_inter)(int16_t *block /* align 16 */,
> +                                      size_t len, int mul, int add);
>      void (*h263_h_loop_filter)(uint8_t *src, int stride, int qscale);
>      void (*h263_v_loop_filter)(uint8_t *src, int stride, int qscale);
>  } H263DSPContext;

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

Re: [FFmpeg-devel] [PATCHv4 2/4] lavc/mpegvideo: use H263DSP dequant function

[Andreas Rheinhardt]

Rémi Denis-Courmont:
> ---
>  libavcodec/mpegvideo.c | 40 +++++++++-------------------------------
>  1 file changed, 9 insertions(+), 31 deletions(-)
> 
> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
> index 7af823b8bd..810888fc47 100644
> --- a/libavcodec/mpegvideo.c
> +++ b/libavcodec/mpegvideo.c
> @@ -201,13 +201,11 @@ static void dct_unquantize_mpeg2_inter_c(MpegEncContext *s,
>  static void dct_unquantize_h263_intra_c(MpegEncContext *s,
>                                    int16_t *block, int n, int qscale)
>  {
> -    int i, level, qmul, qadd;
> -    int nCoeffs;
> +    int qmul = qscale << 1;
> +    int qadd, nCoeffs;
>  
>      av_assert2(s->block_last_index[n]>=0 || s->h263_aic);
>  
> -    qmul = qscale << 1;
> -
>      if (!s->h263_aic) {
>          block[0] *= n < 4 ? s->y_dc_scale : s->c_dc_scale;
>          qadd = (qscale - 1) | 1;
> @@ -219,43 +217,20 @@ static void dct_unquantize_h263_intra_c(MpegEncContext *s,
>      else
>          nCoeffs= s->intra_scantable.raster_end[ s->block_last_index[n] ];
>  
> -    for(i=1; i<=nCoeffs; i++) {
> -        level = block[i];
> -        if (level) {
> -            if (level < 0) {
> -                level = level * qmul - qadd;
> -            } else {
> -                level = level * qmul + qadd;
> -            }
> -            block[i] = level;
> -        }
> -    }
> +    s->h263dsp.h263_dct_unquantize_intra(block, nCoeffs, qmul, qadd);
>  }
>  
>  static void dct_unquantize_h263_inter_c(MpegEncContext *s,
>                                    int16_t *block, int n, int qscale)
>  {
> -    int i, level, qmul, qadd;
> +    int qmul = qscale << 1;
> +    int qadd = (qscale - 1) | 1;
>      int nCoeffs;
>  
>      av_assert2(s->block_last_index[n]>=0);
>  
> -    qadd = (qscale - 1) | 1;
> -    qmul = qscale << 1;
> -
>      nCoeffs= s->inter_scantable.raster_end[ s->block_last_index[n] ];
> -
> -    for(i=0; i<=nCoeffs; i++) {
> -        level = block[i];
> -        if (level) {
> -            if (level < 0) {
> -                level = level * qmul - qadd;
> -            } else {
> -                level = level * qmul + qadd;
> -            }
> -            block[i] = level;
> -        }
> -    }
> +    s->h263dsp.h263_dct_unquantize_inter(block, nCoeffs, qmul, qadd);

What is the cost of these function calls?

>  }
>  
>  
> @@ -275,6 +250,9 @@ static void gray8(uint8_t *dst, const uint8_t *src, ptrdiff_t linesize, int h)
>  static av_cold int dct_init(MpegEncContext *s)
>  {
>      ff_blockdsp_init(&s->bdsp);
> +#if CONFIG_H263DSP
> +    ff_h263dsp_init(&s->h263dsp);
> +#endif

This ensures that no linking errors happen, but the H.261 codecs will
nevertheless be broken when the H.263 codecs are disabled. In this case
they will try to call a NULL function pointer.

>      ff_hpeldsp_init(&s->hdsp, s->avctx->flags);
>      ff_videodsp_init(&s->vdsp, s->avctx->bits_per_raw_sample);
>  

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

Re: [FFmpeg-devel] [PATCHv4 2/4] lavc/mpegvideo: use H263DSP dequant function

[Rémi Denis-Courmont]

Le 11 juin 2024 13:37:57 GMT+03:00, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> a écrit :
>Rémi Denis-Courmont:
>> ---
>>  libavcodec/mpegvideo.c | 40 +++++++++-------------------------------
>>  1 file changed, 9 insertions(+), 31 deletions(-)
>> 
>> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
>> index 7af823b8bd..810888fc47 100644
>> --- a/libavcodec/mpegvideo.c
>> +++ b/libavcodec/mpegvideo.c
>> @@ -201,13 +201,11 @@ static void dct_unquantize_mpeg2_inter_c(MpegEncContext *s,
>>  static void dct_unquantize_h263_intra_c(MpegEncContext *s,
>>                                    int16_t *block, int n, int qscale)
>>  {
>> -    int i, level, qmul, qadd;
>> -    int nCoeffs;
>> +    int qmul = qscale << 1;
>> +    int qadd, nCoeffs;
>>  
>>      av_assert2(s->block_last_index[n]>=0 || s->h263_aic);
>>  
>> -    qmul = qscale << 1;
>> -
>>      if (!s->h263_aic) {
>>          block[0] *= n < 4 ? s->y_dc_scale : s->c_dc_scale;
>>          qadd = (qscale - 1) | 1;
>> @@ -219,43 +217,20 @@ static void dct_unquantize_h263_intra_c(MpegEncContext *s,
>>      else
>>          nCoeffs= s->intra_scantable.raster_end[ s->block_last_index[n] ];
>>  
>> -    for(i=1; i<=nCoeffs; i++) {
>> -        level = block[i];
>> -        if (level) {
>> -            if (level < 0) {
>> -                level = level * qmul - qadd;
>> -            } else {
>> -                level = level * qmul + qadd;
>> -            }
>> -            block[i] = level;
>> -        }
>> -    }
>> +    s->h263dsp.h263_dct_unquantize_intra(block, nCoeffs, qmul, qadd);
>>  }
>>  
>>  static void dct_unquantize_h263_inter_c(MpegEncContext *s,
>>                                    int16_t *block, int n, int qscale)
>>  {
>> -    int i, level, qmul, qadd;
>> +    int qmul = qscale << 1;
>> +    int qadd = (qscale - 1) | 1;
>>      int nCoeffs;
>>  
>>      av_assert2(s->block_last_index[n]>=0);
>>  
>> -    qadd = (qscale - 1) | 1;
>> -    qmul = qscale << 1;
>> -
>>      nCoeffs= s->inter_scantable.raster_end[ s->block_last_index[n] ];
>> -
>> -    for(i=0; i<=nCoeffs; i++) {
>> -        level = block[i];
>> -        if (level) {
>> -            if (level < 0) {
>> -                level = level * qmul - qadd;
>> -            } else {
>> -                level = level * qmul + qadd;
>> -            }
>> -            block[i] = level;
>> -        }
>> -    }
>> +    s->h263dsp.h263_dct_unquantize_inter(block, nCoeffs, qmul, qadd);
>
>What is the cost of these function calls?

The same as any other indirect function tail call. What do you want me to say?! Changing how FFmpeg handles assembler optimisations, or removing the upper level of indirection is outside the scope of this MR (and not very realistic).

For that matter, there are hundreds of indirect calls that could be optimised out on some platforms, notably almost all Armv8 NEON functions and, on x86-64, all MMX or SSE2 exclusives. I don't get nitpicking this one.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCHv4 2/4] lavc/mpegvideo: use H263DSP dequant function

[Andreas Rheinhardt]

Rémi Denis-Courmont:
> 
> 
> Le 11 juin 2024 13:37:57 GMT+03:00, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> a écrit :
>> Rémi Denis-Courmont:
>>> ---
>>>  libavcodec/mpegvideo.c | 40 +++++++++-------------------------------
>>>  1 file changed, 9 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
>>> index 7af823b8bd..810888fc47 100644
>>> --- a/libavcodec/mpegvideo.c
>>> +++ b/libavcodec/mpegvideo.c
>>> @@ -201,13 +201,11 @@ static void dct_unquantize_mpeg2_inter_c(MpegEncContext *s,
>>>  static void dct_unquantize_h263_intra_c(MpegEncContext *s,
>>>                                    int16_t *block, int n, int qscale)
>>>  {
>>> -    int i, level, qmul, qadd;
>>> -    int nCoeffs;
>>> +    int qmul = qscale << 1;
>>> +    int qadd, nCoeffs;
>>>  
>>>      av_assert2(s->block_last_index[n]>=0 || s->h263_aic);
>>>  
>>> -    qmul = qscale << 1;
>>> -
>>>      if (!s->h263_aic) {
>>>          block[0] *= n < 4 ? s->y_dc_scale : s->c_dc_scale;
>>>          qadd = (qscale - 1) | 1;
>>> @@ -219,43 +217,20 @@ static void dct_unquantize_h263_intra_c(MpegEncContext *s,
>>>      else
>>>          nCoeffs= s->intra_scantable.raster_end[ s->block_last_index[n] ];
>>>  
>>> -    for(i=1; i<=nCoeffs; i++) {
>>> -        level = block[i];
>>> -        if (level) {
>>> -            if (level < 0) {
>>> -                level = level * qmul - qadd;
>>> -            } else {
>>> -                level = level * qmul + qadd;
>>> -            }
>>> -            block[i] = level;
>>> -        }
>>> -    }
>>> +    s->h263dsp.h263_dct_unquantize_intra(block, nCoeffs, qmul, qadd);
>>>  }
>>>  
>>>  static void dct_unquantize_h263_inter_c(MpegEncContext *s,
>>>                                    int16_t *block, int n, int qscale)
>>>  {
>>> -    int i, level, qmul, qadd;
>>> +    int qmul = qscale << 1;
>>> +    int qadd = (qscale - 1) | 1;
>>>      int nCoeffs;
>>>  
>>>      av_assert2(s->block_last_index[n]>=0);
>>>  
>>> -    qadd = (qscale - 1) | 1;
>>> -    qmul = qscale << 1;
>>> -
>>>      nCoeffs= s->inter_scantable.raster_end[ s->block_last_index[n] ];
>>> -
>>> -    for(i=0; i<=nCoeffs; i++) {
>>> -        level = block[i];
>>> -        if (level) {
>>> -            if (level < 0) {
>>> -                level = level * qmul - qadd;
>>> -            } else {
>>> -                level = level * qmul + qadd;
>>> -            }
>>> -            block[i] = level;
>>> -        }
>>> -    }
>>> +    s->h263dsp.h263_dct_unquantize_inter(block, nCoeffs, qmul, qadd);
>>
>> What is the cost of these function calls?
> 
> The same as any other indirect function tail call. What do you want me to say?! Changing how FFmpeg handles assembler optimisations, or removing the upper level of indirection is outside the scope of this MR (and not very realistic).
> 
> For that matter, there are hundreds of indirect calls that could be optimised out on some platforms, notably almost all Armv8 NEON functions and, on x86-64, all MMX or SSE2 exclusives. I don't get nitpicking this one.

1. Given that we always build the C versions of dsp functions, the
latter claim is wrong. (It is only the functions without assembly
versions (for a given platform) for which indirect calls could be avoided.)
2. I am not specifically asking about the cost of the indirect call, but
of the call. The callee will only be called from one place in this
scenario (presuming (for the C versions) that the compiler inlines
h263_dct_unquantize_inter_c into h263_dct_unquantize_intra_c), so if it
were not for arch-specific optimized versions we would inline it in its
caller (just as it is now). The loop does not look like it benefits a
lot from vectorizing, so I am really wondering whether this optimization
is even a win when benchmarking the real thing (not only the loop).

- Andreas

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

Re: [FFmpeg-devel] [PATCHv4 2/4] lavc/mpegvideo: use H263DSP dequant function

[Rémi Denis-Courmont]

Le tiistaina 11. kesäkuuta 2024, 16.00.28 EEST Andreas Rheinhardt a écrit :
> Rémi Denis-Courmont:
> > The same as any other indirect function tail call. What do you want me to
> > say?! Changing how FFmpeg handles assembler optimisations, or removing
> > the upper level of indirection is outside the scope of this MR (and not
> > very realistic).
> > 
> > For that matter, there are hundreds of indirect calls that could be
> > optimised out on some platforms, notably almost all Armv8 NEON functions
> > and, on x86-64, all MMX or SSE2 exclusives. I don't get nitpicking this
> > one.

> 1. Given that we always build the C versions of dsp functions, the
> latter claim is wrong.

Of course not. The C function are being compiled in pointlessly (leaving aside 
checkasm), taking up code size and adding unnecessary indirection. Of course 
assembler functions cannot be inlined, but they could very well use *direct* 
and trivially branch-predicted calls.

> 2. I am not specifically asking about the cost of the indirect call, but
> of the call. The callee will only be called from one place in this
> scenario (presuming (for the C versions) that the compiler inlines
> h263_dct_unquantize_inter_c into h263_dct_unquantize_intra_c), so if it
> were not for arch-specific optimized versions we would inline it in its
> caller (just as it is now).

The outer function itself is address-taken and therefore cannot be inlined. A 
a consequence, the block argument is to be in a register on entry anyway. The 
qmul and qadd are very obviously going to go into registers too either way 
since the compiler cannot assume what their value is (at best, it could make a 
special case for qadd=0). And obviously the counter is going to be in a 
register too. So the overhead is really the function call per se.

And so I don't get what you are asking for.

> The loop does not look like it benefits a
> lot from vectorizing, so I am really wondering whether this optimization
> is even a win when benchmarking the real thing (not only the loop).

There are existing optimisations for x86, AArch64, Arm (2x), MIPS (2x), Alpha 
and PowerPC. They all use inline assembler or compiler intrinsics to be able 
to replicate the non-trivial scalar prologue in C. I can replicate those anti-
patterns for RISC-V, and give up on the checkasm tests.

Needless to say that that seems way worse as far as code quality is concerned.

-- 
Rémi Denis-Courmont
http://www.remlab.net/



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

Re: [FFmpeg-devel] [PATCHv4 1/4] lavc/h263dsp: add DCT dequantisation functions

[Rémi Denis-Courmont]

Le tiistaina 11. kesäkuuta 2024, 13.30.28 EEST Andreas Rheinhardt a écrit :
> > +static void h263_dct_unquantize_intra_c(int16_t *block, size_t nCoeffs,
> > +                                        int qmul, int qadd)
> > +{
> > +    if (nCoeffs > 0)
> 
> Great, a branch.

Okay so you want sarcasms, have at it.

If you had ever looked at optimised compilations, you would have noticed that 
optimising compiler emits that branch implicitly in scenarii like the current 
code to deal with that case, thus rewriting the code as

if (nCoeffs > 0) {
    do { ...; i++ } while (i < nCoeffs);
}

This patch is simply making the branch explicit so we can share the otherwise 
identical C code.

> Why don't you just use a signed type?

Just so you know, the correct type for positive sizes is size_t.
You know, a size, like *not* a stride.

-- 
レミ・デニ-クールモン
http://www.remlab.net/



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

Re: [FFmpeg-devel] [PATCHv4 1/4] lavc/h263dsp: add DCT dequantisation functions

[Andreas Rheinhardt]

Rémi Denis-Courmont:
> Le tiistaina 11. kesäkuuta 2024, 13.30.28 EEST Andreas Rheinhardt a écrit :
>>> +static void h263_dct_unquantize_intra_c(int16_t *block, size_t nCoeffs,
>>> +                                        int qmul, int qadd)
>>> +{
>>> +    if (nCoeffs > 0)
>>
>> Great, a branch.
> 
> Okay so you want sarcasms, have at it.
> 
> If you had ever looked at optimised compilations, you would have noticed that 
> optimising compiler emits that branch implicitly in scenarii like the current 
> code to deal with that case, thus rewriting the code as
> 
> if (nCoeffs > 0) {
>     do { ...; i++ } while (i < nCoeffs);
> }
> 
> This patch is simply making the branch explicit so we can share the otherwise 
> identical C code.

With the current code the initial branch can be avoided in the ac_pred case.

> 
>> Why don't you just use a signed type?
> 
> Just so you know, the correct type for positive sizes is size_t.
> You know, a size, like *not* a stride.
> 

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

Re: [FFmpeg-devel] [PATCHv4 1/4] lavc/h263dsp: add DCT dequantisation functions

[James Almer]

On 6/10/2024 5:23 PM, Rémi Denis-Courmont wrote:
> Note that optimised implementations of these functions will be taken
> into actual use only if MpegEncContext.dct_unquantize_h263_{inter,intra}
> are *not* overloaded by existing optimisations.
> ---
>   libavcodec/h263dsp.c | 25 +++++++++++++++++++++++++
>   libavcodec/h263dsp.h |  4 ++++
>   2 files changed, 29 insertions(+)
> 
> diff --git a/libavcodec/h263dsp.c b/libavcodec/h263dsp.c
> index 6a13353499..755d7077bf 100644
> --- a/libavcodec/h263dsp.c
> +++ b/libavcodec/h263dsp.c
> @@ -23,6 +23,29 @@
>   #include "config.h"
>   #include "h263dsp.h"
>   
> +static void h263_dct_unquantize_inter_c(int16_t *block, size_t nCoeffs,
> +                                        int qmul, int qadd)
> +{
> +    for (size_t i = 0; i <= nCoeffs; i++) {
> +        int level = block[i];
> +
> +        if (level) {
> +            if (level < 0)
> +                level = level * qmul - qadd;
> +            else
> +                level = level * qmul + qadd;
> +            block[i] = level;
> +        }
> +    }
> +}
> +
> +static void h263_dct_unquantize_intra_c(int16_t *block, size_t nCoeffs,
> +                                        int qmul, int qadd)
> +{
> +    if (nCoeffs > 0)
> +        h263_dct_unquantize_inter_c(block + 1, nCoeffs - 1, qmul, qadd);

I think you can avoid this branch by doing:

     for (size_t i = 1; i <= nCoeffs; i++) {
         int level = block[i];

         if (level) {
             if (level < 0)
                 level = level * qmul - qadd;
             else
                 level = level * qmul + qadd;
             block[i] = level;
         }
     }

> +}
> +
>   const uint8_t ff_h263_loop_filter_strength[32] = {
>       0, 1, 1, 2, 2, 3, 3,  4,  4,  4,  5,  5,  6,  6,  7, 7,
>       7, 8, 8, 8, 9, 9, 9, 10, 10, 10, 11, 11, 11, 12, 12, 12
> @@ -116,6 +139,8 @@ static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale)
>   
>   av_cold void ff_h263dsp_init(H263DSPContext *ctx)
>   {
> +    ctx->h263_dct_unquantize_intra = h263_dct_unquantize_intra_c;
> +    ctx->h263_dct_unquantize_inter = h263_dct_unquantize_inter_c;
>       ctx->h263_h_loop_filter = h263_h_loop_filter_c;
>       ctx->h263_v_loop_filter = h263_v_loop_filter_c;
>   
> diff --git a/libavcodec/h263dsp.h b/libavcodec/h263dsp.h
> index 2dccd23392..0ecbe83314 100644
> --- a/libavcodec/h263dsp.h
> +++ b/libavcodec/h263dsp.h
> @@ -24,6 +24,10 @@
>   extern const uint8_t ff_h263_loop_filter_strength[32];
>   
>   typedef struct H263DSPContext {
> +    void (*h263_dct_unquantize_intra)(int16_t *block /* align 16 */,
> +                                      size_t len, int mul, int add);
> +    void (*h263_dct_unquantize_inter)(int16_t *block /* align 16 */,
> +                                      size_t len, int mul, int add);
>       void (*h263_h_loop_filter)(uint8_t *src, int stride, int qscale);
>       void (*h263_v_loop_filter)(uint8_t *src, int stride, int qscale);
>   } H263DSPContext;
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCHv4 3/4] checkasm/h263dsp: test dct_unquantize_{intra, inter}

[James Almer]

On 6/10/2024 5:23 PM, Rémi Denis-Courmont wrote:
> ---
>   tests/checkasm/h263dsp.c | 53 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/checkasm/h263dsp.c b/tests/checkasm/h263dsp.c
> index 2d0957a90b..a8530403ee 100644
> --- a/tests/checkasm/h263dsp.c
> +++ b/tests/checkasm/h263dsp.c
> @@ -18,13 +18,61 @@
>    * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>    */
>   
> +#include <stdbool.h>
>   #include <string.h>
>   
>   #include "checkasm.h"
>   
> -#include "libavcodec/h263dsp.h"
> +#include "libavutil/avassert.h"
>   #include "libavutil/mem.h"
>   #include "libavutil/mem_internal.h"
> +#include "libavcodec/h263dsp.h"
> +#include "libavcodec/mpegvideodata.h"
> +
> +static uint_fast8_t mpeg_qscale_rnd(void)
> +{
> +    int n = rnd(), q = (n >> 1) & 31;
> +
> +    if (n & 1)
> +        return ff_mpeg2_non_linear_qscale[q];
> +    else
> +        return q << 1;
> +}
> +
> +typedef void (*unquantizer)(int16_t *, size_t, int, int);
> +
> +static void check_dct_unquantize(unquantizer func, const char *name)
> +{
> +#define LEN 64
> +    LOCAL_ALIGNED_16(int16_t, block0, [LEN]);
> +    LOCAL_ALIGNED_16(int16_t, block1, [LEN]);
> +    size_t coeffs = rnd() % LEN;
> +    const int qscale = mpeg_qscale_rnd();
> +    const int qmul = qscale << 1;
> +    const int qadd = (rnd() & 1) ? (qscale - 1) | 1 : 0;
> +
> +    declare_func(void, int16_t *, size_t, int, int);
> +
> +    for (size_t i = 0; i < LEN; i++)
> +        block1[i] = block0[i] = (rnd() & 1) ? rnd() : 0;

Don't waste rng bits.

for (size_t i = 0; i < LEN; i++) {
     int n = rnd();
     block1[i] = block0[i] = (n & 1) ? n : 0;
}

> +
> +    if (check_func(func, "h263dsp.dct_unquantize_%s", name)) {
> +        call_ref(block0, 0, qmul, qadd);
> +        call_new(block1, 0, qmul, qadd);
> +
> +        if (block0[0] != block1[0])
> +            fail();
> +
> +        av_assert0(coeffs < LEN);
> +        call_ref(block0, coeffs, qmul, qadd);
> +        call_new(block1, coeffs, qmul, qadd);
> +
> +        if (memcmp(block0, block1, (coeffs + 1) * sizeof (int16_t)))
> +            fail();
> +
> +        bench_new(block1, LEN - 1, qmul, qadd);
> +    }
> +}
>   
>   typedef void (*filter)(uint8_t *src, int stride, int qscale);
>   
> @@ -56,6 +104,9 @@ void checkasm_check_h263dsp(void)
>       H263DSPContext ctx;
>   
>       ff_h263dsp_init(&ctx);
> +    check_dct_unquantize(ctx.h263_dct_unquantize_intra, "intra");
> +    check_dct_unquantize(ctx.h263_dct_unquantize_inter, "inter");
> +    report("dct_unquantize");
>       check_loop_filter('h', ctx.h263_h_loop_filter);
>       check_loop_filter('v', ctx.h263_v_loop_filter);
>       report("loop_filter");
_______________________________________________
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".