GNOME Bugzilla – Bug 764947
autodetect: bring element down to NULL on a successful choice
Last modified: 2017-04-10 14:44:00 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.
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.
Created attachment 325794 [details] [review] glimagesink: fix "prepare-window-handle" message with autovideosink fix reference leak
(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.
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.
(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?
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
I do not see this fix in 1.10 branch
It's here https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?h=1.10&id=67f6d3358e4620319335065db25edaaba1f5ae0a
(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.
I'm going to revert it for 1.10 for now then, this apparently needs further investigation
(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
Joakim, what specifically does it break for you other than changing the behaviour?
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.
So there is not really any problem here, and we can just consider it solved?
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.