GNOME Bugzilla – Bug 703486
Implement GstNavigation interface in glimagesink
Last modified: 2014-11-08 14:14:34 UTC
See summary, should be easy enough for the X11 and Wayland backends... more complicated for RPi and Android (once they're there).
Created attachment 279822 [details] [review] gstglwindow: Add methods to get backend input event source and set an input event callback
Created attachment 279823 [details] [review] glimagesink: Add gstnavigation interface
Created attachment 279824 [details] [review] x11, gstglwindow: Add handling of key / mouse events for navigation
I made a first attempt at adding gstnavigation interface (only x11 backend). For now only tested with gst-launch videotestsrc ! navigationtest ! glimagesink If the approach is acceptable, I can also add wayland backend support.
Why do you add this input event source API? It looks like it should all work without that too, by just having the GstGLWindow know about the sink instance via different means. Otherwise this looks good, thanks for the patch :)
Yes, get_input_event_source / gst_gl_window_set_input_event_callback API methods are not necessary, I just use them so that GstGLWindow knows about the sink instance. Do you recommend a simpler way to do this?
Maybe GstGLWindow could have a signal that is emitted whenever a navigation event happens, and glimagesink (or any other use of GstGLWindow) can connect to that and then do whatever is necessary, e.g. sending the navigation event into the pipeline. This signal could be just taking a GstEvent, but that seems weird. So maybe > void mouse-event(const char *event, int button, double x, double y) > void key-event(const char *event, const char *key) just like the API to create GstEvents from these values. What do you think?
sounds reasonable, I 'll look into it.
Created attachment 279917 [details] [review] GstGLWindow : Add mouse-event and key-event signals for navigation
Created attachment 279918 [details] [review] glimagesink: Add navigation interface and callbacks for GstGLWindow mouse/key signals
Created attachment 279919 [details] [review] GstGLWindow_x11 : Emit signals for mouse and key navigation events
v2 with signals. I 've used integers for posx, posy of mouse events (backends e.g. xmotionevent and wl_pointer have integers for surface coordinates) which are then cast to doubles for the navigation event creation. Also, currently I use the same signal both for mouse button clicks and mouse cursor movement. But these can be changed of course.
Review of attachment 279917 [details] [review]: Looks reasonable. ::: gst-libs/gst/gl/gstglwindow.c @@ +115,3 @@ +}; + +static guint gst_glwindow_signals[LAST_SIGNAL] = { 0 }; gst_gl_window_signals :) @@ +154,3 @@ + * + */ + gst_glwindow_signals[EVENT_MOUSE_SIGNAL] = same @@ +168,3 @@ + * + */ + gst_glwindow_signals[EVENT_KEY_SIGNAL] = same
Review of attachment 279918 [details] [review]: Some minor nitpicks :). Otherwise looks good ::: ext/gl/gstglimagesink.c @@ +312,3 @@ gstvideosink_class->show_frame = GST_DEBUG_FUNCPTR (gst_glimage_sink_show_frame); + spurious new line. @@ +423,3 @@ + *key_string, GstGLImageSink * gl_sink) +{ + GST_DEBUG ("glimagesink event %s key %s pressed", event_name, key_string); GST_DEBUG_OBJECT (gl_sink, ...) please. @@ +429,3 @@ + +static void +gst_glimage_sink_mouse_event_cb (GstGLImageSink * window, char *event_name, (GstGLWindow * window, ... ? @@ +432,3 @@ + gint button, int posx, int posy, GstGLImageSink * gl_sink) +{ + GST_DEBUG ("glimagesink event %s at %d, %d", event_name, posx, posy, gl_sink); GST_DEBUG_OBJECT (gl_sink, ...) here as well
Review of attachment 279919 [details] [review]: Some minor nitpicks ::: gst-libs/gst/gl/x11/gstglwindow_x11.c @@ +176,3 @@ g_source_attach (window_x11->x11_source, window_x11->main_context); + spurious new line @@ +254,3 @@ &text_property, &text_property, 0, 0, NULL, &wm_hints, NULL); + spurious new line @@ +593,3 @@ + g_signal_emit_by_name (window, "mouse-event", + event.type == + KeyPress ? "mouse-button-press" : "mouse-button-release", s/KeyPress/ButtonPress/
(In reply to comment #12) > v2 with signals. I 've used integers for posx, posy of mouse events (backends > e.g. xmotionevent and wl_pointer have integers for surface coordinates) which > are then cast to doubles for the navigation event creation. Also, currently I > use the same signal both for mouse button clicks and mouse cursor movement. I would prefer to base the signal type on the navigation interface so they should be doubles. The double up of mouse signals looks ok.
(In reply to comment #16) > I would prefer to base the signal type on the navigation interface so they > should be doubles. The double up of mouse signals looks ok. Ack
Review of attachment 279918 [details] [review]: ::: ext/gl/gstglimagesink.c @@ +197,3 @@ + + if (GST_IS_PAD (pad) && GST_IS_EVENT (event)) + gst_pad_send_event (pad, event); gst_pad_push_event (GST_VIDEO_SINK_PAD (sink), event); @@ +481,3 @@ + (gst_glimage_sink_key_event_cb), gl_sink); + g_signal_connect (window, "mouse-event", G_CALLBACK + (gst_glimage_sink_mouse_event_cb), gl_sink); You probably want to disconnect the signal handlers again when the sink instance is disposed
Review of attachment 279919 [details] [review]: ::: gst-libs/gst/gl/x11/gstglwindow_x11.c @@ +591,3 @@ + GST_DEBUG ("input event mouse button %d pressed over window at %d,%d", + event.xbutton.button, event.xbutton.x, event.xbutton.y); + g_signal_emit_by_name (window, "mouse-event", Don't emit by name but use the ids you got from g_signal_new(). As they are private to the base class, add functions to the base class: gst_gl_window_send_mouse_event(); gst_gl_window_send_key_event(); or somtehing like that. Which mirrors the navigation interface API again
Created attachment 280034 [details] [review] GstGLWindow : Add mouse-event and key-event signals for navigation
Created attachment 280035 [details] [review] glimagesink: Add navigation interface and callbacks for GstGLWindow mouse/key signals
Created attachment 280036 [details] [review] GstGLWindow_x11 : Emit signals for mouse and key navigation events
v3 with some fixes. - Double arguments to mouse event signal - signals now emitted with id, from base class methods. Signal handlers are disconnected in gst_glimage_sink_on_close. NOt sure this is the best place for handler disconnect. - nitpicks fixed (extra lines, GST_DEBUG_OBJECT, gl_window naming, ButtonPress) Another issue: On window resize shouldn't the navigation GstStructures be rescaled to original video dimensions? See xvimagesink's gst_xvimagesink_navigation_send_event for an example. I have an example patch below doing this, but it requires a new GstGLWindow class method to get the backend window dimensions (called get_window_dimensions). Let me know if you think we need this re-scaling, and also if there is a better approach to the problem.
Created attachment 280037 [details] [review] GstGLWindow, GstGLImagesink, x11: Scale navigation events on resized windows
commit 90a4444609b24bc3cd56cf385effffb49bd58793 Author: Vasilis Liaskovitis <vliaskov@gmail.com> Date: Mon Jul 7 00:20:01 2014 +0300 glimagesink: Add navigation interface and callbacks for GstGLWindow mouse/key signals https://bugzilla.gnome.org/show_bug.cgi?id=703486 commit 41632d486bd36b6b11c47d9c8b7835f05f2d0a62 Author: Vasilis Liaskovitis <vliaskov@gmail.com> Date: Sun Jul 6 23:39:47 2014 +0300 GstGLWindow : Add mouse-event and key-event signals for navigation https://bugzilla.gnome.org/show_bug.cgi?id=703486
Review of attachment 280036 [details] [review]: Looks good :) ::: gst-libs/gst/gl/x11/gstglwindow_x11.c @@ +481,3 @@ return "ClientMessage"; + case KeyPress: + return "KeyPressRelease"; "KeyPress" ? :)
Review of attachment 280037 [details] [review]: I like the sound of get_surface_dimensions marginally better. Various other API's are also transitioning towards the surface naming convention as well. ::: ext/gl/gstglimagesink.c @@ +203,3 @@ + event = gst_event_new_navigation (structure); + + window_class->get_window_dimensions (window, &width, &height); Should use a public GstGLWindow function instead of the vfunc. Will also need to check that the returned values are somewhat valid if the GstGLWindow subclass does not implement that function. ::: gst-libs/gst/gl/gstglwindow.h @@ +151,3 @@ guintptr gst_gl_window_get_display (GstGLWindow *window); + Spurious new line
Created attachment 281563 [details] [review] GstGLWindow_x11 : Emit signals for mouse and key navigation events Fixed keypress typo
Hmm, attachment 281563 [details] [review] fails to compile here with -Werror :) gstglwindow_x11.c: In function 'gst_gl_window_x11_handle_event': gstglwindow_x11.c:584:9: error: passing argument 2 of 'gst_gl_window_send_key_event' discards 'const' qualifier from pointer target type [-Werror] gst_gl_window_send_key_event (window, ^ In file included from ../../../../gst-libs/gst/gl/gl.h:36:0, from gstglwindow_x11.h:27, from x11_event_source.h:25, from gstglwindow_x11.c:31: ../../../../gst-libs/gst/gl/gstglwindow.h:156:6: note: expected 'char *' but argument is of type 'const char *' and so on for the other gst_gl_window_send_*_event calls.
Yeah, but not anymore :) commit eab38c5b2181809e3df6440c53be1d877d1def89 Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Jul 24 12:23:03 2014 +0200 glwindow: Constify string parameters to the send_*_event() functions
Comment on attachment 281563 [details] [review] GstGLWindow_x11 : Emit signals for mouse and key navigation events commit 05f908bf455b40a51277ccb1bd422aa65704f5ca Author: Vasilis Liaskovitis <vliaskov@gmail.com> Date: Thu Jul 24 12:25:36 2014 +0300 glwindow/11: Emit signals for mouse and key navigation events https://bugzilla.gnome.org/show_bug.cgi?id=703486
Created attachment 282444 [details] [review] GstGLWindow, GstGLImagesink, x11: Scale navigation events on resized windows Using a public GstGLWindow function get_surface_dimensions to get the actual width and height of surface. Default width and height are set to sink's GST_VIDEO_SINK_WIDTH and GST_VIDEO_SINK_HEIGHT in case the subclass does not implement the get_surface_dimensions vfunc, though I am not sure this is the sanest option.
Also some questions for wayland support: - mouse buttons clicks are handled with wl_pointer_listener 's button function (pointer_handle_button() in gstglwindow_wayland_egl client) . However as the wayland-client-protocol.h says "The location of the click is given by the last motion or enter event. ". So should we save some state in the gstglwindow_wayland_egl client in order for mouse clicks to have accurate position info? and where should that state be(e.g. in struct window?) - the keyboard_listener is disabled in gstglwindow_wayland_egl. Can i reenable it, and also send navigation key events, or is there a reason for it being disabled ?
(In reply to comment #33) > Also some questions for wayland support: > > - mouse buttons clicks are handled with wl_pointer_listener 's button function > (pointer_handle_button() in gstglwindow_wayland_egl client) . However as the > wayland-client-protocol.h says "The location of the click is given by the last > motion or enter event. ". So should we save some state in the > gstglwindow_wayland_egl client in order for mouse clicks to have accurate > position info? and where should that state be(e.g. in struct window?) Sure, wherever it fits. > - the keyboard_listener is disabled in gstglwindow_wayland_egl. Can i reenable > it, and also send navigation key events, or is there a reason for it being > disabled ? Feel free to enable, it was probably only disabled because nothing was using it.
Review of attachment 282444 [details] [review]: Just a couple of little things :) The GST_VIDEO_SINK_WIDTH/HEIGHT usage looks fine. ::: ext/gl/gstglimagesink.c @@ +210,3 @@ + if (width != GST_VIDEO_SINK_WIDTH (sink) && + gst_structure_get_double (structure, "pointer_x", &x)) { + xscale = (gdouble) GST_VIDEO_SINK_WIDTH (sink) / width; potential divide by zero. @@ +216,3 @@ + if (height != GST_VIDEO_SINK_HEIGHT (sink) && + gst_structure_get_double (structure, "pointer_y", &y)) { + yscale = (gdouble) GST_VIDEO_SINK_HEIGHT (sink) / height; potential divide by zero. ::: gst-libs/gst/gl/gstglwindow.c @@ +603,3 @@ +/** + * gst_gl_window_get_surface_dimensions missing : at the end @@ +606,3 @@ + * @window: a #GstGLWindow + * @width: place holder for width of surface + * @height: place holder for height of surface what about? @width: (out): resulting surface width @height: (out): resulting surface height @@ +607,3 @@ + * @width: place holder for width of surface + * @height: place holder for height of surface + * Returns: nothing "Returns:" Not needed for void functions
Created attachment 282704 [details] [review] GstGLWindow, GstGLImagesink, x11: Scale navigation events on resized windows Avoid possible divide by zero and fix gst_gl_window_get_surface_dimensions description
Comment on attachment 282704 [details] [review] GstGLWindow, GstGLImagesink, x11: Scale navigation events on resized windows commit 4dacc4ba5507c748f5d9665dd75d75b2999d5270 Author: Vasilis Liaskovitis <vliaskov@gmail.com> Date: Wed Aug 6 16:48:03 2014 +0300 GstGLWindow, GstGLImagesink, x11: Scale navigation events on resized windows If window is resized, GstStructure pointer values have to be rescaled to original geometry. A get_surface_dimensions GLWindow class method is added for this purpose and used in the navigation send_event function. https://bugzilla.gnome.org/show_bug.cgi?id=703486
This causes bug #736035