diff mbox series

[FFmpeg-devel] cbs: Make tracing more general

Message ID 4f4ceb4d-740c-c4ad-fd00-f1b21338d553@jkqxz.net
State New
Headers show
Series [FFmpeg-devel] cbs: Make tracing more general | expand

Checks

Context Check Description
yinshiyou/configure_loongarch64 warning Failed to apply patch
andriy/configure_x86 warning Failed to apply patch

Commit Message

Mark Thompson Aug. 13, 2023, 9:01 p.m. UTC
Turn tracing into callbacks for each syntax element, with a default
callback to match trace_headers for debug.  Move the construction of
bit strings outside CBS, which simplifies all of the read and write
functions.
---
Prompted by the AV1 VAAPI encode patch which reimplements a load of AV1 header writing code in order to get some position information out.

This would replace all of that header writing code with a single callback which matches the desired syntax elements and records the position.

The default callbacks generate identical output to the old tracing, and it's useful to keep them in the CBS core rather than trace headers because turning them on for debugging in either direction was always a nice feature.

Not well-tested yet.  Maybe a fate test which uses tracing to verify some random syntax elements in a stream?

Linecount-negative despite adding functionality because all of the separate trace string constructions are centralised in one place.

I'm not attached to the precise setup - in particular, the write trace feels rather clumsy.  Ideas welcome.

Thanks,

- Mark


Also fixes a bug where AV1 uvlc write wasn't checking whether there was space to write, which was probably always harmless because it can't be the last element in a unit.

  libavcodec/cbs.c               | 113 +++++++++---------
  libavcodec/cbs.h               |  80 +++++++++++++
  libavcodec/cbs_av1.c           | 206 +++++++++------------------------
  libavcodec/cbs_h2645.c         | 128 +++++++-------------
  libavcodec/cbs_internal.h      |  85 +++++++++++++-
  libavcodec/cbs_vp9.c           | 108 +++++------------
  libavcodec/trace_headers_bsf.c |   1 +
  7 files changed, 341 insertions(+), 380 deletions(-)
diff mbox series

Patch

diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
index 3ec8285e21..e1f522c50d 100644
--- a/libavcodec/cbs.c
+++ b/libavcodec/cbs.c
@@ -496,19 +496,26 @@  void ff_cbs_trace_header(CodedBitstreamContext *ctx,
      av_log(ctx->log_ctx, ctx->trace_level, "%s\n", name);
  }

