Re: [FFmpeg-devel] [PATCH v1] avcodec/cbs: Keep ff_cbs_trace_syntax_element

[Mark Thompson]

On 10/10/2023 04:30, Dai, Jianhui J wrote:

-----Original Message-----
From: Dai, Jianhui J <jianhui.j....@intel.com>
Sent: Tuesday, October 10, 2023 10:57 AM
To: ffmpeg-devel@ffmpeg.org
Subject: [PATCH v1] avcodec/cbs: Keep ff_cbs_trace_syntax_element

Split ff_cbs_trace_syntax_element from ff_cbs_trace_read_log to decouple the
tracing from GetBitContext. This allows CBS implementations that do not have a
GetBitContext to directly use ff_cbs_trace_syntax_element to trace syntax
elements.

Signed-off-by: Jianhui Dai <jianhui.j....@intel.com>
---
  libavcodec/cbs.c          | 41 +++++++++++++++++++++++++--------------
  libavcodec/cbs_internal.h |  4 ++++
  2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c index cdd7adebeb..2f2cfcfb31
100644
--- a/libavcodec/cbs.c
+++ b/libavcodec/cbs.c
@@ -498,26 +498,18 @@ void ff_cbs_trace_header(CodedBitstreamContext
*ctx,
      av_log(ctx->log_ctx, ctx->trace_level, "%s\n", name);  }

-void ff_cbs_trace_read_log(void *trace_context,
-                           GetBitContext *gbc, int length,
-                           const char *str, const int *subscripts,
-                           int64_t value)
+void ff_cbs_trace_syntax_element(CodedBitstreamContext *ctx, int position,
+                                 const char *str, const int *subscripts,
+                                 const char *bits, int64_t value)
  {
-    CodedBitstreamContext *ctx = trace_context;
      char name[256];
-    char bits[256];
      size_t name_len, bits_len;
      int pad, subs, i, j, k, n;
-    int position;
-
-    av_assert0(value >= INT_MIN && value <= UINT32_MAX);

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

-    av_assert0(length < 256);
-    for (i = 0; i < length; i++)
-        bits[i] = get_bits1(gbc) ? '1' : '0';
-    bits[length] = 0;
+    av_assert0(value >= INT_MIN && value <= UINT32_MAX);

      subs = subscripts ? subscripts[0] : 0;
      n = 0;
@@ -545,7 +537,7 @@ void ff_cbs_trace_read_log(void *trace_context,
      av_assert0(n == subs);

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

      if (name_len + bits_len > 60)
          pad = bits_len + 2;
@@ -556,6 +548,25 @@ void ff_cbs_trace_read_log(void *trace_context,
             position, name, pad, bits, value);  }

+void ff_cbs_trace_read_log(void *trace_context,
+                           GetBitContext *gbc, int length,
+                           const char *str, const int *subscripts,
+                           int64_t value) {
+    CodedBitstreamContext *ctx = trace_context;
+    char bits[256];
+    int position;
+
+    position = get_bits_count(gbc);
+
+    av_assert0(length < 256);
+    for (int i = 0; i < length; i++)
+        bits[i] = get_bits1(gbc) ? '1' : '0';
+    bits[length] = 0;
+
+    ff_cbs_trace_syntax_element(ctx, position, str, subscripts, bits,
+value); }
+
  void ff_cbs_trace_write_log(void *trace_context,
                              PutBitContext *pbc, int length,
                              const char *str, const int *subscripts, diff --git
a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h index
07220f1f3e..ff90ce467d 100644
--- a/libavcodec/cbs_internal.h
+++ b/libavcodec/cbs_internal.h
@@ -158,6 +158,10 @@ 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

@Mark Thompson <s...@jkqxz.net>
Could you please take a look if it's a valid change based on your last refactor?
It's intended to use for the reviewing cbs_vp8 patch.

The simple answer here is to forge a GetBitContext pointing at the right place, 
like the write tracing does.

However: for your VP8 case, I'm a bit unclear why it isn't using get_bits 
already?  The setup seems to be to stop normal parsing at the end of the 
uncompressed header and then read bytes through a pointer for the range coder 
rather than continuing with the bit reader.

If the range decoder used the GetBitContext to read the input instead of 
reading bytes from the array then your problem would be solved.  Doing that 
would also allow it to renormalise directly after each read rather than reading 
by bytes, so the actual bits consumed for each symbol would be visible in 
tracing.

(I admit I'm not very clear what your motivation for making a read-only CBS 
implementation for VP8 is, and that might guide the best answer.  If it's 
tracing then showing the consumed bits precisely seems useful, but if it's 
something else then that's less relevant.)

Thanks,

- Mark
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".