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 736035 - glimagesink: Regression: Gtk+ cannot get glimagesink input
glimagesink: Regression: Gtk+ cannot get glimagesink input
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other Linux
: Normal blocker
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-09-04 10:46 UTC by Lubosz Sarnecki
Modified: 2014-11-06 05:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Implement gst_video_overlay_handle_events() (10.22 KB, patch)
2014-09-11 09:18 UTC, Lubosz Sarnecki
needs-work Details | Review
glwindow_x11: don't parent the application provided window (8.77 KB, patch)
2014-10-28 08:21 UTC, Matthew Waters (ystreet00)
needs-work Details | Review
glimagesink: implement gst_video_overlay_handle_events (7.77 KB, patch)
2014-10-29 11:30 UTC, Lubosz Sarnecki
needs-work Details | Review
satisfy gst-indent (1.67 KB, patch)
2014-10-29 11:32 UTC, Lubosz Sarnecki
committed Details | Review
glimagesink: implement gst_video_overlay_handle_events (8.58 KB, patch)
2014-11-03 11:43 UTC, Lubosz Sarnecki
committed Details | Review

Description Lubosz Sarnecki 2014-09-04 10:46:17 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
Comment 1 Sebastian Dröge (slomo) 2014-09-04 11:10:11 UTC
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).
Comment 2 Lubosz Sarnecki 2014-09-11 09:18:37 UTC
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.
Comment 3 Matthew Waters (ystreet00) 2014-09-11 11:13:49 UTC
(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.
Comment 4 Matthew Waters (ystreet00) 2014-09-11 11:22:11 UTC
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.
Comment 5 Sebastian Dröge (slomo) 2014-09-17 17:25:29 UTC
Ping?
Comment 6 Vasilis Liaskovitis 2014-10-27 13:45:25 UTC
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.
Comment 7 Lubosz Sarnecki 2014-10-27 14:06:13 UTC
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.
Comment 8 Matthew Waters (ystreet00) 2014-10-28 08:21:18 UTC
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.
Comment 9 Matthew Waters (ystreet00) 2014-10-28 08:24:43 UTC
Lubosz, I was waiting for your patch :).  What's not covered by comment #3 and comment #4 ?
Comment 10 Lubosz Sarnecki 2014-10-29 11:30:37 UTC
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.
Comment 11 Lubosz Sarnecki 2014-10-29 11:32:10 UTC
Created attachment 289581 [details] [review]
satisfy gst-indent

Had some issues with gst-indent
Comment 12 Matthew Waters (ystreet00) 2014-10-29 12:34:50 UTC
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.
Comment 13 Lubosz Sarnecki 2014-11-03 11:43:58 UTC
Created attachment 289888 [details] [review]
glimagesink: implement gst_video_overlay_handle_events

Remove lock, unref window, use virtual function.
Comment 14 Matthew Waters (ystreet00) 2014-11-06 05:05:20 UTC
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