diff mbox series

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

Message ID DM6PR11MB26818A325A137C1C783852F7B1CDA@DM6PR11MB2681.namprd11.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,v1] avcodec/cbs: Keep ff_cbs_trace_syntax_element | expand

Commit Message

Dai, Jianhui J Oct. 10, 2023, 2:56 a.m. UTC 
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.dai@intel.com>
---
 libavcodec/cbs.c          | 41 +++++++++++++++++++++++++--------------
 libavcodec/cbs_internal.h |  4 ++++
 2 files changed, 30 insertions(+), 15 deletions(-)

Comments

Dai, Jianhui J Oct. 10, 2023, 3:30 a.m. UTC | #1 
> -----Original Message-----
> From: Dai, Jianhui J <jianhui.j.dai@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.dai@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 <sw@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.

> --
> 2.25.1
Mark Thompson Oct. 16, 2023, 9:13 p.m. UTC | #2 
On 10/10/2023 04:30, Dai, Jianhui J wrote:
>> -----Original Message-----
>> From: Dai, Jianhui J <jianhui.j.dai@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.dai@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 <sw@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
Dai, Jianhui J Oct. 17, 2023, 12:42 p.m. UTC | #3 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Tuesday, October 17, 2023 5:14 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v1] avcodec/cbs: Keep
> ff_cbs_trace_syntax_element
> 
> On 10/10/2023 04:30, Dai, Jianhui J wrote:
> >> -----Original Message-----
> >> From: Dai, Jianhui J <jianhui.j.dai@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.dai@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 <sw@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.
> 

Thanks for the suggestion. Using GetBitContext in the range decoder could solve this problem.

> (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.)
> 

I previously worked on a hardware VP8 encoding issue, and I needed to dump all the VP8 bitstream syntax elements for debugging.
To do this, I implemented the cbs VP8 for trace header filter. 
I am looking to upstream it if anyone else needs it for the same purpose.

> 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-request@ffmpeg.org with
> subject "unsubscribe".
diff mbox series

Patch

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