-void ff_cbs_trace_syntax_element(CodedBitstreamContext *ctx, int position,
-                                 const char *str, const int *subscripts,
-                                 const char *bits, int64_t value)
+void ff_cbs_trace_read_log(CodedBitstreamContext *ctx,
+                           GetBitContext *gbc, int length,
+                           const char *str, const int *subscripts,
+                           int64_t value)
  {
      char name[256];
+    char bits[256];
      size_t name_len, bits_len;
      int pad, subs, i, j, k, n;
-
-    if (!ctx->trace_enable)
-        return;
+    int position;

      av_assert0(value >= INT_MIN && value <= UINT32_MAX);

+    position = get_bits_count(gbc);
+
+    av_assert0(length < 256);
+    for (i = 0; i < length; i++)
+        bits[i] = get_bits1(gbc) ? '1' : '0';
+    bits[length] = 0;
+
      subs = subscripts ? subscripts[0] : 0;
      n = 0;
      for (i = j = 0; str[i];) {
@@ -535,7 +542,7 @@  void ff_cbs_trace_syntax_element(CodedBitstreamContext *ctx, int position,
      av_assert0(n == subs);

      name_len = strlen(name);
-    bits_len = strlen(bits);
+    bits_len = length;

      if (name_len + bits_len > 60)
          pad = bits_len + 2;
@@ -546,6 +553,34 @@  void ff_cbs_trace_syntax_element(CodedBitstreamContext *ctx, int position,
             position, name, pad, bits, value);
  }

+void ff_cbs_trace_write_log(CodedBitstreamContext *ctx,
+                            PutBitContext *pbc, int length,
+                            const char *str, const int *subscripts,
+                            int64_t value)
+{
+    // Ensure that the syntax element is written to the output buffer,
+    // make a GetBitContext pointed at the start position, then call the
+    // read log function which can read the bits back to log them.
+
+    GetBitContext gbc;
+    int position;
+
+    if (length > 0) {
+        PutBitContext flush;
+        flush = *pbc;
+        flush_put_bits(&flush);
+    }
+
+    position = put_bits_count(pbc);
+    av_assert0(position >= length);
+
+    init_get_bits(&gbc, pbc->buf, position);
+
+    skip_bits_long(&gbc, position - length);
+
+    ff_cbs_trace_read_log(ctx, &gbc, length, str, subscripts, value);
+}
+
  static av_always_inline int cbs_read_unsigned(CodedBitstreamContext *ctx,
                                                GetBitContext *gbc,
                                                int width, const char *name,
@@ -555,7 +590,8 @@  static av_always_inline int cbs_read_unsigned(CodedBitstreamContext *ctx,
                                                uint32_t range_max)
  {
      uint32_t value;
-    int position;
+
+    CBS_TRACE_READ_START();

      av_assert0(width > 0 && width <= 32);

@@ -565,21 +601,9 @@  static av_always_inline int cbs_read_unsigned(CodedBitstreamContext *ctx,
          return AVERROR_INVALIDDATA;
      }

-    if (ctx->trace_enable)
-        position = get_bits_count(gbc);
-
      value = get_bits_long(gbc, width);

-    if (ctx->trace_enable) {
-        char bits[33];
-        int i;
-        for (i = 0; i < width; i++)
-            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
-        bits[i] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, position, name, subscripts,
-                                    bits, value);
-    }
+    CBS_TRACE_READ_END();

      if (value < range_min || value > range_max) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
@@ -613,6 +637,8 @@  int ff_cbs_write_unsigned(CodedBitstreamContext *ctx, PutBitContext *pbc,
                            const int *subscripts, uint32_t value,
                            uint32_t range_min, uint32_t range_max)
  {
+    CBS_TRACE_WRITE_START();
+
      av_assert0(width > 0 && width <= 32);

      if (value < range_min || value > range_max) {
@@ -625,22 +651,13 @@  int ff_cbs_write_unsigned(CodedBitstreamContext *ctx, PutBitContext *pbc,
      if (put_bits_left(pbc) < width)
          return AVERROR(ENOSPC);

-    if (ctx->trace_enable) {
-        char bits[33];
-        int i;
-        for (i = 0; i < width; i++)
-            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
-        bits[i] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
-                                    name, subscripts, bits, value);
-    }
-
      if (width < 32)
          put_bits(pbc, width, value);
      else
          put_bits32(pbc, value);

+    CBS_TRACE_WRITE_END();
+
      return 0;
  }

@@ -657,7 +674,8 @@  int ff_cbs_read_signed(CodedBitstreamContext *ctx, GetBitContext *gbc,
                         int32_t range_min, int32_t range_max)
  {
      int32_t value;
-    int position;
+
+    CBS_TRACE_READ_START();

      av_assert0(width > 0 && width <= 32);

@@ -667,21 +685,9 @@  int ff_cbs_read_signed(CodedBitstreamContext *ctx, GetBitContext *gbc,
          return AVERROR_INVALIDDATA;
      }

-    if (ctx->trace_enable)
-        position = get_bits_count(gbc);
-
      value = get_sbits_long(gbc, width);

-    if (ctx->trace_enable) {
-        char bits[33];
-        int i;
-        for (i = 0; i < width; i++)
-            bits[i] = value & (1U << (width - i - 1)) ? '1' : '0';
-        bits[i] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, position, name, subscripts,
-                                    bits, value);
-    }
+    CBS_TRACE_READ_END();

      if (value < range_min || value > range_max) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
@@ -699,6 +705,8 @@  int ff_cbs_write_signed(CodedBitstreamContext *ctx, PutBitContext *pbc,
                          const int *subscripts, int32_t value,
                          int32_t range_min, int32_t range_max)
  {
+    CBS_TRACE_WRITE_START();
+
      av_assert0(width > 0 && width <= 32);

      if (value < range_min || value > range_max) {
@@ -711,22 +719,13 @@  int ff_cbs_write_signed(CodedBitstreamContext *ctx, PutBitContext *pbc,
      if (put_bits_left(pbc) < width)
          return AVERROR(ENOSPC);

-    if (ctx->trace_enable) {
-        char bits[33];
-        int i;
-        for (i = 0; i < width; i++)
-            bits[i] = value & (1U << (width - i - 1)) ? '1' : '0';
-        bits[i] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
-                                    name, subscripts, bits, value);
-    }
-
      if (width < 32)
          put_sbits(pbc, width, value);
      else
          put_bits32(pbc, value);

+    CBS_TRACE_WRITE_END();
+
      return 0;
  }

diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
index b4131db5fe..ad3cb78793 100644
--- a/libavcodec/cbs.h
+++ b/libavcodec/cbs.h
@@ -168,6 +168,51 @@  typedef struct CodedBitstreamFragment {
      CodedBitstreamUnit *units;
  } CodedBitstreamFragment;

+
+struct CodedBitstreamContext;
+struct GetBitContext;
+struct PutBitContext;
+
+/**
+ * Callback type for read tracing.
+ *
+ * @param ctx         CBS context.
+ * @param gbc         A GetBitContext set at the start of the syntax
+ *                    element.  This is a copy, the callee does not
+ *                    need to preserve it.
+ * @param length      Length in bits of the syntax element.
+ * @param name        String name of the syntax elements.
+ * @param subscripts  If the syntax element is an array, a pointer to
+ *                    an array of subscripts into the array.
+ * @param value       Parsed value of the syntax element.
+ */
+typedef void (*CBSTraceReadCallback)(struct CodedBitstreamContext *ctx,
+                                     struct GetBitContext *gbc,
+                                     int start_position,
+                                     const char *name,
+                                     const int *subscripts,
+                                     int64_t value);
+
+/**
+ * Callback type for write tracing.
+ *
+ * @param ctx         CBS context.
+ * @param pbc         A PutBitContext set at the end of the syntax
+ *                    element.  The user must not modify this, but may
+ *                    inspect it to determine state.
+ * @param length      Length in bits of the syntax element.
+ * @param name        String name of the syntax elements.
+ * @param subscripts  If the syntax element is an array, a pointer to
+ *                    an array of subscripts into the array.
+ * @param value       Written value of the syntax element.
+ */
+typedef void (*CBSTraceWriteCallback)(struct CodedBitstreamContext *ctx,
+                                     struct PutBitContext *gbc,
+                                     int start_position,
+                                     const char *name,
+                                     const int *subscripts,
+                                     int64_t value);
+
  /**
   * Context structure for coded bitstream operations.
   */
@@ -217,6 +262,22 @@  typedef struct CodedBitstreamContext {
       */
      int trace_level;

+    /**
+     * Callback for read tracing.
+     *
+     * If tracing is enabled then this is called once for each syntax
+     * element parsed.
+     */
+    CBSTraceReadCallback  trace_read_callback;
+
+    /**
+     * Callback for write tracing.
+     *
+     * If tracing is enabled then this is called once for each syntax
+     * element written.
+     */
+    CBSTraceWriteCallback trace_write_callback;
+
      /**
       * Write buffer. Used as intermediate buffer when writing units.
       * For internal use of cbs only.
@@ -450,4 +511,23 @@  void ff_cbs_discard_units(CodedBitstreamContext *ctx,
                            enum AVDiscard skip,
                            int flags);

+
+/**
+ * Helper function for read tracing which formats the syntax element
+ * and logs the result.
+ */
+void ff_cbs_trace_read_log(CodedBitstreamContext *ctx,
+                           struct GetBitContext *gbc, int length,
+                           const char *str, const int *subscripts,
+                           int64_t value);
+
+/**
+ * Helper function for write tracing which formats the syntax element
+ * and logs the result.
+ */
+void ff_cbs_trace_write_log(CodedBitstreamContext *ctx,
+                            struct PutBitContext *pbc, int length,
+                            const char *str, const int *subscripts,
+                            int64_t value);
+
  #endif /* AVCODEC_CBS_H */
diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
index 452e022b36..aa92639235 100644
--- a/libavcodec/cbs_av1.c
+++ b/libavcodec/cbs_av1.c
@@ -31,10 +31,8 @@  static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, GetBitContext *gbc,
                               uint32_t range_min, uint32_t range_max)
  {
      uint32_t zeroes, bits_value, value;
-    int position;

-    if (ctx->trace_enable)
-        position = get_bits_count(gbc);
+    CBS_TRACE_READ_START();

      zeroes = 0;
      while (1) {
@@ -50,6 +48,9 @@  static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, GetBitContext *gbc,
      }

      if (zeroes >= 32) {
+        // Note that the spec allows an arbitrarily large number of
+        // zero bits followed by a one bit in this case, but the
+        // libaom implementation does not support it.
          value = MAX_UINT_BITS(32);
      } else {
          if (get_bits_left(gbc) < zeroes) {
@@ -62,36 +63,7 @@  static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, GetBitContext *gbc,
          value = bits_value + (UINT32_C(1) << zeroes) - 1;
      }

-    if (ctx->trace_enable) {
-        char bits[65];
-        int i, j, k;
-
-        if (zeroes >= 32) {
-            while (zeroes > 32) {
-                k = FFMIN(zeroes - 32, 32);
-                for (i = 0; i < k; i++)
-                    bits[i] = '0';
-                bits[i] = 0;
-                ff_cbs_trace_syntax_element(ctx, position, name,
-                                            NULL, bits, 0);
-                zeroes -= k;
-                position += k;
-            }
-        }
-
-        for (i = 0; i < zeroes; i++)
-            bits[i] = '0';
-        bits[i++] = '1';
-
-        if (zeroes < 32) {
-            for (j = 0; j < zeroes; j++)
-                bits[i++] = (bits_value >> (zeroes - j - 1) & 1) ? '1' : '0';
-        }
-
-        bits[i] = 0;
-        ff_cbs_trace_syntax_element(ctx, position, name,
-                                    NULL, bits, value);
-    }
+    CBS_TRACE_READ_END_NO_SUBSCRIPTS();

      if (value < range_min || value > range_max) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
@@ -109,7 +81,9 @@  static int cbs_av1_write_uvlc(CodedBitstreamContext *ctx, PutBitContext *pbc,
                                uint32_t range_min, uint32_t range_max)
  {
      uint32_t v;
-    int position, zeroes;
+    int zeroes;
+
+    CBS_TRACE_WRITE_START();

      if (value < range_min || value > range_max) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
@@ -118,28 +92,17 @@  static int cbs_av1_write_uvlc(CodedBitstreamContext *ctx, PutBitContext *pbc,
          return AVERROR_INVALIDDATA;
      }

-    if (ctx->trace_enable)
-        position = put_bits_count(pbc);
-
      zeroes = av_log2(value + 1);
      v = value - (1U << zeroes) + 1;
+
+    if (put_bits_left(pbc) < 2 * zeroes + 1)
+        return AVERROR(ENOSPC);
+
      put_bits(pbc, zeroes, 0);
      put_bits(pbc, 1, 1);
      put_bits(pbc, zeroes, v);

-    if (ctx->trace_enable) {
-        char bits[65];
-        int i, j;
-        i = 0;
-        for (j = 0; j < zeroes; j++)
-            bits[i++] = '0';
-        bits[i++] = '1';
-        for (j = 0; j < zeroes; j++)
-            bits[i++] = (v >> (zeroes - j - 1) & 1) ? '1' : '0';
-        bits[i++] = 0;
-        ff_cbs_trace_syntax_element(ctx, position, name, NULL,
-                                    bits, value);
-    }
+    CBS_TRACE_WRITE_END_NO_SUBSCRIPTS();

      return 0;
  }
@@ -148,20 +111,19 @@  static int cbs_av1_read_leb128(CodedBitstreamContext *ctx, GetBitContext *gbc,
                                 const char *name, uint64_t *write_to)
  {
      uint64_t value;
-    int position, err, i;
+    uint32_t byte;
+    int i;

-    if (ctx->trace_enable)
-        position = get_bits_count(gbc);
+    CBS_TRACE_READ_START();

      value = 0;
      for (i = 0; i < 8; i++) {
-        int subscript[2] = { 1, i };
-        uint32_t byte;
-        err = ff_cbs_read_unsigned(ctx, gbc, 8, "leb128_byte[i]", subscript,
-                                   &byte, 0x00, 0xff);
-        if (err < 0)
-            return err;
-
+        if (get_bits_left(gbc) < 8) {
+            av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid leb128 at "
+                   "%s: bitstream ended.\n", name);
+            return AVERROR_INVALIDDATA;
+        }
+        byte = get_bits(gbc, 8);
          value |= (uint64_t)(byte & 0x7f) << (i * 7);
          if (!(byte & 0x80))
              break;
@@ -170,8 +132,7 @@  static int cbs_av1_read_leb128(CodedBitstreamContext *ctx, GetBitContext *gbc,
      if (value > UINT32_MAX)
          return AVERROR_INVALIDDATA;

-    if (ctx->trace_enable)
-        ff_cbs_trace_syntax_element(ctx, position, name, NULL, "", value);
+    CBS_TRACE_READ_END_NO_SUBSCRIPTS();

      *write_to = value;
      return 0;
@@ -180,29 +141,25 @@  static int cbs_av1_read_leb128(CodedBitstreamContext *ctx, GetBitContext *gbc,
  static int cbs_av1_write_leb128(CodedBitstreamContext *ctx, PutBitContext *pbc,
                                  const char *name, uint64_t value)
  {
-    int position, err, len, i;
+    int len, i;
      uint8_t byte;

-    len = (av_log2(value) + 7) / 7;
+    CBS_TRACE_WRITE_START();

-    if (ctx->trace_enable)
-        position = put_bits_count(pbc);
+    len = (av_log2(value) + 7) / 7;

      for (i = 0; i < len; i++) {
-        int subscript[2] = { 1, i };
+        if (put_bits_left(pbc) < 8)
+            return AVERROR(ENOSPC);

          byte = value >> (7 * i) & 0x7f;
          if (i < len - 1)
              byte |= 0x80;

-        err = ff_cbs_write_unsigned(ctx, pbc, 8, "leb128_byte[i]", subscript,
-                                    byte, 0x00, 0xff);
-        if (err < 0)
-            return err;
+        put_bits(pbc, 8, byte);
      }

-    if (ctx->trace_enable)
-        ff_cbs_trace_syntax_element(ctx, position, name, NULL, "", value);
+    CBS_TRACE_WRITE_END_NO_SUBSCRIPTS();

      return 0;
  }
@@ -212,12 +169,11 @@  static int cbs_av1_read_ns(CodedBitstreamContext *ctx, GetBitContext *gbc,
                             const int *subscripts, uint32_t *write_to)
  {
      uint32_t m, v, extra_bit, value;
-    int position, w;
+    int w;

-    av_assert0(n > 0);
+    CBS_TRACE_READ_START();

-    if (ctx->trace_enable)
-        position = get_bits_count(gbc);
+    av_assert0(n > 0);

      w = av_log2(n) + 1;
      m = (1 << w) - n;
@@ -240,18 +196,7 @@  static int cbs_av1_read_ns(CodedBitstreamContext *ctx, GetBitContext *gbc,
          value = (v << 1) - m + extra_bit;
      }

-    if (ctx->trace_enable) {
-        char bits[33];
-        int i;
-        for (i = 0; i < w - 1; i++)
-            bits[i] = (v >> i & 1) ? '1' : '0';
-        if (v >= m)
-            bits[i++] = extra_bit ? '1' : '0';
-        bits[i] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, position,
-                                    name, subscripts, bits, value);
-    }
+    CBS_TRACE_READ_END();

      *write_to = value;
      return 0;
@@ -262,7 +207,8 @@  static int cbs_av1_write_ns(CodedBitstreamContext *ctx, PutBitContext *pbc,
                              const int *subscripts, uint32_t value)
  {
      uint32_t w, m, v, extra_bit;
-    int position;
+
+    CBS_TRACE_WRITE_START();

      if (value > n) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
@@ -271,9 +217,6 @@  static int cbs_av1_write_ns(CodedBitstreamContext *ctx, PutBitContext *pbc,
          return AVERROR_INVALIDDATA;
      }

-    if (ctx->trace_enable)
-        position = put_bits_count(pbc);
-
      w = av_log2(n) + 1;
      m = (1 << w) - n;

@@ -290,18 +233,7 @@  static int cbs_av1_write_ns(CodedBitstreamContext *ctx, PutBitContext *pbc,
          put_bits(pbc, 1, extra_bit);
      }

-    if (ctx->trace_enable) {
-        char bits[33];
-        int i;
-        for (i = 0; i < w - 1; i++)
-            bits[i] = (v >> i & 1) ? '1' : '0';
-        if (value >= m)
-            bits[i++] = extra_bit ? '1' : '0';
-        bits[i] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, position,
-                                    name, subscripts, bits, value);
-    }
+    CBS_TRACE_WRITE_END();

      return 0;
  }
@@ -311,33 +243,24 @@  static int cbs_av1_read_increment(CodedBitstreamContext *ctx, GetBitContext *gbc
                                    const char *name, uint32_t *write_to)
  {
      uint32_t value;
-    int position, i;
-    char bits[33];

-    av_assert0(range_min <= range_max && range_max - range_min < sizeof(bits) - 1);
-    if (ctx->trace_enable)
-        position = get_bits_count(gbc);
+    CBS_TRACE_READ_START();

-    for (i = 0, value = range_min; value < range_max;) {
+    av_assert0(range_min <= range_max && range_max - range_min < 32);
+
+    for (value = range_min; value < range_max;) {
          if (get_bits_left(gbc) < 1) {
              av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid increment value at "
                     "%s: bitstream ended.\n", name);
              return AVERROR_INVALIDDATA;
          }
-        if (get_bits1(gbc)) {
-            bits[i++] = '1';
+        if (get_bits1(gbc))
              ++value;
-        } else {
-            bits[i++] = '0';
+        else
              break;
-        }
      }

-    if (ctx->trace_enable) {
-        bits[i] = 0;
-        ff_cbs_trace_syntax_element(ctx, position,
-                                    name, NULL, bits, value);
-    }
+    CBS_TRACE_READ_END_NO_SUBSCRIPTS();

      *write_to = value;
      return 0;
@@ -349,6 +272,8 @@  static int cbs_av1_write_increment(CodedBitstreamContext *ctx, PutBitContext *pb
  {
      int len;

+    CBS_TRACE_WRITE_START();
+
      av_assert0(range_min <= range_max && range_max - range_min < 32);
      if (value < range_min || value > range_max) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
@@ -364,23 +289,11 @@  static int cbs_av1_write_increment(CodedBitstreamContext *ctx, PutBitContext *pb
      if (put_bits_left(pbc) < len)
          return AVERROR(ENOSPC);

-    if (ctx->trace_enable) {
-        char bits[33];
-        int i;
-        for (i = 0; i < len; i++) {
-            if (range_min + i == value)
-                bits[i] = '0';
-            else
-                bits[i] = '1';
-        }
-        bits[i] = 0;
-        ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
-                                    name, NULL, bits, value);
-    }
-
      if (len > 0)
          put_bits(pbc, len, (1 << len) - 1 - (value != range_max));

+    CBS_TRACE_WRITE_END_NO_SUBSCRIPTS();
+
      return 0;
  }

@@ -388,12 +301,10 @@  static int cbs_av1_read_subexp(CodedBitstreamContext *ctx, GetBitContext *gbc,
                                 uint32_t range_max, const char *name,
                                 const int *subscripts, uint32_t *write_to)
  {
-    uint32_t value;
-    int position, err;
-    uint32_t max_len, len, range_offset, range_bits;
+    uint32_t value, max_len, len, range_offset, range_bits;
+    int err;

-    if (ctx->trace_enable)
-        position = get_bits_count(gbc);
+    CBS_TRACE_READ_START();

      av_assert0(range_max > 0);
      max_len = av_log2(range_max - 1) - 3;
@@ -425,9 +336,7 @@  static int cbs_av1_read_subexp(CodedBitstreamContext *ctx, GetBitContext *gbc,
      }
      value += range_offset;

-    if (ctx->trace_enable)
-        ff_cbs_trace_syntax_element(ctx, position,
-                                    name, subscripts, "", value);
+    CBS_TRACE_READ_END_VALUE_ONLY();

      *write_to = value;
      return err;
@@ -437,9 +346,11 @@  static int cbs_av1_write_subexp(CodedBitstreamContext *ctx, PutBitContext *pbc,
                                  uint32_t range_max, const char *name,
                                  const int *subscripts, uint32_t value)
  {
-    int position, err;
+    int err;
      uint32_t max_len, len, range_offset, range_bits;

+    CBS_TRACE_WRITE_START();
+
      if (value > range_max) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
                 "%"PRIu32", but must be in [0,%"PRIu32"].\n",
@@ -447,9 +358,6 @@  static int cbs_av1_write_subexp(CodedBitstreamContext *ctx, PutBitContext *pbc,
          return AVERROR_INVALIDDATA;
      }

-    if (ctx->trace_enable)
-        position = put_bits_count(pbc);
-
      av_assert0(range_max > 0);
      max_len = av_log2(range_max - 1) - 3;

@@ -489,9 +397,7 @@  static int cbs_av1_write_subexp(CodedBitstreamContext *ctx, PutBitContext *pbc,
              return err;
      }

-    if (ctx->trace_enable)
-        ff_cbs_trace_syntax_element(ctx, position,
-                                    name, subscripts, "", value);
+    CBS_TRACE_WRITE_END_VALUE_ONLY();

      return err;
  }
diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
index 318c997d94..32f0f1c7e1 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -36,41 +36,32 @@  static int cbs_read_ue_golomb(CodedBitstreamContext *ctx, GetBitContext *gbc,
                                uint32_t *write_to,
                                uint32_t range_min, uint32_t range_max)
  {
-    uint32_t value;
-    int position, i, j;
-    unsigned int k;
-    char bits[65];
+    uint32_t leading_bits, value;
+    int leading_zeroes;

-    position = get_bits_count(gbc);
+    CBS_TRACE_READ_START();

-    for (i = 0; i < 32; i++) {
-        if (get_bits_left(gbc) < i + 1) {
-            av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid ue-golomb code at "
-                   "%s: bitstream ended.\n", name);
-            return AVERROR_INVALIDDATA;
-        }
-        k = get_bits1(gbc);
-        bits[i] = k ? '1' : '0';
-        if (k)
-            break;
-    }
-    if (i >= 32) {
+    leading_bits = show_bits_long(gbc, 32);
+    if (leading_bits == 0) {
+        // This could be the bitstream ending instead, but padding
+        // makes the read ok and we will be caught by the next check.
          av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid ue-golomb code at "
                 "%s: more than 31 zeroes.\n", name);
          return AVERROR_INVALIDDATA;
      }
-    value = 1;
-    for (j = 0; j < i; j++) {
-        k = get_bits1(gbc);
-        bits[i + j + 1] = k ? '1' : '0';
-        value = value << 1 | k;
+
+    leading_zeroes = 31 - av_log2(leading_bits);
+    skip_bits_long(gbc, leading_zeroes);
+
+    if (get_bits_left(gbc) < leading_zeroes + 1) {
+        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid ue-golomb code at "
+               "%s: bitstream ended.\n", name);
+        return AVERROR_INVALIDDATA;
      }
-    bits[i + j + 1] = 0;
-    --value;

-    if (ctx->trace_enable)
-        ff_cbs_trace_syntax_element(ctx, position, name, subscripts,
-                                    bits, value);
+    value = get_bits_long(gbc, leading_zeroes + 1) - 1;
+
+    CBS_TRACE_READ_END();

      if (value < range_min || value > range_max) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
@@ -88,45 +79,30 @@  static int cbs_read_se_golomb(CodedBitstreamContext *ctx, GetBitContext *gbc,
                                int32_t *write_to,
                                int32_t range_min, int32_t range_max)
  {
+    uint32_t leading_bits, unsigned_value;
+    int leading_zeroes;
      int32_t value;
-    int position, i, j;
-    unsigned int k;
-    uint32_t v;
-    char bits[65];

-    position = get_bits_count(gbc);
+    CBS_TRACE_READ_START();

-    for (i = 0; i < 32; i++) {
-        if (get_bits_left(gbc) < i + 1) {
-            av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid se-golomb code at "
-                   "%s: bitstream ended.\n", name);
-            return AVERROR_INVALIDDATA;
-        }
-        k = get_bits1(gbc);
-        bits[i] = k ? '1' : '0';
-        if (k)
-            break;
-    }
-    if (i >= 32) {
+    leading_bits = show_bits_long(gbc, 32);
+    if (leading_bits == 0) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid se-golomb code at "
                 "%s: more than 31 zeroes.\n", name);
          return AVERROR_INVALIDDATA;
      }
-    v = 1;
-    for (j = 0; j < i; j++) {
-        k = get_bits1(gbc);
-        bits[i + j + 1] = k ? '1' : '0';
-        v = v << 1 | k;
-    }
-    bits[i + j + 1] = 0;
-    if (v & 1)
-        value = -(int32_t)(v / 2);
+
+    leading_zeroes = 31 - av_log2(leading_bits);
+    skip_bits_long(gbc, leading_zeroes);
+
+    unsigned_value = get_bits_long(gbc, leading_zeroes + 1);
+
+    if (unsigned_value & 1)
+        value = -(int32_t)(unsigned_value / 2);
      else
-        value = v / 2;
+        value = unsigned_value / 2;

-    if (ctx->trace_enable)
-        ff_cbs_trace_syntax_element(ctx, position, name, subscripts,
-                                    bits, value);
+    CBS_TRACE_READ_END();

      if (value < range_min || value > range_max) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
@@ -146,6 +122,8 @@  static int cbs_write_ue_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
  {
      int len;

+    CBS_TRACE_WRITE_START();
+
      if (value < range_min || value > range_max) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
                 "%"PRIu32", but must be in [%"PRIu32",%"PRIu32"].\n",
@@ -158,27 +136,14 @@  static int cbs_write_ue_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
      if (put_bits_left(pbc) < 2 * len + 1)
          return AVERROR(ENOSPC);

-    if (ctx->trace_enable) {
-        char bits[65];
-        int i;
-
-        for (i = 0; i < len; i++)
-            bits[i] = '0';
-        bits[len] = '1';
-        for (i = 0; i < len; i++)
-            bits[len + i + 1] = (value + 1) >> (len - i - 1) & 1 ? '1' : '0';
-        bits[len + len + 1] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
-                                    name, subscripts, bits, value);
-    }
-
      put_bits(pbc, len, 0);
      if (len + 1 < 32)
          put_bits(pbc, len + 1, value + 1);
      else
          put_bits32(pbc, value + 1);

+    CBS_TRACE_WRITE_END();
+
      return 0;
  }

@@ -190,6 +155,8 @@  static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
      int len;
      uint32_t uvalue;

+    CBS_TRACE_WRITE_START();
+
      if (value < range_min || value > range_max) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
                 "%"PRId32", but must be in [%"PRId32",%"PRId32"].\n",
@@ -209,27 +176,14 @@  static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
      if (put_bits_left(pbc) < 2 * len + 1)
          return AVERROR(ENOSPC);

-    if (ctx->trace_enable) {
-        char bits[65];
-        int i;
-
-        for (i = 0; i < len; i++)
-            bits[i] = '0';
-        bits[len] = '1';
-        for (i = 0; i < len; i++)
-            bits[len + i + 1] = (uvalue + 1) >> (len - i - 1) & 1 ? '1' : '0';
-        bits[len + len + 1] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
-                                    name, subscripts, bits, value);
-    }
-
      put_bits(pbc, len, 0);
      if (len + 1 < 32)
          put_bits(pbc, len + 1, uvalue + 1);
      else
          put_bits32(pbc, uvalue + 1);

+    CBS_TRACE_WRITE_END();
+
      return 0;
  }

diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
index da84697a29..de9295137e 100644
--- a/libavcodec/cbs_internal.h
+++ b/libavcodec/cbs_internal.h
@@ -157,10 +157,6 @@  typedef struct CodedBitstreamType {
  void ff_cbs_trace_header(CodedBitstreamContext *ctx,
                           const char *name);

-void ff_cbs_trace_syntax_element(CodedBitstreamContext *ctx, int position,
-                                 const char *name, const int *subscripts,
-                                 const char *bitstring, int64_t value);
-

  // Helper functions for read/write of common bitstream elements, including
  // generation of trace output. The simple functions are equivalent to
@@ -206,6 +202,87 @@  int ff_cbs_write_signed(CodedBitstreamContext *ctx, PutBitContext *pbc,
  // range_min in the above functions.
  #define MIN_INT_BITS(length) (-(INT64_C(1) << ((length) - 1)))

+
+// Start of a syntax element during read tracing.
+#define CBS_TRACE_READ_START() \
+    GetBitContext trace_start; \
+    do { \
+        if (ctx->trace_enable) \
+            trace_start = *gbc; \
+    } while (0)
+
+// End of a syntax element for tracing, make callback.
+#define CBS_TRACE_READ_END() \
+    do { \
+        if (ctx->trace_enable) { \
+            int start_position = get_bits_count(&trace_start); \
+            int end_position   = get_bits_count(gbc); \
+            av_assert0(start_position <= end_position); \
+            ctx->trace_read_callback(ctx, &trace_start, \
+                                     end_position - start_position, \
+                                     name, subscripts, value); \
+        } \
+    } while (0)
+
+// End of a syntax element with no subscript entries.
+#define CBS_TRACE_READ_END_NO_SUBSCRIPTS() \
+    do { \
+        const int *subscripts = NULL; \
+        CBS_TRACE_READ_END(); \
+    } while (0)
+
+// End of a syntax element which is made up of subelements which
+// are aleady traced, so we are only showing the value.
+#define CBS_TRACE_READ_END_VALUE_ONLY() \
+    do { \
+        if (ctx->trace_enable) { \
+            ctx->trace_read_callback(ctx, &trace_start, 0, \
+                                     name, subscripts, value); \
+        } \
+    } while (0)
+
+// Start of a syntax element during write tracing.
+#define CBS_TRACE_WRITE_START() \
+    int start_position; \
+    do { \
+        if (ctx->trace_enable) \
+            start_position = put_bits_count(pbc);; \
+    } while (0)
+
+// End of a syntax element for tracing, make callback.
+#define CBS_TRACE_WRITE_END() \
+    do { \
+        if (ctx->trace_enable) { \
+            int end_position   = put_bits_count(pbc); \
+            av_assert0(start_position <= end_position); \
+            ctx->trace_write_callback(ctx, pbc, \
+                                      end_position - start_position, \
+                                      name, subscripts, value); \
+        } \
+    } while (0)
+
+// End of a syntax element with no subscript entries.
+#define CBS_TRACE_WRITE_END_NO_SUBSCRIPTS() \
+    do { \
+        const int *subscripts = NULL; \
+        CBS_TRACE_WRITE_END(); \
+    } while (0)
+
+// End of a syntax element which is made up of subelements which are
+// aleady traced, so we are only showing the value.  This forges a
+// PutBitContext to point to the position of the start of the syntax
+// element, but the other state doesn't matter because length is zero.
+#define CBS_TRACE_WRITE_END_VALUE_ONLY() \
+    do { \
+        if (ctx->trace_enable) { \
+            PutBitContext tmp; \
+            init_put_bits(&tmp, pbc->buf, start_position); \
+            skip_put_bits(&tmp, start_position); \
+            ctx->trace_write_callback(ctx, &tmp, 0, \
+                                      name, subscripts, value); \
+        } \
+    } while (0)
+
  #define TYPE_LIST(...) { __VA_ARGS__ }
  #define CBS_UNIT_TYPE_POD(type_, structure) { \
          .nb_unit_types  = 1, \
diff --git a/libavcodec/cbs_vp9.c b/libavcodec/cbs_vp9.c
index b0d5bd8763..816d06da04 100644
--- a/libavcodec/cbs_vp9.c
+++ b/libavcodec/cbs_vp9.c
@@ -28,11 +28,10 @@  static int cbs_vp9_read_s(CodedBitstreamContext *ctx, GetBitContext *gbc,
                            const int *subscripts, int32_t *write_to)
  {
      uint32_t magnitude;
-    int position, sign;
+    int sign;
      int32_t value;

-    if (ctx->trace_enable)
-        position = get_bits_count(gbc);
+    CBS_TRACE_READ_START();

      if (get_bits_left(gbc) < width + 1) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid signed value at "
@@ -44,17 +43,7 @@  static int cbs_vp9_read_s(CodedBitstreamContext *ctx, GetBitContext *gbc,
      sign      = get_bits1(gbc);
      value     = sign ? -(int32_t)magnitude : magnitude;

-    if (ctx->trace_enable) {
-        char bits[33];
-        int i;
-        for (i = 0; i < width; i++)
-            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
-        bits[i] = sign ? '1' : '0';
-        bits[i + 1] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, position, name, subscripts,
-                                    bits, value);
-    }
+    CBS_TRACE_READ_END();

      *write_to = value;
      return 0;
@@ -67,27 +56,19 @@  static int cbs_vp9_write_s(CodedBitstreamContext *ctx, PutBitContext *pbc,
      uint32_t magnitude;
      int sign;

+    CBS_TRACE_WRITE_START();
+
      if (put_bits_left(pbc) < width + 1)
          return AVERROR(ENOSPC);

      sign      = value < 0;
      magnitude = sign ? -value : value;

-    if (ctx->trace_enable) {
-        char bits[33];
-        int i;
-        for (i = 0; i < width; i++)
-            bits[i] = magnitude >> (width - i - 1) & 1 ? '1' : '0';
-        bits[i] = sign ? '1' : '0';
-        bits[i + 1] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
-                                    name, subscripts, bits, value);
-    }
-
      put_bits(pbc, width, magnitude);
      put_bits(pbc, 1, sign);

+    CBS_TRACE_WRITE_END();
+
      return 0;
  }

@@ -96,32 +77,24 @@  static int cbs_vp9_read_increment(CodedBitstreamContext *ctx, GetBitContext *gbc
                                    const char *name, uint32_t *write_to)
  {
      uint32_t value;
-    int position, i;
-    char bits[8];

-    av_assert0(range_min <= range_max && range_max - range_min < sizeof(bits) - 1);
-    if (ctx->trace_enable)
-        position = get_bits_count(gbc);
+    CBS_TRACE_READ_START();

-    for (i = 0, value = range_min; value < range_max;) {
+    av_assert0(range_min <= range_max && range_max - range_min < 32);
+
+    for (value = range_min; value < range_max;) {
          if (get_bits_left(gbc) < 1) {
              av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid increment value at "
                     "%s: bitstream ended.\n", name);
              return AVERROR_INVALIDDATA;
          }
-        if (get_bits1(gbc)) {
-            bits[i++] = '1';
+        if (get_bits1(gbc))
              ++value;
-        } else {
-            bits[i++] = '0';
+        else
              break;
-        }
      }

-    if (ctx->trace_enable) {
-        bits[i] = 0;
-        ff_cbs_trace_syntax_element(ctx, position, name, NULL, bits, value);
-    }
+    CBS_TRACE_READ_END_NO_SUBSCRIPTS();

      *write_to = value;
      return 0;
@@ -133,6 +106,8 @@  static int cbs_vp9_write_increment(CodedBitstreamContext *ctx, PutBitContext *pb
  {
      int len;

+    CBS_TRACE_WRITE_START();
+
      av_assert0(range_min <= range_max && range_max - range_min < 8);
      if (value < range_min || value > range_max) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
@@ -148,23 +123,11 @@  static int cbs_vp9_write_increment(CodedBitstreamContext *ctx, PutBitContext *pb
      if (put_bits_left(pbc) < len)
          return AVERROR(ENOSPC);

-    if (ctx->trace_enable) {
-        char bits[8];
-        int i;
-        for (i = 0; i < len; i++) {
-            if (range_min + i == value)
-                bits[i] = '0';
-            else
-                bits[i] = '1';
-        }
-        bits[i] = 0;
-        ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
-                                    name, NULL, bits, value);
-    }
-
      if (len > 0)
          put_bits(pbc, len, (1 << len) - 1 - (value != range_max));

+    CBS_TRACE_WRITE_END_NO_SUBSCRIPTS();
+
      return 0;
  }

@@ -173,12 +136,11 @@  static int cbs_vp9_read_le(CodedBitstreamContext *ctx, GetBitContext *gbc,
                             const int *subscripts, uint32_t *write_to)
  {
      uint32_t value;
-    int position, b;
+    int b;

-    av_assert0(width % 8 == 0);
+    CBS_TRACE_READ_START();

-    if (ctx->trace_enable)
-        position = get_bits_count(gbc);
+    av_assert0(width % 8 == 0);

      if (get_bits_left(gbc) < width) {
          av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid le value at "
@@ -190,17 +152,7 @@  static int cbs_vp9_read_le(CodedBitstreamContext *ctx, GetBitContext *gbc,
      for (b = 0; b < width; b += 8)
          value |= get_bits(gbc, 8) << b;

-    if (ctx->trace_enable) {
-        char bits[33];
-        int i;
-        for (b = 0; b < width; b += 8)
-            for (i = 0; i < 8; i++)
-                bits[b + i] = value >> (b + i) & 1 ? '1' : '0';
-        bits[b] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, position, name, subscripts,
-                                    bits, value);
-    }
+    CBS_TRACE_READ_END();

      *write_to = value;
      return 0;
@@ -212,26 +164,18 @@  static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
  {
      int b;

+    CBS_TRACE_WRITE_START();
+
      av_assert0(width % 8 == 0);

      if (put_bits_left(pbc) < width)
          return AVERROR(ENOSPC);

-    if (ctx->trace_enable) {
-        char bits[33];
-        int i;
-        for (b = 0; b < width; b += 8)
-            for (i = 0; i < 8; i++)
-                bits[b + i] = value >> (b + i) & 1 ? '1' : '0';
-        bits[b] = 0;
-
-        ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc),
-                                    name, subscripts, bits, value);
-    }
-
      for (b = 0; b < width; b += 8)
          put_bits(pbc, 8, value >> b & 0xff);

+    CBS_TRACE_WRITE_END();
+
      return 0;
  }

diff --git a/libavcodec/trace_headers_bsf.c b/libavcodec/trace_headers_bsf.c
index 028b0a1e59..2124981d21 100644
--- a/libavcodec/trace_headers_bsf.c
+++ b/libavcodec/trace_headers_bsf.c
@@ -44,6 +44,7 @@  static int trace_headers_init(AVBSFContext *bsf)

      ctx->cbc->trace_enable = 1;
      ctx->cbc->trace_level  = AV_LOG_INFO;
+    ctx->cbc->trace_read_callback = ff_cbs_trace_read_log;

      if (bsf->par_in->extradata) {
          CodedBitstreamFragment *frag = &ctx->fragment;