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