GNOME Bugzilla – Bug 732203
h264parse: improve handling of byte-stream/au format
Last modified: 2014-07-04 12:57:26 UTC
If stream-format=byte-stream and alignment=au, i.e. when this is not avcC format (avc/au), h264parse will skip NAL unit if we did not receive an SPS/PPS pair yet. The issue is that this not only drops the unhandled NAL unit (e.g. SPS-Ext or SEI message with buffering_period()), but also *all* previous NALs. Subsequently, downstream doesn't know about SPS, and would fail to decode the stream. There are multiple ways to fix this. However, since we are about to release 1.4-RC, I am going to explore the following solution: always call gst_h264_parse_process_nal() in _handle_frame, regardless of the current NAL unit and make that function perform some checking (e.g. got SPS, got PPS, etc.), and skip the current NAL in case of failure then. This would make it possible to (i) handle SEI NAL with buffering_period() message inserted between an SPS and PPS, (ii) handle valid SPS-Ext NAL unit that follow the SPS, and fix additional bugs. The most correct way to fix this, IMHO, is to enable transform = TRUE in that case and accumulate the valid NAL units to the GstAdapter, instead to just starting over. With the additional condition that, if we need to genuinely skip a NAL unit (e.g. broken NAL), and if no slice NAL was received so far, then, don't drop the previous NALs, emit them right away. That could allow some form of recovery to downstream. Most importantly, we keep receiving SPS and PPS. Both approaches require the same amount of time for implementation. However, approach (2) is more risky and requires additional regression testing. That's why I would focus on approach (1), which I believe should fit our immediate purposes.
I will also rebase patch for bug #732156 onto this.
More precisely, "perform some checking" is meant to be something along what I implemented for gstreamer-vaapi: GstH264ParserState with { got_sps, got_pps, got_slice }, thus replacing have_sps and have_pps variables altogether.
Created attachment 279217 [details] [review] h264parse: introduce new state tracking variables Improve parser state tracking by introducing new flags reflecting it: "got-sps", "got-pps" and "got-slice". This is an addition for robustness purposes. Older have_sps and have_pps variables are kept because they have a different meaning. i.e. they are used for deciding on when to submit updated caps or not, and rather mean "have new SPS/PPS to be submitted?"
Created attachment 279218 [details] [review] h264parse: improve conditions for skipping NAL unit Carefully track cases when skipping broken or invalid NAL units is necessary. In particular, always allow NAL units to be processed and let that gst_h264_parse_process_nal() function decide on whether the current NAL needs to be dropped or not. This fixes parsing of streams with SEI NAL buffering_period() message inserted between SPS and PPS, or SPS-Ext NAL following a traditional SPS NAL unit, among other cases too. Practical examples include from H.264 AVC conformance suite include alphaconformanceG, CVSE2_Sony_B, CVSE3_Sony_H, CVSEFDFT3_Sony_E when parsing in stream-format=byte-stream,alignment=au mode.
(In reply to comment #4) > Created an attachment (id=279218) [details] [review] > h264parse: improve conditions for skipping NAL unit > > Carefully track cases when skipping broken or invalid NAL units is necessary. > In particular, always allow NAL units to be processed and let that > gst_h264_parse_process_nal() function decide on whether the current NAL needs > to be dropped or not. > > This fixes parsing of streams with SEI NAL buffering_period() message inserted > between SPS and PPS, or SPS-Ext NAL following a traditional SPS NAL unit, among > other cases too. > > Practical examples include from H.264 AVC conformance suite include > alphaconformanceG, CVSE2_Sony_B, CVSE3_Sony_H, CVSEFDFT3_Sony_E when parsing in > stream-format=byte-stream,alignment=au mode. This also obsoletes patch for bug #731831. I am not too certain of that former requirement to discard SEI messages available between SPS and PPS. They are definitely allowed, at least for buffering_period() messages. I could trivially update gst_h264_parse_process_sei() to return a boolean and check for that if needed. i.e. only allow buffering_period() to appear anywhere after an SPS, and disallow all other SEI messages between SPS and PPS.
I meant bug #732156 :)
Created attachment 279236 [details] [review] tests: h264parse: add test for byte-stream/au output Add tests. However, the test_parse_skip_garbage() test is disabled because it would require additional work. Essentially, solution (2) mentioned above. i.e. use a GstAdapter in this case.
(In reply to comment #7) > Created an attachment (id=279236) [details] [review] > tests: h264parse: add test for byte-stream/au output > > Add tests. However, the test_parse_skip_garbage() test is disabled because it > would require additional work. Essentially, solution (2) mentioned above. i.e. > use a GstAdapter in this case. I finally foresee a patch of around 10 lines to properly implement solution (2), based on the first two patches. That should be safe enough too. I gave the hint in comment #1 :)
Created attachment 279297 [details] [review] h264parse: fix collection of access units to preserve config headers Always use a GstAdapter when collecting access units (alignment="au") in either byte-stream or avcC format. This is required to properly preserve config headers like SPS and PPS when invalid or broken NAL units are subsequently parsed. More precisely, this fixes scenario like: [ <SPS> <PPS> <invalid-NAL> <slice> ] where we used to reset the output frame buffer when an invalid or broken NAL is parsed, i.e. SPS and PPS NAL units were lost, thus preventing the next slice unit to be decoded, should this also represent any valid data. This addresses comment #8
Created attachment 279298 [details] [review] tests: h264parse: add test for byte-stream/au output This enables the test_parse_skip_garbage() when converting to byte-stream/au format.
I don't expect any more patches to address the issues reported in this bug. Please review the patches. Thanks. :)
(In reply to comment #10) > Created an attachment (id=279298) [details] [review] > tests: h264parse: add test for byte-stream/au output > > This enables the test_parse_skip_garbage() when converting to byte-stream/au > format. This patch fixes decoding of FRExt1_Panasonic_D and FRExt2_Panasonic_C in byte-stream/au mode, thus allowing a 100% PASS rate with vaapi in this mode. To be honest, this is unexpected, and I will try to dig in what precise NAL this corresponded to.
Grmbl, in reply to comment #9 actually.
Created attachment 279312 [details] [review] tests: h264parse: add test for byte-stream/au output Changes: - Improved commit log to include a more meaningful description. - Dropped obsolete comments: all leading zeros must be preserved.
Review of attachment 279297 [details] [review]: ::: gst/videoparsers/gsth264parse.c @@ +1057,3 @@ + !(h264parse->state & GST_H264_PARSE_STATE_VALID_PICTURE_HEADERS) || + (h264parse->state & GST_H264_PARSE_STATE_GOT_SLICE)) + gst_h264_parse_reset_frame (h264parse); Wouldn't this throw away the complete AU if there is a broken SEI between SPS and PPS?
(In reply to comment #15) > Review of attachment 279297 [details] [review]: > > ::: gst/videoparsers/gsth264parse.c > @@ +1057,3 @@ > + !(h264parse->state & GST_H264_PARSE_STATE_VALID_PICTURE_HEADERS) || > + (h264parse->state & GST_H264_PARSE_STATE_GOT_SLICE)) > + gst_h264_parse_reset_frame (h264parse); > > Wouldn't this throw away the complete AU if there is a broken SEI between SPS > and PPS? Yes, you are right. On the other hand, I can't check the NAL unit type here (for SEI) because that could be used in cases where the NAL unit could not be parsed at all. Maybe add a check for SEI and alignment=au if gst_h264_parse_process_nal() failed and goto out in this case?
Sounds like a plan, if I understood you correctly :)
(In reply to comment #17) > Sounds like a plan, if I understood you correctly :) Hmm, it seems that I was not too mad when I wrote the patch because no change seems to be needed after all. :) The check is: !(h264parse->state & GST_H264_PARSE_STATE_VALID_PICTURE_HEADERS) which means, "no SPS nor PPS received so far". I have not used GST_H264_PARSE_STATE_VALID(), which checks for all the supplied bits in state. However, the troublesome case is if alignment=au and (SPS or PPS was received) and we got a slice too. We should reset the frame only if the broken NAL is a slice.
(In reply to comment #18) > (In reply to comment #17) > > Sounds like a plan, if I understood you correctly :) > > Hmm, it seems that I was not too mad when I wrote the patch because no change > seems to be needed after all. :) I will try to amend the SEI-based test to exhibit that. > The check is: > !(h264parse->state & GST_H264_PARSE_STATE_VALID_PICTURE_HEADERS) > > which means, "no SPS nor PPS received so far". I have not used > GST_H264_PARSE_STATE_VALID(), which checks for all the supplied bits in state. > > However, the troublesome case is > if alignment=au and (SPS or PPS was received) and we got a slice too. We should > reset the frame only if the broken NAL is a slice. I have no idea for this one yet, but at least I presume that the situation is no worse than without the patch.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > Sounds like a plan, if I understood you correctly :) > > > > Hmm, it seems that I was not too mad when I wrote the patch because no change > > seems to be needed after all. :) > > I will try to amend the SEI-based test to exhibit that. Before trying to write a test, I should have read the sources. It turns out it is not possible to reach this condition: "skip a broken/unsupported SEI NAL unit". Reason: gst_h264_parse_process_sei() just ignores any error.
I have created bug #732542, but I finally think it's not worth the effort to finely report errors in SEI NAL units. Delegating that to downstream element could be just fine IMHO.
*** Bug 732156 has been marked as a duplicate of this bug. ***
commit 804c0ac27bb011dc3e4a62b547c7cbed34666612 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Wed Jun 25 17:19:00 2014 +0200 tests: h264parse: add test for byte-stream/au output. Check that conversion to byte-stream/au formats work and that we can effectively drop broken/invalid NAL units from the resulting access unit buffer. https://bugzilla.gnome.org/show_bug.cgi?id=732203 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit fb2263632524296f499812cdc548f4b3f9a60dd9 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Wed Jun 25 18:47:55 2014 +0200 tests: h264parse: check SEI buffering_period() message is output. If an SEI NAL unit with a buffering_period() message is inserted between an SPS and PPS NAL unit, check that the output buffer still contain it. i.e. make sure that this SEI message is not dropped. https://bugzilla.gnome.org/show_bug.cgi?id=732156 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit 7d44a51bfe6f54061956be9d31590fa11319a8ef Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Thu Jun 26 09:44:26 2014 +0200 h264parse: fix collection of access units to preserve config headers. Always use a GstAdapter when collecting access units (alignment="au") in either byte-stream or avcC format. This is required to properly preserve config headers like SPS and PPS when invalid or broken NAL units are subsequently parsed. More precisely, this fixes scenario like: <SPS> <PPS> <invalid-NAL> <slice> where we used to reset the output frame buffer when an invalid or broken NAL is parsed, i.e. SPS and PPS NAL units were lost, thus preventing the next slice unit to be decoded, should this also represent any valid data. https://bugzilla.gnome.org/show_bug.cgi?id=732203 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit 34c2cfd4ddcc370a273d5c86d403e421a94d0157 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Wed Jun 25 13:14:10 2014 +0200 h264parse: improve conditions for skipping NAL units. Carefully track cases when skipping broken or invalid NAL units is necessary. In particular, always allow NAL units to be processed and let that gst_h264_parse_process_nal() function decide on whether the current NAL needs to be dropped or not. This fixes parsing of streams with SEI NAL buffering_period() message inserted between SPS and PPS, or SPS-Ext NAL following a traditional SPS NAL unit, among other cases too. Practical examples from the H.264 AVC conformance suite include alphaconformanceG, CVSE2_Sony_B, CVSE3_Sony_H, CVSEFDFT3_Sony_E when parsing in stream-format=byte-stream,alignment=au mode. https://bugzilla.gnome.org/show_bug.cgi?id=732203 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit 7bb6443bfbaaddfe1f48617ba5f58518053736ab Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Wed Jun 25 11:06:41 2014 +0200 h264parse: introduce new state tracking variables. Improve parser state tracking by introducing new flags reflecting it: "got-sps", "got-pps" and "got-slice". This is an addition for robustness purposes. Older have_sps and have_pps variables are kept because they have a different meaning. i.e. they are used for deciding on when to submit updated caps or not, and rather mean "have new SPS/PPS to be submitted?" Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>