[FFmpeg-devel] [PATCH v1] avcodec/cbs: Keep ff_cbs_trace_syntax_element
Dai, Jianhui J
jianhui.j.dai at intel.com
Tue Oct 17 15:42:56 EEST 2023
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: Tuesday, October 17, 2023 5:14 AM
> To: ffmpeg-devel at 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 at intel.com>
> >> Sent: Tuesday, October 10, 2023 10:57 AM
> >> To: ffmpeg-devel at 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 at 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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email ffmpeg-devel-request at ffmpeg.org with
> subject "unsubscribe".
More information about the ffmpeg-devel
mailing list