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

[Derek Buitenhuis]

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?

- Derek