[FFmpeg-devel] [PATCH v2] avcodec: add an AV1 parser

[James Almer]

On 10/3/2018 10:38 AM, Derek Buitenhuis wrote:
> On 03/10/2018 14:26, James Almer wrote:
>>> Shouldn't this be set at the bottom of this block (since parsing can fail)?
>>
>> If extradata parsing fails and we bail out without setting this first,
>> no packet will ever be parsed since this runs first thing every time.
>>
>> A better API would allow us to check it during init(), like in the
>> decoder and bitstream filter ones, but that's not the case here.
> 
> OK, yeah, that's a bit meh, but I see the point.
> 
>>> Not strictly related to this patch, and not a blocker, but is it not
>>> about time the API gains the ability to mark frames with more clarity
>>> than just "I"? H.264, HEVC, etc. also have this issue (e.g. I vs IDR,
>>> and HEVC's many times of RAPs). AV_PICTURE_TYPE_SI is kinda related,
>>> I guess, but not exactly what I mean.
> 
> [...]
> 
>>> I assume we're not going to mark alt-refs in any special way since
>>> they're combined in one packet with a visible frame?
>>
>> Exactly. They are handled in the following packets, when the visible
>> frame is a show_existing_frame one pointing to them.
> 
> [...]
> 
>>>> +        ctx->picture_structure = AV_PICTURE_STRUCTURE_FRAME;
>>>
>>> Any reason we just don't always set this at the top of the function?
>>
>> Because the parsing can fail, and i figured it was nicer to keep it as
>> the default AV_PICTURE_STRUCTURE_UNKNOWN in those cases.
> 
> Fair enough.
>>>> +        av_assert2(ctx->format != AV_PIX_FMT_NONE);
>>>
>>> I assume ctx->bit_depth will always be set to something valid.
>>
>> If you look at how i set ctx->format, it depends on the values of
>> color->subsampling_x and color->subsampling_y. If for whatever reason
>> cbs_av1 wrongly sets the former as 0 and the latter as 1 (which is
>> invalid), the lookup table will return AV_PIX_FMT_NONE. This assert is
>> to make sure that doesn't happen, as it'd be an internal bug.
>>
>> I can remove it if you prefer, but by being av_assert2 it will not run
>> outside of builds purposely enabling level 2 asserts.
> 
> I have no strong opinon.
> 
> As an aside, where does HDR metadata fit into all of this?

That's in Metadata OBUs and/or in container defined structures. It's
meant to be propagated internally as stream/packet/frame side data,
which the parser API can't handle.

The demuxer and/or the decoder are the ones that need to handle that.

> 
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>