[FFmpeg-devel] [PATCH v2 2/2] cbs_av1: Support redundant frame headers

[Mark Thompson]

On 05/11/18 18:16, James Almer wrote:
> On 11/5/2018 12:25 PM, Mark Thompson wrote:
>> ---
>> On 05/11/18 00:55, James Almer wrote:
>>> On 11/4/2018 9:10 PM, Mark Thompson wrote:
>>>> ...
>>>> +                xf(1, frame_header_copy[k], b, b, b, 1, k);
>>> This is making a lot of noise in the trace output for no real gain,
>>> since it prints every single bit as its own line. Can you silence it?
>> I think it's sensible to keep some trace output here to reflect what's actually happening, so I've made it go by bytes rather than bits to be less spammy.
>>
>>>> +            priv->frame_header_size = fh_bits;
>>>> +            priv->frame_header =
>>>> +                av_mallocz(fh_bytes + AV_INPUT_BUFFER_PADDING_SIZE);
>>>> +            if (!priv->frame_header)
>>>> +                return AVERROR(ENOMEM);
>>>> +            memcpy(priv->frame_header, fh_start, fh_bytes);
>>> No way to use AVBufferRef for this?
>> Refcounting added only for reading.  It's a bit nasty because it passes the bufferref down into the template code which shouldn't really have it, but I don't see any better way to make that work.
>>
>>>> ...
>> Also fixed the memory leak.
>>
>> Thanks,
>>
>> - Mark
>>
>>
>>  libavcodec/cbs_av1.c                 | 16 ++++--
>>  libavcodec/cbs_av1.h                 |  5 +-
>>  libavcodec/cbs_av1_syntax_template.c | 82 +++++++++++++++++++++++++---
>>  3 files changed, 91 insertions(+), 12 deletions(-)
>>
>> ...
>> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
>> index e146bbf8bb..0da79b615d 100644
>> --- a/libavcodec/cbs_av1_syntax_template.c
>> +++ b/libavcodec/cbs_av1_syntax_template.c
>> @@ -1463,24 +1463,90 @@ static int FUNC(uncompressed_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>  }
>>  
>>  static int FUNC(frame_header_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>> -                                  AV1RawFrameHeader *current)
>> +                                  AV1RawFrameHeader *current, int redundant,
>> +                                  AVBufferRef *rw_buffer_ref)
>>  {
>>      CodedBitstreamAV1Context *priv = ctx->priv_data;
>> -    int err;
>> -
>> -    HEADER("Frame Header");
>> +    int start_pos, fh_bits, fh_bytes, err;
>> +    uint8_t *fh_start;
>>  
>>      if (priv->seen_frame_header) {
>> -        // Nothing to do.
>> +        if (!redundant) {
>> +            av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid repeated "
>> +                   "frame header OBU.\n");
>> +            return AVERROR_INVALIDDATA;
>> +        } else {
>> +            GetBitContext fh;
>> +            size_t i, b;
>> +            uint32_t val;
>> +
>> +            HEADER("Redundant Frame Header");
>> +
>> +            av_assert0(priv->frame_header_ref && priv->frame_header);
>> +
>> +            init_get_bits(&fh, priv->frame_header,
>> +                          priv->frame_header_size);
>> +            for (i = 0; i < priv->frame_header_size; i += 8) {
>> +                b = FFMIN(priv->frame_header_size - i, 8);
>> +                val = get_bits(&fh, b);
>> +                xf(b, frame_header_copy[i],
>> +                   val, val, val, 1, i / 8);
>> +            }
>> +        }
>>      } else {
>> +        if (redundant)
>> +            HEADER("Redundant Frame Header (used as Frame Header)");
>> +        else
>> +            HEADER("Frame Header");
>> +
>>          priv->seen_frame_header = 1;
>>  
>> +#ifdef READ
>> +        start_pos = get_bits_count(rw);
>> +#else
>> +        start_pos = put_bits_count(rw);
>> +#endif
>> +
>>          CHECK(FUNC(uncompressed_header)(ctx, rw, current));
>>  
>>          if (current->show_existing_frame) {
>>              priv->seen_frame_header = 0;
>>          } else {
>>              priv->seen_frame_header = 1;
>> +
>> +            av_buffer_unref(&priv->frame_header_ref);
>> +
>> +#ifdef READ
>> +            fh_bits  = get_bits_count(rw) - start_pos;
>> +            fh_start = (uint8_t*)rw->buffer + start_pos / 8;
>> +#else
>> +            // Need to flush the bitwriter so that we can copy its output,
>> +            // but use a copy so we don't affect the caller's structure.
>> +            {
>> +                PutBitContext tmp = *rw;
>> +                flush_put_bits(&tmp);
>> +            }
>> +
>> +            fh_bits  = put_bits_count(rw) - start_pos;
>> +            fh_start = rw->buf + start_pos / 8;
>> +#endif
>> +            fh_bytes = (fh_bits + 7) / 8;
>> +
>> +            priv->frame_header_size = fh_bits;
>> +
>> +            if (rw_buffer_ref) {
>> +                priv->frame_header_ref = av_buffer_ref(rw_buffer_ref);
>> +                if (!priv->frame_header_ref)
>> +                    return AVERROR(ENOMEM);
>> +                priv->frame_header = fh_start;
> 
> Is this the reason you can't create the ref outside the template code?
> If it's only to skip the OBU header bits, can't that be done right after
> the call to cbs_av1_read_frame_header_obu()?

... which is also called from cbs_av1_read_frame_obu(), and may or may not be wanted in the redundant frame header case.  It's all fixable with some more magic state in the other direction (maybe priv->make_frame_header_ref indicating that the caller should ref the current OBU buffer to remember the frame header), but that splits it between two places and isn't obviously any nicer.

>> +            } else {
>> +                priv->frame_header_ref =
>> +                    av_buffer_alloc(fh_bytes + AV_INPUT_BUFFER_PADDING_SIZE);
>> +                if (!priv->frame_header_ref)
>> +                    return AVERROR(ENOMEM);
>> +                priv->frame_header = priv->frame_header_ref->data;
>> +                memcpy(priv->frame_header, fh_start, fh_bytes);
>> +            }
>>          }
>>      }
>>  
>> ...

- Mark