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