After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 758274 - gst-omx: video decoder blocks the pipeline when a drain request is issued during flush
gst-omx: video decoder blocks the pipeline when a drain request is issued dur...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-omx
git master
Other Linux
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-11-18 10:52 UTC by Enrique Ocaña González
Modified: 2016-07-06 09:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (2.16 KB, patch)
2015-11-18 10:52 UTC, Enrique Ocaña González
none Details | Review
Proposed patch 1/2 (5.04 KB, patch)
2015-11-18 13:12 UTC, Enrique Ocaña González
committed Details | Review
Proposed patch 2/2 (1.44 KB, patch)
2015-11-18 13:14 UTC, Enrique Ocaña González
committed Details | Review

Description Enrique Ocaña González 2015-11-18 10:52:54 UTC
Created attachment 315815 [details] [review]
Proposed patch

When a drain request is issued, the "input thread" in the OMX video decoder element sets the "draining" condition and keeps waiting until the streaming thread processes it and awakes the input thread by broadcasting on "drain_cond".

However, when a flush happens in the middle of this process, the code paths in gst_omx_video_dec_loop() just stop the streaming thread without caring about the "draining" condition, leaving the "input thread" waiting forever and half of the pipeline blocked because of that.

The proposed patch takes care of the "draining" condition during flushes and just does nothing (the flush is going to drain the component anyway).

Even with that change, gst_omx_video_dec_flush() blocks forever in http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n2110 when the previous call to gst_omx_component_set_state() fails in http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomx.c#n827. To mitigate that, I set "last_error" to true, so the code in http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomx.c#n862 exits early and doesn't block.
Comment 1 Sebastian Dröge (slomo) 2015-11-18 10:57:35 UTC
Review of attachment 315815 [details] [review]:

The same has to be done for gstomxaudiodec.c and maybe also the encoders.

::: omx/gstomx.c
@@ +836,3 @@
+        "Last operation returned an error. Setting last_error manually.");
+    comp->last_error = err;
+  }

Can you make this a separate commit?
Comment 2 Enrique Ocaña González 2015-11-18 13:12:29 UTC
Created attachment 315822 [details] [review]
Proposed patch 1/2
Comment 3 Enrique Ocaña González 2015-11-18 13:14:12 UTC
Created attachment 315823 [details] [review]
Proposed patch 2/2

Thanks for the suggestions.

I've split the patch and applied the same fix to the audio decoder and to the audio/video encoders.
Comment 4 Sebastian Dröge (slomo) 2015-11-18 13:19:30 UTC
commit 271093d6338f01501fc0bb1965e90f6b441f62fb
Author: Enrique Ocaña González <eocanha@igalia.com>
Date:   Wed Nov 18 13:00:28 2015 +0000

    Remember the last_error after a failed set state call to avoid blocking the next get state call
    
    gst_omx_video_dec_flush() blocks forever in
    http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c?id=9adf0ff82903cad5331e40975ae91ed5d11bc102#n2110
    when the previous call to gst_omx_component_set_state() fails in
    http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomx.c?id=9adf0ff82903cad5331e40975ae91ed5d11bc102#n827.
    To mitigate that, I set "last_error" to true, so the code in
    http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomx.c?id=9adf0ff82903cad5331e40975ae91ed5d11bc102#n862
    exits early and doesn't block.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=758274

commit d1a79d7c591fbe68f2048bcfb02a14ba49c312e4
Author: Enrique Ocaña González <eocanha@igalia.com>
Date:   Wed Nov 18 12:59:59 2015 +0000

    Properly handle drain requests while flushing
    
    Without this commit the decoder streaming thread stops without ever attending
    the drain request, leaving the decoder input thread waiting forever.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=758274