GNOME Bugzilla – Bug 733897
vaapidecode hangs when the downstream returns an error
Last modified: 2014-07-29 14:13:56 UTC
Pipeline: ... ! vaapidecode ! {glimagesink,cluttersink} The above pipeline hangs when the sink returns an error (e.g. when the window is closed)
Created attachment 281907 [details] [review] vaapidecode: return from decode loop properly when downstream returns an error
Created attachment 281909 [details] [review] vaapidecode: return from decode loop properly when downstream returns an error There was still a possible hang/race when decode_loop() signalled before _finish() had the chance to g_cond_wait()
Review of attachment 281909 [details] [review]: LGTM. I'd probably commit after 0.5.9 release. Otherwise, I'd need to respin the tarballs and test again on every build back to 0.10. Or, is this an issue that is bound to occur in common situations? Thanks. ::: gst/vaapi/gstvaapidecode.c @@ +450,3 @@ return; } + GST_VIDEO_DECODER_STREAM_UNLOCK(vdec); Maybe could we just move this one up. I like being able to count the number of lock vs. unlock operations to make sure we didn't have forgotten any. :)
commit 6003596e82b9aaaa699135f89bd7839ae7792cb7 Author: Matthew Waters <ystreet00@gmail.com> Date: Tue Jul 29 16:22:01 2014 +1000 vaapidecode: properly return from decode loop on downstream errors. Fixes a hang/race on shutdown where _decode_loop() had already completed its execution and _finish() was waiting on a GCond for decode_loop() to complete. Also fixes the possible race where _finish() is called but _decode_loop() endlessly returns before signalling completion iff the decoder instance returns GST_FLOW_OK. Found with: ... ! vaapidecode ! {glimagesink,cluttersink} https://bugzilla.gnome.org/show_bug.cgi?id=733897 [factored out GST_VIDEO_DECODER_STREAM_UNLOCK() call] Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
Re-opening.
Created attachment 281953 [details] [review] vaapidecode: decode and output all pending frames on normal EOS The previous brought an issue whereby it would have returned too early in normal conditions. The gst_vaapidecode_decode_loop() function is called within a separate task to fetch and output all frames that were decoded so far. So, if the decoder_loop_status is forcibly set to EOS when _finish() is called, then we are bound to exist the task without submitting the pending frames. If the downstream element error'ed out, then the gst_pad_push() would propagate up an error and so we will get it right for cutting off _finish() early in that case. Tested locally and it works for both my use cases (normal exist), and yours (abnormal exit from downstream). Could you please give it a try? Thanks.
(In reply to comment #6) > Tested locally and it works for both my use cases (normal exist), and yours > (abnormal exit from downstream). Could you please give it a try? Thanks. Works here :)
Pushed. Thanks. Hopefully, QA auto-testing bot would not notice it overnight. :)
commit 528f486513d406c45f1e30436f391f726bbb3b3d Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Tue Jul 29 15:47:46 2014 +0200 vaapidecode: decode and output all pending frames on normal EOS. The gst_vaapidecode_decode_loop() function is called within a separate task to fetch and output all frames that were decoded so far. So, if the decoder_loop_status is forcibly set to EOS when _finish() is called, then we are bound to exist the task without submitting the pending frames. If the downstream element error'ed out, then the gst_pad_push() would propagate up an error and so we will get it right for cutting off _finish() early in that case. This is a regression from 6003596. https://bugzilla.gnome.org/show_bug.cgi?id=733897