[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.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.

(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