GNOME Bugzilla – Bug 731974
videodecoder: parse source data until a frame is obtained
Last modified: 2014-07-04 12:57:25 UTC
Parse any pending data until a complete frame is obtained. This is a memory optimization to avoid expansion of video packets stuffed into the GstAdapter, and a fix to EOS condition when the user .parse() function ultimately only need to call into gvd_have_frame() and doesn't need any gvd_add_to_frame(). Additionally optimize the check for any pending data in the GstAdapter by using gst_adapter_available_fast(). We now only want to determine if there is any data left, not exactly how much. This avoids a full traversal of the GstAdapter list of buffers. Note: the same could apply to git master (1.3), but due to immediate release needs, I only tested for 1.2. Interestingly, this fixes custom pipelines with gst-launch-1.0 command line when h264parse with "byte-stream" format and "au" alignment is used. Somehow, programmatically, decodebin|playbin would get it right with either "au" or "nal" alignment.
Created attachment 278848 [details] [review] videodecoder: parse source data until a frame is obtaine
Waiting for QA validation with their gst-launch based tool. I have unfortunately not caught that earlier since I programatically use decodebin|playbin where this behaviour is not exposed.
(In reply to comment #2) > Waiting for QA validation with their gst-launch based tool. I have > unfortunately not caught that earlier since I programatically use > decodebin|playbin where this behaviour is not exposed. Here are the tests I ran 1. stream-format=avc,alignment=au 168/168 (100.0%) 2. stream-format=byte-stream,alignment=nal 166/168 (98.81%) 3. stream-format=byte-stream,alignment=au 162/168 (94.43%) I believe the remaining issues are from gstreamer-vaapi, and more particularly the h264parse backport I use from git master.
And, looking at the logs, this is confirmed, independently from gstreamer-vaapi. h264parse bails out, and no chance is given to the decode elements. I will investigate tomorrow.
(In reply to comment #4) > And, looking at the logs, this is confirmed, independently from > gstreamer-vaapi. h264parse bails out, and no chance is given to the decode > elements. I will investigate tomorrow. This part is now tracked in bug #732203.
(In reply to comment #3) > (In reply to comment #2) > > Waiting for QA validation with their gst-launch based tool. I have > > unfortunately not caught that earlier since I programatically use > > decodebin|playbin where this behaviour is not exposed. > > Here are the tests I ran > 1. stream-format=avc,alignment=au 168/168 (100.0%) > 2. stream-format=byte-stream,alignment=nal 166/168 (98.81%) > 3. stream-format=byte-stream,alignment=au 162/168 (94.43%) > > I believe the remaining issues are from gstreamer-vaapi, and more particularly > the h264parse backport I use from git master. With other patches to h264parse, the results are now: For H.264 AVC 1. stream-format=avc,alignment=au 168/168 (100.0%) 2. stream-format=byte-stream,alignment=nal 166/168 (98.81%) 3. stream-format=byte-stream,alignment=au 166/168 (98.81%) For H.264 MVC "all-views" 1. stream-format=avc,alignment=au 20/20 (100.0%) 2. stream-format=byte-stream,alignment=nal 20/20 (100.0%) 3. stream-format=byte-stream,alignment=au 20/20 (100.0%) The failures for H.264 AVC in stream-format=byte-stream mode are the same in either alignment mode. For some reason, I get the last frame with nal_unit_type = 0. This is probably an unrelated issue though.
With all other fixes to h264parse as mentioned in bug #732154, bug #732156, bug #732167 and bug #732203, the results are: For H.264 AVC 1. stream-format=avc,alignment=au 168/168 (100.0%) 2. stream-format=byte-stream,alignment=nal 166/168 (98.81%) 3. stream-format=byte-stream,alignment=au 168/168 (100.0%) For H.264 MVC "all-views" 1. stream-format=avc,alignment=au 20/20 (100.0%) 2. stream-format=byte-stream,alignment=nal 20/20 (100.0%) 3. stream-format=byte-stream,alignment=au 20/20 (100.0%) The remaining two issues are FRExt1_Panasonic_D, FRExt2_Panasonic_C. There are two kind of bugs, it seems we are receiving an invalid NAL unit (codec parser bug?), and also missed two frames at the end of the stream, but after an end-of-sequence (EOSEQ) was received. The H.264 standard allows this behaviour though, i.e. decoding past an EOSEQ is allowed.
Note: support for H.264 is tracked in bug #696135 but I am mentioning that here because this impacts how access units are collected. i.e. all patches need to maintain this in a working state too. :)
Review of attachment 278848 [details] [review]: ::: gst-libs/gst/video/gstvideodecoder.c @@ +926,3 @@ ret = decoder_class->parse (dec, priv->current_frame, priv->input_adapter, at_eos); + available = gst_adapter_available_fast (priv->input_adapter); Makes sense but please add a counter here to check if we iterate more than twice without consuming data, and in that case error out completely (it would be a bug in the subclass!)
Created attachment 279744 [details] [review] videodecoder: parse any source data that is still available Fix gst_video_decoder_parse_available() to really parse any pending source data that is still available in the adapter. This is a memory optimization to avoid expansion of video packed added to the adapter, but also a fix to EOS condition when the subclass parse() function ultimately only needed to call into gvd_have_frame() and no additional source bytes were consumed, i.e. gvd_add_to_frame() is not called. This situation can occur when decoding H.264 streams in byte-stream/nal mode for instance. A decoder always requires the next NAL unit to be parsed so that to determine picture boundaries. When a new picture is found, no byte is consumed (i.e. gvd_add_to_frame() is not called) but gvd_have_frame() is called (i.e. priv->current_frame is gone). Also make sure to avoid infinite loops caused by incorrect subclass parse() implementations. This can occur when no byte gets consumed and no appropriate indication (GST_VIDEO_DECODER_FLOW_NEED_DATA) is returned.
Note: I dropped the use of gst_adapter_available_fast() because this would complicate the counter check. Second, it turns out that the "fast" here is only if we map the adapter. The current gst_adapter_available() implementation just returns an internal (accumulated) bytes counter. i.e. the default gst_adapter_available() turns out to be faster to process than gst_adapter_available_fast(). :)
Review of attachment 279744 [details] [review]: Looks good
Review of attachment 279744 [details] [review]: Applied to git master branch. Thanks. I also added a comment in-source. This would be more useful than reading through logs for other developers.
commit fb44ec9615489f8baa388f658a86991635697697 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Fri Jun 20 18:02:31 2014 +0200 videodecoder: parse any source data that is still available. Fix gst_video_decoder_parse_available() to really parse any pending source data that is still available in the adapter. This is a memory optimization to avoid expansion of video packed added to the adapter, but also a fix to EOS condition when the subclass parse() function ultimately only needed to call into gvd_have_frame() and no additional source bytes were consumed, i.e. gvd_add_to_frame() is not called. This situation can occur when decoding H.264 streams in byte-stream/nal mode for instance. A decoder always requires the next NAL unit to be parsed so that to determine picture boundaries. When a new picture is found, no byte is consumed (i.e. gvd_add_to_frame() is not called) but gvd_have_frame() is called (i.e. priv->current_frame is gone). Also make sure to avoid infinite loops caused by incorrect subclass parse() implementations. This can occur when no byte gets consumed and no appropriate indication (GST_VIDEO_DECODER_FLOW_NEED_DATA) is returned. https://bugzilla.gnome.org/show_bug.cgi?id=731974