GNOME Bugzilla – Bug 732553
codecparsers: h264: fix identification of EOSEQ and EOS NALs
Last modified: 2018-11-02 17:35:13 UTC
An end_of_seq() [EOSEQ] or end_of_stream() [EOS] NAL unit is really one byte long ecause this shall include the NalHeaderBytes (1) too. The NALU.offset starts from the first byte of the header.
Created attachment 279679 [details] [review] codecparsers: h264: fix identification of EOSEQ and EOS NALs I did not try to add a testcase because a minor infrastructure change would be needed there as only prefix'ed config headers are allowed, not extra data after the codec frame. Unless someone knows how to test that? This fixes parsing of FRExt1_Panasonic_D and FRExt2_Panasonic_C.
Comment on attachment 279679 [details] [review] codecparsers: h264: fix identification of EOSEQ and EOS NALs Makes sense, but why don't we scan for next start code if nalu->size == 1?
(In reply to comment #2) > (From update of attachment 279679 [details] [review]) > Makes sense, but why don't we scan for next start code if nalu->size == 1? We can only reach res == GST_H264_PARSER_OK && nalu->size == 1 if we have an EOSEQ or EOS. Once we got that, we cannot parse the next start code because it may not exist at all. This is OK because... we definitely don't have anything past an EOS NAL unit, and we might not have anything either after an EOSEQ.
Right, and if there is something afterwards we will handle it as usual. Find the start code and go ahead. Alright :)
Created attachment 279689 [details] [review] codecparsers: h264: clarifications and documentation fixes This is not strictly related, but here are the documentation fixes I plan to push at the same time. Hopefully, this also disambiguates the matters. Fix documentation for GstH264NalUnit. The @ref_idc part was totally unbalanced. Also add a note about @offset and @size fields to remind that this is relative to the start of the NAL unit, thus including the header bytes.
commit a61b7728b4d03f61c130d5b5df67e1903fe84a3b Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Tue Jul 1 16:09:28 2014 +0200 codecparsers: h264: clarifications and documentation fixes. Fix documentation for GstH264NalUnit. The @ref_idc part was totally unbalanced. Also add a note about @offset and @size fields to remind that this is relative to the start of the NAL unit, thus including the header bytes. commit 22b68b60ec7d83ef8487533cec3d4689b43bfd62 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Tue Jul 1 15:46:17 2014 +0200 codecparsers: h264: fix identification of EOSEQ and EOS NALs. An end_of_seq() [EOSEQ] or end_of_stream() [EOS] NAL unit is really one byte long because this shall include the NalHeaderBytes (1) too. The NALU.offset starts from the first byte of the header. This is the proper fix to commit d37f842. In practice, this fixes parsing of FRExt1_Panasonic_D and FRExt2_Panasonic_C, that include additional frames after an EOSEQ. https://bugzilla.gnome.org/show_bug.cgi?id=732553 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
What's really nice about this patch is that any 5 bytes with a start-code and valid type will now return GST_H264_PARSER_OK, instead of GST_H264_PARSER_NO_NAL_END. I'll file a specific bug considering how old this is.
(In reply to Gwenole Beauchesne from comment #3) > (In reply to comment #2) > > (From update of attachment 279679 [details] [review] [review]) > > Makes sense, but why don't we scan for next start code if nalu->size == 1? > > We can only reach res == GST_H264_PARSER_OK && nalu->size == 1 if we have an > EOSEQ or EOS. Once we got that, we cannot parse the next start code because > it may not exist at all. This is OK because... we definitely don't have > anything past an EOS NAL unit, and we might not have anything either after > an EOSEQ. ^ That's untrue. Whenever 5 first bytes of a NAL is sent, this check will succeed.