GNOME Bugzilla – Bug 793876
h265parse: add support for 'Format range extensions profiles'
Last modified: 2018-10-18 15:47:21 UTC
Those patches implement the profiles defined in section A.3.5 of the spec. I split them to make them easier to review but we may want to squash them before merging. I considered internally storing profiles using one byte rather than 8 distinct fields but that may be premature optimization so I'd like input about that first.
Created attachment 369007 [details] [review] h265parse: clean up get_profile_string() - Use constants rather than magic numbers to identify profiles - Use a switch on profile_idc - Pass GstH265SPS as I'm going to need it to detect profile extensions No semantic change.
Created attachment 369008 [details] [review] h265parser: parse extra profile fields Those fields have been introduced in version 2 and later to define new profiles like the format range extensions profiles (A.3.5).
Created attachment 369009 [details] [review] h265parse: add support for 'Format range extensions profiles' Those profiles have been introduced in version 2 of the HEVC spec (A.3.5).
If we are too late in the 1.14 cycle to merge this that's ok. But I'd be interested to get feedback on the profile names defined in the latest patch. We have customer code using those profiles and I'd like to make sure we won't diverge from upstream once this is merged.
The names seem fine to me. They look nice enough and match https://en.wikipedia.org/wiki/High_Efficiency_Video_Coding#Version_2_profiles so what's not to like? Presumably they match with the spec too? I'm not too fussed about the freeze here, it's just an enum really. My main question is whether the enum value should be GST_H265_PROFILE_FORMAT_RANGE_EXTENSION or if there should be new enum values for all these extended profiles? (I didn't have time to look at the context, spec or code much, just commenting from the sidelines.)
(In reply to Tim-Philipp Müller from comment #5) > The names seem fine to me. They look nice enough and match > > > https://en.wikipedia.org/wiki/High_Efficiency_Video_Coding#Version_2_profiles > > so what's not to like? Presumably they match with the spec too? Yeah, I just used the names from the spec (Table A.2), put them in lowercase and replace spaces with '-'. > My main question is whether the enum value should be > GST_H265_PROFILE_FORMAT_RANGE_EXTENSION or if there should be new enum > values for all these extended profiles? Good point. The exact semantic of this enum isn't that clear to be either. It's not explicitly exposed in the parser API and only used to check profile_idc so I assumed it was meant to map the semantic of this field. On the other hand it could make sense to move the profile detection logic from the element to the lib and have something like this: GstH265Profile gst_h265_profile_tier_level_get_profile (GstH265ProfileTierLevel *ptl);
Review of attachment 369009 [details] [review]: ::: gst/videoparsers/gsth265parse.c @@ +1296,3 @@ + + return profile.name; + } I think it would be better to match a compatible profile instead of doing an exact match. As an example, we know that we have bitstream using a unamed profile: {"uname", 1, 1, 1, 1, 0, 0, 0, 0, TRUE}, This is compatible with "main-422-10", just that max_8bit_constraint_flag is set. As per the spec a "main-422-10" compliant decoder should support this, there is just no specific profile name for it. In this implementation, the profile would not be set, and then any decoder will endup being tried. So I think we should expose a profile name that are compatible. That's closer to what we do in H264 parser, where we simply check the IDC and if certain flags are set. To do se, we'll need to search for a profile that match, and then keep the one that has the least extra constraints. In our example, main-422-10 would have 1 extra contraints, while main-422-12 would have two, so main-422-10 would be a better valid profile choice for the custom bitstream.
Created attachment 369142 [details] [review] h265parser: decouple GstH265Profile and GstH265ProfileIDC We used to have the same enum to represent H265 profiles and idc values. Those are no longer the same with extension profiles defined from version 2 of the spec. Split those enums so the semantic of each is clearer and we'll be able to add extension profiles to GstH265Profile. Also add gst_h265_profile_tier_level_get_profile() to retrieve the GstH265Profile from the GstH265ProfileTierLevel. It will be used to implement the detection of extension profiles.
Created attachment 369143 [details] [review] h265parser: parse extra profile fields Those fields have been introduced in version 2 and later to define new profiles like the format range extensions profiles (A.3.5).
Created attachment 369144 [details] [review] h265parse: add support for 'Format range extensions profiles' Those profiles have been introduced in version 2 of the HEVC spec (A.3.5).
Here is a new set of patches implementing a cleaner API in the parser lib. Are you happy with this approach? I did not implemented yet Nicolas's suggestion regarding partial matching. I'd like to add some unit tests first (which should now be easy thanks to gst_h265_profile_tier_level_get_profile().
Review of attachment 369144 [details] [review]: ::: gst-libs/gst/codecparsers/gsth265parser.h @@ +56,3 @@ GST_H265_PROFILE_MAIN_10 = 2, + GST_H265_PROFILE_MAIN_STILL_PICTURE = 3, + GST_H265_PROFILE_MONOCHROME, Forgot to document the new enums. Fixing in the new version of the patch.
For the record my WIP branch is here: https://gitlab.collabora.com/cassidy/gst-plugins-bad/commits/hevc-profiles
(In reply to Nicolas Dufresne (stormer) from comment #7) > I think it would be better to match a compatible profile instead of doing an > exact match. As an example, we know that we have bitstream using a unamed > profile: > > {"uname", 1, 1, 1, 1, 0, 0, 0, 0, TRUE}, > > This is compatible with "main-422-10", just that max_8bit_constraint_flag is > set. As per the spec a "main-422-10" compliant decoder should support this, > there is just no specific profile name for it. In this implementation, the > profile would not be set, and then any decoder will endup being tried. > > So I think we should expose a profile name that are compatible. That's > closer to what we do in H264 parser, where we simply check the IDC and if > certain flags are set. To do se, we'll need to search for a profile that > match, and then keep the one that has the least extra constraints. In our > example, main-422-10 would have 1 extra contraints, while main-422-12 would > have two, so main-422-10 would be a better valid profile choice for the > custom bitstream. In this example we actually have 2 profiles having 1 extra contraint: main-422-10 and main-444. Which logic should the parser use to pick one rather than the other?
If you sort the initial array in a certain order, you'll be able to pick one. Which one does not matter, as long as it's valid. As this mostly affects preemptive allocation, is there one smaller ?
Created attachment 369342 [details] [review] h265parser: decouple GstH265Profile and GstH265ProfileIDC We used to have the same enum to represent H265 profiles and idc values. Those are no longer the same with extension profiles defined from version 2 of the spec. Split those enums so the semantic of each is clearer and we'll be able to add extension profiles to GstH265Profile. Also add gst_h265_profile_tier_level_get_profile() to retrieve the GstH265Profile from the GstH265ProfileTierLevel. It will be used to implement the detection of extension profiles.
Created attachment 369343 [details] [review] h265parser: parse extra profile fields Those fields have been introduced in version 2 and later to define new profiles like the format range extensions profiles (A.3.5).
Created attachment 369344 [details] [review] h265parse: add support for 'Format range extensions profiles' Those profiles have been introduced in version 2 of the HEVC spec (A.3.5).
Created attachment 369345 [details] [review] h265parser: allow partial matching on range extension profile Best to return a valid profiles rather than no profile if bitstream uses a not standard profile.
Review of attachment 369342 [details] [review]: Good.
Review of attachment 369343 [details] [review]: We'll need to mark the API break here (adding fields in the middle of the structure), but it's -bad so it make sense to keep it clean.
Review of attachment 369344 [details] [review]: Good.
Review of attachment 369345 [details] [review]: That's what I was looking for.
Review of attachment 369343 [details] [review]: Isn't this an ABI break? How should I mark this break?
it's ABI break, sorry, wrong acronyme, I'll take care, mentioning it in the comment is important for later tracking.
Attachment 369342 [details] pushed as d252f50 - h265parser: decouple GstH265Profile and GstH265ProfileIDC Attachment 369343 [details] pushed as 977af86 - h265parser: parse extra profile fields Attachment 369344 [details] pushed as 9f25fcd - h265parse: add support for 'Format range extensions profiles' Attachment 369345 [details] pushed as 6dd9975 - h265parser: allow partial matching on range extension profile
See also https://bugzilla.gnome.org/show_bug.cgi?id=772584 This fix here was a bit incomplete :) Can someone here who knows about this h265 specifics review and merge those patches?
Incomplete like we missed some profiles or incomplete because we didn't think of updating pbutils ?
The latter
Ok, the split is unfortunate, we forgot about it because we didn't use it in our current application. Guillaume is away for couple of weeks, hopefully I'll be able to give it a look next week.