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 662330 - [decodebin2] Should link and add elements to the bin before checking if they can reach READY state
[decodebin2] Should link and add elements to the bin before checking if they ...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
0.10.x
Other Linux
: Normal enhancement
: 0.10.36
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-10-20 20:27 UTC by Nicolas Dufresne (ndufresne)
Modified: 2011-10-22 06:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
playbin2: link elements before testing ready state (6.60 KB, patch)
2011-10-20 20:27 UTC, Nicolas Dufresne (ndufresne)
needs-work Details | Review
Link before testing, now using the decodebin lock (5.97 KB, patch)
2011-10-21 18:34 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2011-10-20 20:27:20 UTC
Created attachment 199584 [details] [review]
playbin2: link elements before testing ready state

One of the basic rule in GStreamer is to allocate the resources (e.g. open the X11 display) in the transition from NULL to READY. The auto-plugger take advantage of this by doing the state transition before adding and linking the element. The problem with that approach arrive when an element must share a context (see GstVideoContext proposal, bug #662321). As the element is not linked, it's not possible to query neighbour elements or application for an already allocated video context. This imply implementing hacks in the element to only allocate the display upon linking (e.g during a call to get_caps() if in ready state). As this is not straightforward and contributes to make things unclear, I propose to filter errors on elements and link the element before testing the ready state.

Note that more patches are required in GstPlaySink, audovideosrc and other auto-plugger to fully move to this approach.
Comment 1 Sebastian Dröge (slomo) 2011-10-21 06:53:08 UTC
Review of attachment 199584 [details] [review]:

Good idea and looks good in general, just two minor comments

::: gst/playback/gstdecodebin2.c
@@ +178,3 @@
+
+  GMutex *filtered_lock;
+  GList *filtered;              /* element for which error message are filterd */

You should be able to use the object lock here instead of introducing a new mutex.

@@ +1090,3 @@
+    g_mutex_free (decode_bin->filtered_lock);
+    decode_bin->filtered_lock = NULL;
+  }

Just to be on the safe side, please also free the list of filtered elements here
Comment 2 Nicolas Dufresne (ndufresne) 2011-10-21 18:34:52 UTC
Created attachment 199684 [details] [review]
Link before testing, now using the decodebin lock
Comment 3 Sebastian Dröge (slomo) 2011-10-22 06:28:36 UTC
commit cf9da5c280603edde373f71ff1319aea4222130a
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Oct 13 11:34:49 2011 -0400

    decodebin2: Link elements before testing if they can reach the READY state
    
    This is made possible by filtering errors. This is required to let
    harware accelerated element query the video context. The video context
    is used to determine if the HW is capable, and thus if the element is
    supported or not.
    
    Fixes bug #662330.