GNOME Bugzilla – Bug 685215
codecparsers: h264: Add initial MVC parser
Last modified: 2014-10-29 13:18:35 UTC
Hi, This patch series adds initial parsing for MVC. The first two patches are just helper functions that are to be reused for both MVC and SVC parsers. The third patch adds initial MVC support if the user doesn't care of MVC specific syntax elements in subset SPS header. The last patch adds full MVC SPS header parsing. New VUI and SEI messages are left out for now. I am not fully sure this is the right way to handle the subset SPS headers but it's definitely needed to perform dynamic memory allocation somewhere. Otherwise, a full SPS descriptor suitable for MVC expands to a few MBs. Just wanted to publish the MVC codec parser early so that to get your feedback on the proposed API. Thanks.
Created attachment 225487 [details] [review] codecparsers: h264: add gst_h264_parse_sps_data() helper Split seq_parameter_set_data() parsing off gst_h264_parse_sps() so that it could be re-used later on.
Created attachment 225488 [details] [review] codecparsers: h264: add gst_h264_parse_nalu_header() helper. Add helper to parse the NALU header. Move bounds checking to there.
Created attachment 225489 [details] [review] codecparsers: h264: parse MVC syntax elements Add basic MVC parser without MVC-specific subset SPS header data.
Created attachment 225490 [details] [review] codecparsers: h264: parse seq_parameter_set_mvc_extension() The "large" patch that handles subset SPS data for MVC. Due to the number of possible views, dynamically allocation of data structures were necessary to me, unless we want the full subset SPS structure to be larger than one megabyte, thus not fitting stack anyway.
Created attachment 225506 [details] [review] codecparsers: h264: parse MVC syntax elements
Created attachment 225507 [details] [review] codecparsers: h264: parse seq_parameter_set_mvc_extension() Fixed and cleaned up version.
Created attachment 235146 [details] [review] codecparsers: h264: add gst_h264_parse_nalu_header() helper Fix minor off-by-one error in gst_h264_parse_nalu_header().
Created attachment 235147 [details] [review] codecparsers: h264: parse MVC syntax elements - handle stereoscopic profiles ; - re-arrange gst_h264_parse_nalu_header() to accomodate SVC ; - remove slice_parse_ref_pic_list_mvc_modification(). The final ISO specification fixed the typos ; - add constraint_set{4,5}_flag
This has been laying in my private repositories for a long time. Not touched since, just regenerated the patches and submitted here. This fixed a couple of parsing regressions and improvements to cover more streams. I also removed the videoparsers changes. Slightly more changes are needed, especially for better AU detection.
The second attachment (http://bugzilla-attachments.gnome.org/attachment.cgi?id=225506) seems obsoleted by fourth one..right?
(In reply to comment #10) > The second attachment > (http://bugzilla-attachments.gnome.org/attachment.cgi?id=225506) seems > obsoleted by fourth one..right? Fixed. You should get patches 0001 to 0004 now with other correctly obsoleted.
Created attachment 235513 [details] [review] codecparsers: h264parse: Fix the nal_unit header parsing. Initialize the bit reader to correct offset and skip the svc_extension_flag before parsing NalUnitExtensionMVC header.
Created attachment 235515 [details] [review] codecparsers: h264parse: Fix the nal_unit header parsing. Initialize the bit reader to correct offset and skip the svc_extension_flag before parsing NalUnitExtensionMVC header. Just renamed the previous patch to 0005-* to keep the proper order with Gwenole's patch sets.
Created attachment 237352 [details] [review] 264parser: Fix the IdrPicFlag calculation for MVC streams. If there is a non_idr_flag present in the MVC nal unit header extension, the IdrPicFlag calculation will be based on this field.
Created attachment 237984 [details] [review] codecparsers: h264parser: parse mvc extension for Stereoscopic High Profile Parse the seq_parameter_set_mvc_extension for both Stereoscopic(profile_idc==128) profile and Multiview (profile_idc==118) profile during subset sequence parameter set parsing. Currently the parser API is only considering the Multiview profile.
Created attachment 238381 [details] [review] h264parser: Add stereo_video_info payload parsing to SEI parser API. Adding necessary payload parsing support to SEI parser api to support the StereoScopic profile.
Created attachment 238382 [details] [review] h264parser: Add frame_packing_arrangement payload parsing to SEI parser API. This will parse the frame_packing_arrangement payload in SEI message. This information can be used by the decoders to appropriately rearrange the samples which belongs to Stereoscopic and Multiview profiles.
Created attachment 238774 [details] [review] h264parser: Add enum value for sps_extension nal unit type. Not related to mvc parser. But just completing the NAL_UNIT type enums.
Created attachment 239074 [details] [review] h264parser: define the max value for SPS+SSPS count. For MVC streams, numOfSequenceParameterSets indicates the number of SPSs and subset SPSs that are used for decoding the MVC elementary stream which has a max count value of 128 (7 bit field in MVCDecoderConfigurationRecord).
I have created a new bug for tracking mvc stream parsing support in h264_videoparser element based on the APIs provided here. https://bugzilla.gnome.org/show_bug.cgi?id=696135
This has been hanging here for the last seven months !! Why can't we push these stuffs?
Like all new API it will have to go through our API review process. Please be patient. It might have to wait to the next cycle (along with the other stereoscopic/multiview stuff).
I can't see any reason to keep all these patches in bugzilla with out pushing to master for more than a year !! Keeping something like this will not help to stabilize the code.
While I agree with you, it still needs someone to review the code and spend some time on it. It would be great to merge this though... Do these patches still apply against latest master? I could move this up on my list a bit, but no promises...
(In reply to comment #24) > While I agree with you, it still needs someone to review the code and spend > some time on it. It would be great to merge this though... It was a working code (and had decent stability). If it get pushed sometimes before, then it could have reached enough stability by this time. > > Do these patches still apply against latest master? most probably not..and this is one of the main problem because of the delay. Each time some one need to put effort to rebase it.. I could move this up on my > list a bit, but no promises...
Review of attachment 239074 [details] [review]: I have dropped this patch from the MVC codecparser series. The H.264 standard is strict, SPS id range is from 0 to 31, thus 32 entries maximum. If this is an issue for the videoparser, please open a separate issue.
Review of attachment 238381 [details] [review]: Fixed locally. Several issues in there. BTW, it would be interesting to list any sample that exhibit this SEI message. Thanks. :) BTW, this patch is non-essential for the whole MVC codecparser series. ::: gst-libs/gst/codecparsers/gsth264parser.c @@ +1210,3 @@ + READ_UINT8 (nr, info->next_frame_is_second_view_flag, 1); + READ_UINT8 (nr, info->left_view_self_contained_flag, 1); + READ_UINT8 (nr, info->right_view_self_contained_flag, 1); Missed field_views_flag condition for top_field_is_left_view_flag, current_frame_is_left_view_flag, next_frame_is_second_view_flag. Besides, using the _unchecked variant is possible and desirable. ::: gst-libs/gst/codecparsers/gsth264parser.h @@ +875,3 @@ GstH264BufferingPeriod buffering_period; GstH264PicTiming pic_timing; + GstH264StereoVideoInfo stereo_info; stereo_video_info to match H.264 spec.
Review of attachment 238382 [details] [review]: Non essential to MVC codecparser series but should be OK. Thanks.
Created attachment 272783 [details] [review] codecparsers: h264: complete set of NAL unit types
Created attachment 272784 [details] [review] codecparsers: h264: add gst_h264_parse_sps_data() helper
Created attachment 272785 [details] [review] codecparsers: h264: add gst_h264_parse_nalu_header() helper
Created attachment 272786 [details] [review] codecparsers: h264: parse MVC syntax elements
Created attachment 272787 [details] [review] codecparsers: h264: parse seq_parameter_set_mvc_extension()
Created attachment 272788 [details] [review] codecparsers: h264: fix slice_header() parsing for MVC The idr_pic_id syntax element depends on IdrPicFlag, which is a calculated value that does not only depend on NAL unit type (IDR), but possibly also on MVC non_idr_flag syntax element. This is not specific to MVC. So, I will eventually commit as is, as obvious bug fix.
Created attachment 272789 [details] [review] codecparsers: h264: add support for Stereo Video Information SEI message
Created attachment 272790 [details] [review] codecparsers: h264: add support for Frame Packing Arrangement SEI message Added GST_H264_FRAME_PACKING_NONE and fixed conformance to D.2.25 in regards to frame_packing_arrangement_extension_flag and any subsequent bits, within the payload, that need to be skipped.
Patches submitted as attachment #272783 [details], attachment #272784 [details], attachment #272785 [details], attachment #272788 [details] are trivial enough to be committed as is. Comments welcome for others, but they are fine to me. SEI message parsing selection will be changed to a switch instead.
(In reply to comment #24) > While I agree with you, it still needs someone to review the code and spend > some time on it. It would be great to merge this though... > > Do these patches still apply against latest master? I could move this up on my > list a bit, but no promises... Done, patches were rebased to master. Reviewed once more, and patches mentioned in comment #38 are either trivial or cosmetical changes. Even the SEI related ones, I'd say. Others indeed brings in new APIs that could be discussed. I renamed gst_h264_sps_free_1() to gst_h264_sps_clear() as it could be misleading. i.e. it only clears the internal resources, no the actual GstH264Sps structure.
Regression testing completed with the patches mentioned in comment #38, they will be pushed so that to only keep MVC-specific patches in this bug report.
Review of attachment 272790 [details] [review]: ::: gst-libs/gst/codecparsers/gsth264parser.c @@ +950,3 @@ + if (!frame_packing_extension_flag) + goto error; + nal_reader_skip (nr, payload_size - (nal_reader_get_pos (nr) - start_pos)); I will introduce a nal_reader_skip_long() function instead. It looks like nal_reader_skip() is more tailored to nbits <= sizeof(cache) * 8 [64 bits]. Thus, also simplifying the default case in the SEI payloader parser.
Review of attachment 272783 [details] [review]: Pused to master.
Review of attachment 272784 [details] [review]: Pushed to master.
Review of attachment 272785 [details] [review]: Pushed to master.
Review of attachment 272788 [details] [review]: Pushed to master.
Review of attachment 272783 [details] [review]: Pushed to master.
Review of attachment 272787 [details] [review]: I will keep gst_h264_sps_copy() internal and only export (and comment) gst_h264_sps_clear() function instead. WDYT? In terms of new functions, that's the only tolerable one. As to the MVC specific structure names, comments are welcome. Though, I could hardly do differently but dynamically allocating the arrays to be completely generic and conforming. Hence, gst_h264_sps_clear() will be needed to release any internal resources used.
Review of attachment 272786 [details] [review]: ::: gst-libs/gst/codecparsers/gsth264parser.h @@ +481,3 @@ guint8 constraint_set3_flag; + guint8 constraint_set4_flag; + guint8 constraint_set5_flag; I will move that down to maintain some binary compatibility.
(In reply to comment #48) > Review of attachment 272786 [details] [review]: > > ::: gst-libs/gst/codecparsers/gsth264parser.h > @@ +481,3 @@ > guint8 constraint_set3_flag; > + guint8 constraint_set4_flag; > + guint8 constraint_set5_flag; > > I will move that down to maintain some binary compatibility. Sorry, I lied, no release was made up from -bad master, and I would like to keep the fields aligned together. There is readily an ABI incompatible change in maste vs. 1.2, so another one wouldn't hurt I believe. And we can still bump the SONAME anyway.
Created attachment 272847 [details] [review] codecparsers: h264: parse seq_parameter_set_mvc_extension() Changes: - Made gst_h264_sps_copy() internal only
Created attachment 272848 [details] [review] codecparsers: h264: add support for Stereo Video Information SEI message Changes: - Migrated to use a switch in gst_h264_parser_parse_sei_message() - Added some log message in h264parse element to shut up compiler warnings
Created attachment 272849 [details] [review] codecparsers: h264: add support for Frame Packing Arrangement SEI message Changes: - Migrated to use a switch in gst_h264_parser_parse_sei_message() - Added log message in h264parse element to shut up compiler warnings - Used nal_reader_skip_long() as depicted in the previous comment
Review of attachment 272786 [details] [review]: ::: gst-libs/gst/codecparsers/gsth264parser.c @@ +242,3 @@ + break; + default: + nalu->extension_type = GST_H264_NAL_EXTENSION_NONE; Before I forget, default initialization to GST_H264_NAL_EXTENSION_NONE needs to be moved up before the switch() so that we can have some sensible value for SVC streams, even if we don't support them. Or, we should correctly initialize that for SVC NAL units too, and reject them subsequently in parser functions.
No additional change needed beyond comment #53. Full H.264 MVC decoding support was pushed to gstreamer-vaapi git master branch. No regression on H.264 AVC conformance testing either. Please let me know if this could be suitable for 1.4.y with y > 0 or y == 0? :) I would prefer 1.4.0 because of bug #727017, thus avoiding further ABI changes in the future. Even though gstreamer-vaapi is the only user at this time, and has its own local copy, I'd really like minimal changes between those, reduced to none actually. Thanks.
Thanks, merged with one minor fix and pushed to master. Sorry it took so long!
Just to be sure, this breaks ABI through the SPS structure extension and is not suitable for 1.4 (because of that and in general).