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

[James Almer]

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!

> 
>>> +            } 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>