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