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

[Mark Thompson]

On 05/11/18 18:51, James Almer wrote:
> On 11/5/2018 3:33 PM, Mark Thompson wrote:
>> 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.
> 
> Alright. It could be looked at again later in any case.
> 
> LGTM, thanks!

Ok, applied to master and the 4.1 branch.

Thanks,

- Mark