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 764947 - autodetect: bring element down to NULL on a successful choice
autodetect: bring element down to NULL on a successful choice
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal blocker
: 1.12.0
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-04-12 11:47 UTC by Jakub Adam
Modified: 2017-04-10 14:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
glimagesink: fix "prepare-window-handle" message with autovideosink (2.98 KB, patch)
2016-04-12 11:47 UTC, Jakub Adam
none Details | Review
glimagesink: fix "prepare-window-handle" message with autovideosink (3.01 KB, patch)
2016-04-12 12:36 UTC, Jakub Adam
rejected Details | Review
autodetect: bring the element state down after success. (1.69 KB, patch)
2016-11-11 03:42 UTC, Matthew Waters (ystreet00)
committed Details | Review

Description Jakub Adam 2016-04-12 11:47:56 UTC
Created attachment 325789 [details] [review]
glimagesink: fix "prepare-window-handle" message with autovideosink

GstGLImageSink calls gst_video_overlay_prepare_window_handle() only once on initialization of its GL context. However, when autovideosink instantiates GL sink as its actual child sink element, that GL context creation happens while glimagesink is connected to a dummy GstBus, different from the bus of autosink's pipeline (see gst_auto_detect_find_best() in GstAutoDetect).

Because application never receives the "prepare-window-handle" element message posted on dummy bus, it doesn't get a chance to set its own window for video display and thus glimagesink always creates a new one.

Decoupling window handle assignment from GL context creation in this commit causes that "prepare-window-handle" message is resent whenever _ensure_gl_setup() is called with window_id unset, allowing the application to pick it up once autovideosink connects the glimagesink into its pipeline.
Comment 1 Matthew Waters (ystreet00) 2016-04-12 12:17:31 UTC
Review of attachment 325789 [details] [review]:

This unfortunately needs careful testing on all platforms to make sure everything still works.

I'm pretty sure X11, android and possibly wayland are fine with setting the window handle after context initialization. I'm not entirely sure about the others.

::: ext/gl/gstglimagesink.c
@@ +825,3 @@
+    gl_sink->window_id = gl_sink->new_window_id;
+    GST_DEBUG_OBJECT (gl_sink, "Setting window handle on gl window");
+    window = gst_gl_context_get_window (gl_sink->context);

You don't unref this anywhere.
Comment 2 Jakub Adam 2016-04-12 12:36:15 UTC
Created attachment 325794 [details] [review]
glimagesink: fix "prepare-window-handle" message with autovideosink

fix reference leak
Comment 3 Jakub Adam 2016-04-12 12:56:17 UTC
(In reply to Matthew Waters (ystreet00) from comment #1)
> I'm pretty sure X11, android and possibly wayland are fine with setting the
> window handle after context initialization. I'm not entirely sure about the
> others.

I've found the same piece of code which changes handle on a window with an existing context also in gst_glimage_sink_expose() and gst_glimage_sink_prepare() (perhaps it could be factored out into a common function?). So if it has been working until now, I'd expect this change shouldn't break anything.
Comment 4 Matthew Waters (ystreet00) 2016-11-11 03:42:51 UTC
Created attachment 339592 [details] [review]
autodetect: bring the element state down after success.

This fixes this issue specifically.  Another option is to implement message filtering in autodetect and add the element to the bin like decodebin does.
Comment 5 Joakim Tjernlund 2017-01-04 14:22:34 UTC
(In reply to Matthew Waters (ystreet00) from comment #4)
> Created attachment 339592 [details] [review] [review]
> autodetect: bring the element state down after success.
> 
> This fixes this issue specifically.  Another option is to implement message
> filtering in autodetect and add the element to the bin like decodebin does.

Is this now the preferred solution? Comitted even?
Comment 6 Matthew Waters (ystreet00) 2017-01-09 14:22:41 UTC
commit bbe88b190bbbbcf0c169b2e3909f5fc9c48b7caf
Author: Matthew Waters <matthew@centricular.com>
Date:   Fri Nov 11 14:31:03 2016 +1100

    autodetect: bring the element state down after success
    
    Otherwise some messages that are emitted by the element on NULL->READY
    will not reach the application.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=764947
Comment 7 Joakim Tjernlund 2017-01-09 15:41:41 UTC
I do not see this fix in 1.10 branch
Comment 9 Frank VanZile 2017-01-25 07:11:07 UTC
(In reply to Sebastian Dröge (slomo) from comment #8)
> It's here
> https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?h=1.
> 10&id=67f6d3358e4620319335065db25edaaba1f5ae0a

This seems wrong to me.  after 2 days of debugging.  I had to comment out 
gst_element_set_state (el, GST_STATE_NULL); 

i am on OSX, with a pipe of 

rtspsrc debug=false timeout=0 location=%@ latency=%d ntp-sync=false drop-on-latency=false udp-reconnect=true do-retransmission=true max-rtcp-rtp-time-diff=-1 name=rtsp ! rtph264depay name=x2 ! h264parse name=x3 ! vtdec name=x4 ! autovideosink  name=prim

when i set the pipe to ready.  the element get set to ready, then this change sets back to null.  setting the state to null closes and releases the element.  So, i do not think you want to set the state back to null.
Comment 10 Sebastian Dröge (slomo) 2017-01-26 11:53:54 UTC
I'm going to revert it for 1.10 for now then, this apparently needs further investigation
Comment 11 Joakim Tjernlund 2017-01-26 12:36:22 UTC
(In reply to Sebastian Dröge (slomo) from comment #10)
> I'm going to revert it for 1.10 for now then, this apparently needs further
> investigation

It seems like Jakub's original fix is the way to go
Comment 12 Sebastian Dröge (slomo) 2017-01-26 12:59:44 UTC
Joakim, what specifically does it break for you other than changing the behaviour?
Comment 13 Joakim Tjernlund 2017-01-26 16:10:06 UTC
Nothing, I am just using the pidgin-sipe SW Jakub has extended for Lync Desktop
sharing. So far Jakub's change has NOT broken anything here tough.
Comment 14 Sebastian Dröge (slomo) 2017-04-09 07:50:42 UTC
So there is not really any problem here, and we can just consider it solved?
Comment 15 Jakub Adam 2017-04-10 14:44:00 UTC
I've checked the problematic autovideosink + glimagesink case as I originally reported it, and with GStreamer 1.11.90 now the GL sink uses the correct window handle for its video display. No more extra windows popping up in my application.

This bug can therefore be closed.