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

["Dai, Jianhui J" <jianhui.j.dai-at-intel.com@ffmpeg.org>]

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