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