GNOME Bugzilla – Bug 736035
glimagesink: Regression: Gtk+ cannot get glimagesink input
Last modified: 2014-11-06 05:05:36 UTC
Recent changes to glwindow/x11 prevent Gtk+ mouse events to get captured. This is due to the implementation of the GstNavigation interface in glimagesink. https://bugzilla.gnome.org/show_bug.cgi?id=703486 Reverting 7b2c0d4431c630e409f7497b4104373d478c5107 solves the issue. The problem is caused by adding KeyPressMask, KeyReleaseMask, ButtonPressMask, ButtonReleaseMask and PointerMotionMask to win_attr.event_mask in gst-libs/gst/gl/x11/gstglwindow_x11.c. The scroll-event event still works in Gtk+, since it is not set on the X11 window. This minimal python example fails since the said commit https://gist.github.com/lubosz/d6209998907d122462a3 Please not that it still works for xvimagesink, despite GstNavigation being implemented. Please also note that gst-video-overlay-handle-events is not implemented in glimagesink. But it's not required to call that for xvimagesink. http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gstvideooverlay.html#gst-video-overlay-handle-events
Should probably look closer at how xvimagesink handles this code. But I would've assumed that it also filters out the events unless otherwise specifie (handle-events=true by default).
Created attachment 285893 [details] [review] Implement gst_video_overlay_handle_events() The X11 implementations of xvimagesink and glimagesink are slightly different. xvimagesink creates the X11 window with XCreateSimpleWindow. glimagesink needs to abstract window systems with the context. The context has also different windowing systems. In our case the GLX context creates a X11 window with classic XCreateWindow. I was not able to use XCreateSimpleWindow for GLX. I guess it needs the extra options. I removed the event handle options from the GLX X11 window initialization and added an option to set them later (heavily inspired by xvimagesink). This option needs to be carried though all the abstraction layers to the sink. With this patch I can disable the event handling of the sink. Even with event handling disabled I set the following events for the window to work properly: StructureNotifyMask | ExposureMask | VisibilityChangeMask The patch shows different behavior than xvimagesink when event handling is active, as described in this Python example: https://gist.github.com/lubosz/956002b007650915221a Basically things are broken when event handling is turned on. I left it as default, but it maybe should not be set as default. Setting custom Gdk mouse cursors are broken with sink.handle_events (True). Also the mouse motion event is only grabbed when dragging (a mouse button was clicked), and the button event is set. In the glimagesink _ensure_gl_setup needs to be called, so the context is initialized before manipulating the event handling.
(In reply to comment #2) > Created an attachment (id=285893) [details] [review] > Implement gst_video_overlay_handle_events() > > The X11 implementations of xvimagesink and glimagesink are slightly different. > xvimagesink creates the X11 window with XCreateSimpleWindow. glimagesink needs > to abstract window systems with the context. The context has also different > windowing systems. In our case the GLX context creates a X11 window with > classic XCreateWindow. I was not able to use XCreateSimpleWindow for GLX. I > guess it needs the extra options. The major difference between them is the parenting dance we do with the user supplied window. That is we create a window with its parent being the user supplied window. As a result, we steal all the input events that may need to be propagated to the use supplied window. > I removed the event handle options from the GLX X11 window initialization and > added an option to set them later (heavily inspired by xvimagesink). > > This option needs to be carried though all the abstraction layers to the sink. I think not. The sink can extract the window from the context just fine using gst_gl_context_get_window(). > With this patch I can disable the event handling of the sink. > Even with event handling disabled I set the following events for the window to > work properly: > > StructureNotifyMask | ExposureMask | VisibilityChangeMask > > The patch shows different behavior than xvimagesink when event handling is > active, as described in this Python example: > > https://gist.github.com/lubosz/956002b007650915221a > > Basically things are broken when event handling is turned on. I left it as > default, but it maybe should not be set as default. Another related option for fixing this bug is to remove the parenting logic and just use the window handle the application provides without creating our own. This effectively removes our X event handling code from ever running when a user window is supplied, is equivalent to the xvimagesink code and properly solves bug #723529. Then _handle_events() only has any real effect when there is no application supplied window. I have a patch that does that however I need to finish testing it properly.
Review of attachment 285893 [details] [review]: In addition to the issues mentioned in the previous comment. Make the function names similar to the video overlay interface method. Something like gst_glimage_sink_set_handle_events or remove the 'set'. Up to you. Repeat for the other objects. Otherwise, it looks good.
Ping?
Matthew, any plans for this? Do you prefer to wait for Lubosz 's updated patch or go on with your patch that you mentioned in comment 3? I can work on it if neither of you have time (since i also introduced this), let me know.
I am also waiting for the patch Mathew mentioned, so I do not duplicate work. Besides renaming the methods, I also don't know what to change in particular. I would like a comment on that.
Created attachment 289498 [details] [review] glwindow_x11: don't parent the application provided window This is my current patch. It seems to flicker when resizing with gtk+ and I have no idea why.
Lubosz, I was waiting for your patch :). What's not covered by comment #3 and comment #4 ?
Created attachment 289580 [details] [review] glimagesink: implement gst_video_overlay_handle_events I removed the event handling from the context and added one method to the abtract window. Setting event handling still is unimplemented for Android, Windows etc. Also renamed everything to "handle_events", like in GstOverlay. Unfortunately there is a method called "handle_event" in gstglwindow_x11.
Created attachment 289581 [details] [review] satisfy gst-indent Had some issues with gst-indent
Review of attachment 289580 [details] [review]: I have a couple of questions and comments below. ::: ext/gl/gstglimagesink.c @@ +987,3 @@ + GstGLWindow *window; + window = gst_gl_context_get_window (glimage_sink->context); + gst_gl_window_handle_events (window, handle_events); Need to gst_object_unref the window when done. ::: ext/gl/gstglimagesink.h @@ +68,3 @@ + gboolean handle_events; + + GMutex flow_lock; Why the lock? It only seems to protect multiple _handle_events() calls ::: gst-libs/gst/gl/gstglwindow.c @@ +941,3 @@ +void +gst_gl_window_handle_events (GstGLWindow * window, gboolean handle_events) +{ This should be a vfunc on the GstWindowClass that is called when it exists. Like gst_gl_window_get_window_handle except the subclass should only be called if it exists. ::: gst-libs/gst/gl/x11/gstglwindow_x11.h @@ +104,3 @@ gint gst_gl_window_x11_untrap_x_errors (void); +void gst_gl_window_x11_handle_events (GstGLWindowX11 * window_x11, + gboolean handle_events); There is no real reason to expose this after the move to the vfunc.
Created attachment 289888 [details] [review] glimagesink: implement gst_video_overlay_handle_events Remove lock, unref window, use virtual function.
commit 6702811e1408dfc6d7b1275d16a25d6c64b8a17e Author: Lubosz Sarnecki <lubosz@gmail.com> Date: Wed Oct 29 12:24:16 2014 +0100 glimagesink: implement gst_video_overlay_handle_events https://bugzilla.gnome.org/show_bug.cgi?id=736035