GNOME Bugzilla – Bug 721772
decoder: add support for H.264 MVC (Annex.H)
Last modified: 2014-06-03 11:46:00 UTC
Support MVC(stereo profile) decoding in gstreamer-vaapi, related gstreamer-codecparser patches can be tracked here: https://bugzilla.gnome.org/show_bug.cgi?id=685215
Created attachment 265640 [details] [review] MVC dec: Fix the slice data offset when MVC NAL presents. When MVC slice NAL(ext slice and prefix NAL) present, the NAL header bytes sum is three, not one byte as usual
Created attachment 265641 [details] [review] MVC dec: Wrap DPB related parameters to a structure One DPB(decoded picture buffer) for each view, and every DPB is managed independently, this policy also complies with the requirement from MVC
Created attachment 265642 [details] [review] MVC dec: Map stereo and multiview profiles to H264 we use H264 high profile to decode MVC profiles in plugins and driver level
Created attachment 265643 [details] [review] MVC dec: Parse MVC specific NALs before decoding Parse MVC subset sps NAL, prefix NAL and slice extend NAL in h264 decoder.
Created attachment 265644 [details] [review] MVC dec: add non-base view decoding support Add non base view DPB management to support MVC decoding, mainly involves non-base view dpb and interview reference list create/initialize/reorder/remove and other related operations.
Created attachment 265645 [details] [review] MVC dec: Register references picture in all DPB layer. Register all reference frames in both base and no-base dpb layer to picture parameters, otherwise no-base view decoding will affect the reference frame information of base view in driver level, so this should be a workaround because of driver
Created attachment 265646 [details] [review] MVC dec: Handle the initialization of pic_num for view Avoid to calculate the pic number for reference pictures which has a different layer id from current slice
Created attachment 265647 [details] [review] MVC dec: Handle the prefix NAL unit correctly Cache the prefix_nal until we finish the parsing of next nal unit. If the nal unit following the prefix_nal unit is a SLICE/SLICE_IDR nal then, the prefix_nal should be considered as the part of it's associated nal unit. Likewise we should skip the prefix_nal and associated_nal if the associated nal is not a SLICE/SLICE_IDR
Created attachment 265648 [details] [review] MVC dec: Detect the first VCL NAL unit of a picture. Detect the first VCL nal unit of a picture for mvc based on view_id and VOIdx as per H.7.4.1.2.4
Created attachment 265649 [details] [review] MVC dec: Dump all layer dpb when end of decoding Output all frames located in both base view dpb and non-base view DPB alternately.
Created attachment 265650 [details] [review] MVC dec: adjust the views output order Add one output frames pool to guarantee views with same POC will be output successively, the output views(left and right) should be in order of "L0,R0,L1,R1...", this would be helpful for stereo video processing
Created attachment 265651 [details] [review] MVC dec: initialize NAL parser information object Clear up the parameters when creating the nal parser information object, otherwise these uninitialized parameters may cause decoding error
Created attachment 265652 [details] [review] MVC dec: limit the MVC view number to 2
Created attachment 265653 [details] [review] MVC dec: flush one or all dpb layers selectively When detect one IDR picture in init_picture(), all previous reference frames in all dpb layers will be flushed out alternaltive, and in other cases just flush current active layer dpb pools
Created attachment 265654 [details] [review] MVC dec: Fix B frame display order issue MVC dec: Fix B frame display order issue Usually B frames will be not used as reference, so when DPB is full we need display it in time, in MVC display mechanism, we need store it to pending pool at first, because some picture with lower POC may still exists in pending pool, otherwise some B frames will display before some picture with lower POC value
Created attachment 265655 [details] [review] MVC dec: initialize defalut value of mvc_view_count
(In reply to comment #0) > Support MVC(stereo profile) decoding in gstreamer-vaapi, related > gstreamer-codecparser patches can be tracked here: > https://bugzilla.gnome.org/show_bug.cgi?id=685215 Inorder to code view convenience, I also attached MVC realted codecparser patches to the bottom of the patches list.
Created attachment 265656 [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 265657 [details] [review] codecparsers: h264: parse MVC syntax elements
Created attachment 265658 [details] [review] codecparsers: h264: parse seq_parameter_set_mvc_extension
Created attachment 265660 [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 265661 [details] [review] h264parser: Fix the IdrPicFlag calculation for MVC If there is a non_idr_flag present in the MVC nal unit header extension, then the IdrPicFlag calculation will be based on this field
Created attachment 265662 [details] [review] h264parser: parse the mvc extension for Stereoscopic 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 265663 [details] [review] codecparser: h264: append nal header bytes to slice hdr
Created attachment 265664 [details] [review] codecparser: fix the slice hdr parsing for MVC
xiaowei: Why did you Restrict Group Visibility on this bug report? Is there something private in here?
(In reply to comment #26) > xiaowei: Why did you Restrict Group Visibility on this bug report? > Is there something private in here? There is no privacy at all, I have already removed this restriction.
Review of attachment 265640 [details] [review]: +1
Review of attachment 265641 [details] [review]: +1 and just a small question of the view num. ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c @@ +41,3 @@ +/* Define the max views supported */ +#define MAX_VIEW_NUM 2 is it possible to allocate required num of views in dbp_init? just for suggestion.
Review of attachment 265642 [details] [review]: +1
Review of attachment 265643 [details] [review]: +1
Review of attachment 265644 [details] [review]: +1 ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c @@ +1554,3 @@ + continue; + + dpb_layer = &priv->dpb_layers[layer_id]; possible remove each dbp_layer init into a seperate function
Review of attachment 265645 [details] [review]: +1
Review of attachment 265646 [details] [review]: +1
Review of attachment 265647 [details] [review]: +1
Review of attachment 265648 [details] [review]: +1
Review of attachment 265649 [details] [review]: +1 ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c @@ +3694,3 @@ + dpb_clear(decoder); + + priv->current_dpb_layer = dpb_layer; I'm thinking maybe every dbp_xxx need a layer_id to avoid every reset of current_dpb_layer
Review of attachment 265650 [details] [review]: +1, comments can consider later. first is to make thing work. ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c @@ +44,3 @@ +/* Define the max frames pending for MVC output */ +#define MAX_PENDING_FRAME_NUM 4 why use 4, spec related to this num? @@ +628,3 @@ +static gboolean +pending_frames_bump(GstVaapiDecoderH264 *decoder) +{ not sure about this bump to sync views. if it's from the spec it's ok, else maybe consider a better views sync.
Review of attachment 265651 [details] [review]: +1
Review of attachment 265652 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c @@ +1088,3 @@ + return GST_VAAPI_DECODER_STATUS_ERROR_UNKNOWN; + } + are most MVC videos got 2 views?
Review of attachment 265653 [details] [review]: +1
Review of attachment 265655 [details] [review]: +1
(In reply to comment #38) > Review of attachment 265650 [details] [review]: > +1, comments can consider later. first is to make thing work. > ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c > @@ +44,3 @@ > +/* Define the max frames pending for MVC output */ > +#define MAX_PENDING_FRAME_NUM 4 > why use 4, spec related to this num? > @@ +628,3 @@ > +static gboolean > +pending_frames_bump(GstVaapiDecoderH264 *decoder) > +{ > not sure about this bump to sync views. if it's from the spec it's ok, else > maybe consider a better views sync. (In reply to comment #38) > Review of attachment 265650 [details] [review]: > +1, comments can consider later. first is to make thing work. > ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c > @@ +44,3 @@ > +/* Define the max frames pending for MVC output */ > +#define MAX_PENDING_FRAME_NUM 4 > why use 4, spec related to this num? > @@ +628,3 @@ > +static gboolean > +pending_frames_bump(GstVaapiDecoderH264 *decoder) > +{ > not sure about this bump to sync views. if it's from the spec it's ok, else > maybe consider a better views sync. not from the spec, our legacy decoder may bump out several frames for display when decoding one view, this is not prefered when matching complementary view. This method is applied to be compatible with H264 single view decoding. Also the pending buffer size is not finalized yet, the bigger, the better, but we need save memory. If possible , we can work out one better solution to sync views.
(In reply to comment #40) > Review of attachment 265652 [details] [review]: > ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c > @@ +1088,3 @@ > + return GST_VAAPI_DECODER_STATUS_ERROR_UNKNOWN; > + } > + > are most MVC videos got 2 views? We original planned to support 2 views at most, but it is easy to extend to support more views decoding.
Created attachment 268974 [details] [review] Rename some mvc variables according to spec
Review of attachment 265656 [details] [review]: Merged into gst-vaapi codecparsers
Review of attachment 265657 [details] [review]: Merged into gst-vaapi codecparsers, among with current fixes.
Review of attachment 265658 [details] [review]: Merged into gst-vaapi codecparsers
Review of attachment 265660 [details] [review]: Merged into gst-vaapi codecparsers
Review of attachment 265661 [details] [review]: Merged into gst-vaapi codecparsers, in a previous patch and simplified to fit a single conditional result.
Review of attachment 265662 [details] [review]: Was already merged into the gst-vaapi codecparsers branch.
Review of attachment 265663 [details] [review]: This was not needed. We already have this information from the companion NAL data structure.
Review of attachment 265664 [details] [review]: Merged into upstream gst-plugin-bad a while back.
Review of attachment 265641 [details] [review]: For reference, I don't really believe in this. DPB fullness accounts for frame buffers that contain view components of other views. So, if you have multiple DPB "layers", you end up ruining the eviction/bumping process. DPB layer fullness != DPB fullness. Besides, and AFAIK, the standard only mentions a unique DPB with a size up to MIN( mvcScale /*2*/ * MaxDpbMbs / PicSizeMbs, NumViews * 16). cf. H.10.2. Now, is there an optimization that could help you split into multiple dpb layers. Maybe, but you'd still need to traverse other layers too. So, I don't quite see the real gain here.
Review of attachment 265640 [details] [review]: Merged into gstreamer-vaapi master branch.
Review of attachment 265642 [details] [review]: In theory, we could indeed support H.264 MVC decoding with standard H.264 AVC, or at least program the hardware to operate this way. In practice, it is not possible to generalize this. What is acceptable: 1. H.264 Stereo -> H.264 High profile, on Intel platforms only ; 2. H.264 Multiview High -> H.264 High profile if there are at most 16 frames in the DPB, on Intel platforms only. (1) because, as per the existing VA-API requirements, slice level decoding is performed, so it is perfectly fine to support stereo profiles. On other platforms (NVIDIA, AMD), where frame level decoding is used, this is not possible, unless the hardware also support the MVC syntax elements. So, the safer approach is to map that on Intel platforms only. (2) because of the same reasons as for (1) + because older Intel platforms have specific requirements. It could be possible to support more, but it's not worth the effort due to the added complexity (and minor performance cost). There would be a definitive solution, but it would be at a performance cost too since it would require patching certain buffers. Besides, those internal formats are not documented, so an independent party would have to reverse engineer them. So, full support for the multiview high profile requires an explicit support from the driver to attest that it will work in *any* case (+ expose multiview-high profile, of course). Otherwise, there is no guarantee that the finest details are to be supported.
Review of attachment 265641 [details] [review]: The NumViews definition confirms what I said. So, that cannot be the right approach. Besides, the JM Reference SW decoder for AVC, is not the reference SW decoder for MVC. So, implementation details/decisions made there are inappropriate for the full MVC set. Not to mention, MVC Stereo was explicitly marked as "experimental" in that SW, and that I am not going to integrate patches that borrow concepts from SW with probably incompatible licenses.
Review of attachment 265648 [details] [review]: That's the correct approach to detect view boundaries. However, in order to be strictly and fully conformant, it is needed to correctly detect access units boundaries too. Locally patched + added another patch for that. Besides, and for reference, in our gstreamer-vaapi implementation, the "Frame" relates to view components, not to the coded picture as mentioned in the standard. That's a minor difference to bear in mind.
Review of attachment 265647 [details] [review]: That's almost OK. Locally patched to change the strategy to (i) maintain previous NAL unit "parsed info" instead and (ii) infer default values for slice if the previous NAL unit was not a Prefix NAL unit (H.7.4.1.1).
Review of attachment 265645 [details] [review]: The driver needs to be fixed. I supplied patches for that. It's not acceptable to have driver workarounds in codec layers, not to mention that there are other codec layers that would benefit of having the real fixes in the driver too...
Review of attachment 265643 [details] [review]: parse_slice() from NAL with type={20,21} is inappropriate for non-{AVC,MVC} streams. So additional checks are required at this time, in view to supporting base layers of SVC streams too. Besides, this patch alone could actually bring other issues. Locally fixed in view to supporting base views of MVC streams only too.
Review of attachment 265651 [details] [review]: Why is this needed in particular? The code is finally tuned so that an explicit additional initialization is not needed. i.e. implicit initializations are done. So, if some values are wrong for you, this either means that (i) those values are used whereas they shouldn't in reality, or (ii) the implicitly initializers are incorrect. So far, I have not captured any defficiency related to (ii). So, please be more specific.
Review of attachment 265654 [details] [review]: That pool is not needed at all. Should you have handled the DPB in... a unique DPB, it is trivial to apply standard requirements whereby pictures are output in order by view order index. So, this means that you can output pictures/views of lower POC first, along with associated view components that were in the same access unit.
(In reply to comment #62) > Review of attachment 265651 [details] [review]: > > Why is this needed in particular? The code is finally tuned so that an explicit > additional initialization is not needed. i.e. implicit initializations are > done. So, if some values are wrong for you, this either means that (i) those > values are used whereas they shouldn't in reality, or (ii) the implicitly > initializers are incorrect. So far, I have not captured any defficiency related > to (ii). So, please be more specific. s/finally/finely/ s/implicitly initializers/implicit initializers/ s/defficiency/deficiency/ for correctness purposes. :)
Review of attachment 265643 [details] [review]: Another thing, forgot to mention we were leaking memory from the associated per-extension data. Locally fixed. gst_h264_sps_clear() needs to be called on gst_vaapi_parser_info_h264_finalize().
Pushed to git master branch. Need to commit vaapiparse_h264 and most importantly the VA driver patches.
commit abcc8bb7a2f45852af6957af91a656a65a46fc50 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Wed May 21 17:57:00 2014 +0200 decoder: h264: add support for MVC interlaced streams. Fix support for MVC Stereo High profile streams with interlaced frames. Also improve the detection logic of the first field. commit 5d76afb40d0eaea34331278dfc4b44120f6013cb Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Tue May 20 18:08:15 2014 +0200 decoder: h264: add MVC profiles compatibility logic. Add safe fallbacks for MVC profiles: - all MultiView High profile streams with 2 views at most can be decoded with a Stereo High profile compliant decoder ; - all Stereo High profile streams with only progressive views can be decoded with a MultiView High profile compliant decoder ; - all drivers that support slice-level decoding could normally support MVC profiles when the DPB holds at most 16 frames. commit 8c5e89c29801e5c0b7d4bc2d2dc86f0dd1df3e5d Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Fri May 2 14:58:45 2014 +0200 decoder: h264: add initial support for MVC. https://bugzilla.gnome.org/show_bug.cgi?id=721772 commit 42b112f8bd7cd8fedc1b57e4b1b5e3f4142e23fd Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Thu May 1 19:16:09 2014 +0200 decoder: h264: dynamically allocate the DPB. Dynamically allocate the Decoded Picture Buffer (DPB) and add provisions for supporting the MVC allocation requirements. commit ae1d8267d9830487d6868db6f12aebde1c1c5068 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Thu May 1 19:33:40 2014 +0200 decoder: h264: fix detection of access unit boundaries. In order to have a stricter conforming implementation, we need to carefully detect access unit boundaries. Additional operations could be necessary to perform at those boundaries. commit a6215a0757fd8e1321dae8c9b3a34f7a9c220420 Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Wed Mar 13 11:44:38 2013 +0200 decoder: h264: detect the first VCL NAL unit of a picture for MVC. Detect the first VCL NAL unit of a picture for MVC, based on the view_id as per H.7.4.1.2.4. Note that we only need to detect new view components. Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit e4166f5a919fe90839333d2dda1138f44af33ffe Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Thu Oct 31 19:32:55 2013 +0800 decoder: h264: properly handle Prefix NAL units. Always cache the previous NAL unit so that we could check whether there is a Prefix NAL unit immediately preceding the current slice or IDR NAL unit. In that case, the NAL unit metadata is copied into the current NAL unit. Otherwise, some default values are inferred, tentatively. e.g. view_id shall be set to 0 and inter_view_flag to 1. [infer default values for slice if previous NAL was not a Prefix] Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit 79118904a07031acef6116df2885ec30dcbad231 Author: Xiaowei Li <xiaowei.a.li@intel.com> Date: Thu Feb 28 15:59:55 2013 +0800 decoder: h264: add support for MVC base views. Allow decoding for base views of MVC encoded streams. For now, just skip the slice extension and prefix NAL units, and skip non-base view frames. Signed-off-by: Xiaowei Li <xiaowei.a.li@intel.com> [fixed memory leak, improved check for MVC NAL units] Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
The gstreamer-vaapi part is completed. Closing here as it is not possible to cross reference bugs on different DBs and sites. :)