diff mbox series

[FFmpeg-devel] avcodec/lagarith: use VLC for probe code length

Message ID CAPYw7P7CHxNtBN6vUYhd-KVw5Um=77zo9qV-45Uev2s9gx7dhA@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/lagarith: use VLC for probe code length | expand

Checks

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

Commit Message

Paul B Mahol Sept. 13, 2023, 9:19 p.m. UTC 
Patch Attached.

Comments

Andreas Rheinhardt Sept. 14, 2023, 1:28 a.m. UTC | #1 
Paul B Mahol:
> 
> +#include "libavutil/mem_internal.h"

I don't get what this header is needed for. You are not adding anything
ALIGNED and this file does not require it.

> +#define VLC_BITS 11
> +
>  enum LagarithFrameType {
>      FRAME_RAW           = 1,    /**< uncompressed */
>      FRAME_U_RGB24       = 2,    /**< unaligned RGB24 */
> @@ -56,6 +61,35 @@ typedef struct LagarithContext {
>      int zeros_rem;              /**< number of zero bytes remaining to output */
>  } LagarithContext;
>  
> +static VLC lag_tab;
> +
> +static const uint8_t lag_bits[] = {
> +    7, 7, 7, 2, 7, 3, 4, 5, 6, 7, 7, 7, 7, 7, 6, 7, 4, 5, 7, 7, 7, 7,
> +    5, 6, 7, 7, 7, 7, 7, 7, 6, 7, 7, 7, 7, 7, 6, 7, 7, 7, 7, 7, 7, 7,
> +    7, 7, 7, 7, 7, 7, 7, 7, 7, 7,
> +};
> +
> +static const uint8_t lag_codes[] = {
> +    0x00, 0x01, 0x02, 0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x04, 0x05,
> +    0x08, 0x09, 0x0A, 0x0B, 0x0B, 0x0B, 0x0B, 0x10, 0x11, 0x12, 0x13,
> +    0x13, 0x13, 0x14, 0x15, 0x20, 0x21, 0x22, 0x23, 0x23, 0x24, 0x25,
> +    0x28, 0x29, 0x2A, 0x2B, 0x2B, 0x40, 0x41, 0x42, 0x43, 0x44, 0x45,
> +    0x48, 0x49, 0x4A, 0x4B, 0x50, 0x51, 0x52, 0x53, 0x54, 0x55,
> +};
> +
> +static const uint8_t lag_symbols[] = {
> +    -1, 20, 12, 0, 12, 1, 2, 4, 7, 7, 28, 4, 25, 17,
> +    10, 17, 3, 6, 2, 23, 15, 15, 5, 9, 10, 31, 1, 22,
> +    14, 14, 8, 9, 30, 6, 27, 19, 11, 19, 0, 21, 13, 13,
> +    8, 29, 5, 26, 18, 18, 3, 24, 16, 16, 11, 32,
> +};
> +
> +static av_cold void lag_init_static_data(void)
> +{
> +    VLC_INIT_SPARSE_STATIC(&lag_tab, VLC_BITS, FF_ARRAY_ELEMS(lag_bits),
> +                           lag_bits, 1, 1, lag_codes, 1, 1, lag_symbols, 1, 1, 2048);
> +}
> +

If the longest code has seven bits, why are you using 11 bits for the
VLC? This just wastes cache/memory.

Apart from that:
Your first entry will be converted to an uint8_t of 255 (and give a
-Woverflow warning when said warning is enabled) and this is what
get_vlc2() will return for it, i.e. it will trigger the bits > 31 check
and error out, which is probably what you intend it to do given that
this behaviour coincides with the current behaviour.

But the more natural way for VLCs to achieve this is to actually not add
invalid codes. get_vlc2() will then return -1 for them; this means that
the check for invalid values becomes "bits < 0" (in which case the flags
from this comparison can be reused for the "bits == 0" check).
In contrast to the current code and your proposed patch no bits would be
consumed upon encountering such an invalid code though. But it seems to
me that the we error out anyway and the state of the GetBitContext
afterwards doesn't matter.

If you were to use the init_from_lengths variants, you would have to use
negative bitlengths for the invalid values.

- Andreas
Paul B Mahol Sept. 14, 2023, 6:57 a.m. UTC | #2 
New patch attached.
Andreas Rheinhardt Sept. 14, 2023, 1:35 p.m. UTC | #3 
Paul B Mahol:
> +    unsigned val, bits;
> +
> +    bits = get_vlc2(gb, lag_tab.table, VLC_BITS, 3);
> +    if (bits > 31) {
>          *value = 0;
>          return AVERROR_INVALIDDATA;

Using a signed int and checking for zero is simpler.
And the depth of this VLC is 1, not 3.

- Andreas
Paul B Mahol Sept. 14, 2023, 1:35 p.m. UTC | #4 
On Thu, Sep 14, 2023 at 3:34 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Paul B Mahol:
> > +    unsigned val, bits;
> > +
> > +    bits = get_vlc2(gb, lag_tab.table, VLC_BITS, 3);
> > +    if (bits > 31) {
> >          *value = 0;
> >          return AVERROR_INVALIDDATA;
>
> Using a signed int and checking for zero is simpler.
> And the depth of this VLC is 1, not 3.
>

There is 32 symbol value bit length, that current code ignores too.


>
> - 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".
>
Paul B Mahol Sept. 16, 2023, 12:21 p.m. UTC | #5 
Will apply soon.
diff mbox series

Patch

From cd774229dcf32cb44455031663b4357f0597b4c0 Mon Sep 17 00:00:00 2001
From: Paul B Mahol <onemda@gmail.com>
Date: Tue, 12 Sep 2023 01:29:55 +0200
Subject: [PATCH] avcodec/lagarith: use VLC for prob code length

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavcodec/lagarith.c | 57 ++++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/libavcodec/lagarith.c b/libavcodec/lagarith.c
index ebc1f7613a..31cad907c0 100644
--- a/libavcodec/lagarith.c
+++ b/libavcodec/lagarith.c
@@ -27,6 +27,9 @@ 
 
 #include <inttypes.h>
 
+#include "libavutil/mem_internal.h"
+#include "libavutil/thread.h"
+
 #include "avcodec.h"
 #include "codec_internal.h"
 #include "get_bits.h"
@@ -35,6 +38,8 @@ 
 #include "lossless_videodsp.h"
 #include "thread.h"
 
+#define VLC_BITS 11
+
 enum LagarithFrameType {
     FRAME_RAW           = 1,    /**< uncompressed */
     FRAME_U_RGB24       = 2,    /**< unaligned RGB24 */
@@ -56,6 +61,35 @@  typedef struct LagarithContext {
     int zeros_rem;              /**< number of zero bytes remaining to output */
 } LagarithContext;
 
+static VLC lag_tab;
+
+static const uint8_t lag_bits[] = {
+    7, 7, 7, 2, 7, 3, 4, 5, 6, 7, 7, 7, 7, 7, 6, 7, 4, 5, 7, 7, 7, 7,
+    5, 6, 7, 7, 7, 7, 7, 7, 6, 7, 7, 7, 7, 7, 6, 7, 7, 7, 7, 7, 7, 7,
+    7, 7, 7, 7, 7, 7, 7, 7, 7, 7,
+};
+
+static const uint8_t lag_codes[] = {
+    0x00, 0x01, 0x02, 0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x04, 0x05,
+    0x08, 0x09, 0x0A, 0x0B, 0x0B, 0x0B, 0x0B, 0x10, 0x11, 0x12, 0x13,
+    0x13, 0x13, 0x14, 0x15, 0x20, 0x21, 0x22, 0x23, 0x23, 0x24, 0x25,
+    0x28, 0x29, 0x2A, 0x2B, 0x2B, 0x40, 0x41, 0x42, 0x43, 0x44, 0x45,
+    0x48, 0x49, 0x4A, 0x4B, 0x50, 0x51, 0x52, 0x53, 0x54, 0x55,
+};
+
+static const uint8_t lag_symbols[] = {
+    -1, 20, 12, 0, 12, 1, 2, 4, 7, 7, 28, 4, 25, 17,
+    10, 17, 3, 6, 2, 23, 15, 15, 5, 9, 10, 31, 1, 22,
+    14, 14, 8, 9, 30, 6, 27, 19, 11, 19, 0, 21, 13, 13,
+    8, 29, 5, 26, 18, 18, 3, 24, 16, 16, 11, 32,
+};
+
+static av_cold void lag_init_static_data(void)
+{
+    VLC_INIT_SPARSE_STATIC(&lag_tab, VLC_BITS, FF_ARRAY_ELEMS(lag_bits),
+                           lag_bits, 1, 1, lag_codes, 1, 1, lag_symbols, 1, 1, 2048);
+}
+
 /**
  * Compute the 52-bit mantissa of 1/(double)denom.
  * This crazy format uses floats in an entropy coder and we have to match x86
@@ -101,23 +135,10 @@  static uint8_t lag_calc_zero_run(int8_t x)
 
 static int lag_decode_prob(GetBitContext *gb, uint32_t *value)
 {
-    static const uint8_t series[] = { 1, 2, 3, 5, 8, 13, 21 };
-    int i;
-    int bit     = 0;
-    int bits    = 0;
-    int prevbit = 0;
-    unsigned val;
-
-    for (i = 0; i < 7; i++) {
-        if (prevbit && bit)
-            break;
-        prevbit = bit;
-        bit = get_bits1(gb);
-        if (bit && !prevbit)
-            bits += series[i];
-    }
-    bits--;
-    if (bits < 0 || bits > 31) {
+    unsigned val, bits;
+
+    bits = get_vlc2(gb, lag_tab.table, VLC_BITS, 3);
+    if (bits > 31) {
         *value = 0;
         return AVERROR_INVALIDDATA;
     } else if (bits == 0) {
@@ -720,10 +741,12 @@  static int lag_decode_frame(AVCodecContext *avctx, AVFrame *p,
 
 static av_cold int lag_decode_init(AVCodecContext *avctx)
 {
+    static AVOnce init_static_once = AV_ONCE_INIT;
     LagarithContext *l = avctx->priv_data;
     l->avctx = avctx;
 
     ff_llviddsp_init(&l->llviddsp);
+    ff_thread_once(&init_static_once, lag_init_static_data);
 
     return 0;
 }
-- 
2.39.1