[spice-gtk] Gstreamer: Control GstVideoOverlay from the widget

Submitted by Snir Sheriber on Dec. 9, 2018, 1:26 p.m.

Details

Message ID 20181209132630.19915-1-ssheribe@redhat.com
State New
Headers show
Series "Gstreamer: Control GstVideoOverlay from the widget" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Snir Sheriber Dec. 9, 2018, 1:26 p.m.
This patch is changing the way gstvideooverlay is being set.
Once pipeline is created a pointer is passed to the widget using
GObject signal, so we can set there the overlay interface and call
its functions from widget callbacks. By doing that, issues like
resizing the window were solved.

Signed-off-by: Snir Sheriber <ssheribe@redhat.com>
---

This patch is replacing the streaming-mode signal but this signal was never documented
so it should not be an issue.

---
 src/channel-display-gst.c  |  24 ++------
 src/channel-display-priv.h |   1 +
 src/channel-display.c      |  35 ++++++------
 src/spice-marshal.txt      |   2 +-
 src/spice-widget-priv.h    |   8 +++
 src/spice-widget.c         | 111 +++++++++++++++++++++++++++++++++----
 6 files changed, 135 insertions(+), 46 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 2c07f35..f079132 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -334,20 +334,6 @@  static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer v
         g_free(filename);
         break;
     }
-    case GST_MESSAGE_ELEMENT: {
-        if (gst_is_video_overlay_prepare_window_handle_message(msg)) {
-            GstVideoOverlay *overlay;
-
-            SPICE_DEBUG("prepare-window-handle msg received (handle: %" G_GUINTPTR_FORMAT")",
-                        decoder->win_handle);
-            if (decoder->win_handle != 0) {
-                overlay = GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg));
-                gst_video_overlay_set_window_handle(overlay, decoder->win_handle);
-                gst_video_overlay_handle_events(overlay, false);
-            }
-        }
-        break;
-    }
     default:
         /* not being handled */
         break;
@@ -396,11 +382,11 @@  static gboolean create_pipeline(SpiceGstDecoder *decoder)
         return FALSE;
     }
 
-    /* Will try to get window handle in order to apply the GstVideoOverlay
-     * interface, setting overlay to this window will happen only when
-     * prepare-window-handle message is received
+    /* Passing the pipeline to widget, try to get window handle and
+     * set the GstVideoOverlay interface, setting overlay to the window
+     * will happen only when prepare-window-handle message is received
      */
-    decoder->win_handle = get_window_handle(decoder->base.stream);
+    decoder->win_handle = hand_pipeline_to_widget(decoder->base.stream, GST_PIPELINE(playbin));
     SPICE_DEBUG("Creating Gstreamer pipeline (handle for overlay %s)\n",
                 decoder->win_handle ? "received" : "not received");
     if (decoder->win_handle == 0) {
@@ -576,7 +562,7 @@  static void spice_gst_decoder_destroy(VideoDecoder *video_decoder)
  * 3) As soon as the GStreamer pipeline no longer needs the compressed frame it
  *    will call frame->unref_data() to free it.
  *
- * If GstVideoOverlay is used (win_handle was obtained by pipeline creation):
+ * If GstVideoOverlay is used (win_handle was obtained successfully):
  *   4) Decompressed frames will be renderd to widget directly from gstreamer's pipeline
  *      using some gstreamer sink plugin which implements the GstVideoOverlay interface
  *      (last step).
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index c1b3fe5..29804b4 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -202,6 +202,7 @@  void stream_dropped_frame_on_playback(display_stream *st);
 #define SPICE_UNKNOWN_STRIDE 0
 void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t width, uint32_t height, int stride, uint8_t* data);
 guintptr get_window_handle(display_stream *st);
+guintptr hand_pipeline_to_widget(display_stream *st, gpointer pipeline);
 
 
 G_END_DECLS
diff --git a/src/channel-display.c b/src/channel-display.c
index 7c663cb..e4d9df9 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -88,7 +88,7 @@  enum {
     SPICE_DISPLAY_INVALIDATE,
     SPICE_DISPLAY_MARK,
     SPICE_DISPLAY_GL_DRAW,
-    SPICE_DISPLAY_STREAMING_MODE,
+    SPICE_DISPLAY_OVERLAY,
 
     SPICE_DISPLAY_LAST_SIGNAL,
 };
@@ -453,26 +453,27 @@  static void spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
                      G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT);
 
     /**
-     * SpiceDisplayChannel::streaming-mode:
+     * SpiceDisplayChannel::gst-video-overlay
      * @display: the #SpiceDisplayChannel that emitted the signal
-     * @streaming_mode: %TRUE when it's streaming mode
+     * @pipeline: pointer to gstreamer's pipeline
      *
-     * Return: handle for the display window if possible
+     * Return: valid window handle on success
      *
-     * The #SpiceDisplayChannel::streaming-mode signal is emitted when
-     * spice server is working in streaming mode.
+     * The #SpiceDisplayChannel::gst-video-overlay signal is emitted when
+     * pipeline is ready and can be passed to widget to regeister gstreamer
+     * overlay interface and other gstreamer callbacks.
      *
-     * Since 0.35
+     * Since 0.36
      **/
-    signals[SPICE_DISPLAY_STREAMING_MODE] =
-        g_signal_new("streaming-mode",
+    signals[SPICE_DISPLAY_OVERLAY] =
+        g_signal_new("gst-video-overlay",
                      G_OBJECT_CLASS_TYPE(gobject_class),
                      0, 0,
                      NULL, NULL,
-                     g_cclosure_user_marshal_POINTER__BOOLEAN,
+                     g_cclosure_user_marshal_POINTER__POINTER,
                      G_TYPE_POINTER,
                      1,
-                     G_TYPE_BOOLEAN);
+                     G_TYPE_POINTER);
 
     channel_set_handlers(SPICE_CHANNEL_CLASS(klass));
 }
@@ -1391,13 +1392,15 @@  void stream_display_frame(display_stream *st, SpiceFrame *frame,
     }
 }
 
-guintptr get_window_handle(display_stream *st)
+guintptr hand_pipeline_to_widget(display_stream *st, gpointer pipeline)
 {
-   void* handle = 0;
+    void* handle = NULL;
 
-   g_signal_emit(st->channel, signals[SPICE_DISPLAY_STREAMING_MODE], 0,
-                 st->surface->streaming_mode, &handle);
-   return st->surface->streaming_mode ? (guintptr)handle : 0;
+    if (st->surface->streaming_mode) {
+        g_signal_emit(st->channel, signals[SPICE_DISPLAY_OVERLAY], 0,
+                      pipeline, &handle);
+    }
+    return (guintptr)handle;
 }
 
 /* after a sequence of 3 drops, push a report to the server, even
diff --git a/src/spice-marshal.txt b/src/spice-marshal.txt
index b3a8e69..58a2b0e 100644
--- a/src/spice-marshal.txt
+++ b/src/spice-marshal.txt
@@ -13,4 +13,4 @@  BOOLEAN:UINT,POINTER,UINT
 BOOLEAN:UINT,UINT
 VOID:OBJECT,OBJECT
 VOID:BOXED,BOXED
-POINTER:BOOLEAN
+POINTER:POINTER
diff --git a/src/spice-widget-priv.h b/src/spice-widget-priv.h
index 96f6c1d..81bb089 100644
--- a/src/spice-widget-priv.h
+++ b/src/spice-widget-priv.h
@@ -32,6 +32,10 @@ 
 #include "spice-common.h"
 #include "spice-gtk-session.h"
 
+#ifdef HAVE_GSTVIDEO
+#include <gst/video/videooverlay.h>
+#endif
+
 G_BEGIN_DECLS
 
 #define DISPLAY_DEBUG(display, fmt, ...) \
@@ -149,6 +153,10 @@  struct _SpiceDisplayPrivate {
     } egl;
 #endif // HAVE_EGL
     double scroll_delta_y;
+#ifdef HAVE_GSTVIDEO
+    GstVideoOverlay         *overlay;
+    GstElement              *pipeline;
+#endif
 };
 
 int      spice_cairo_image_create                 (SpiceDisplay *display);
diff --git a/src/spice-widget.c b/src/spice-widget.c
index 9bb4221..7ec17cb 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -120,6 +120,10 @@  static void size_allocate(GtkWidget *widget, GtkAllocation *conf, gpointer data)
 static gboolean draw_event(GtkWidget *widget, cairo_t *cr, gpointer data);
 static void update_size_request(SpiceDisplay *display);
 static GdkDevice *spice_gdk_window_get_pointing_device(GdkWindow *window);
+#ifdef HAVE_GSTVIDEO
+void gst_size_allocate(GtkWidget *widget, GdkRectangle *a, gpointer data);
+static gboolean gst_draw_event(GtkWidget *widget, cairo_t *cr, gpointer data);
+#endif
 
 /* ---------------------------------------------------------------- */
 
@@ -649,8 +653,14 @@  static void spice_display_init(SpiceDisplay *display)
                      NULL);
     gtk_stack_add_named(d->stack, area, "gl-area");
 #endif
+#ifdef HAVE_GSTVIDEO
     area = gtk_drawing_area_new();
     gtk_stack_add_named(d->stack, area, "gst-area");
+    g_object_connect(area,
+                     "signal::draw", gst_draw_event, display,
+                     "signal::size-allocate", gst_size_allocate, display,
+                     NULL);
+#endif
 
     gtk_widget_show_all(widget);
 
@@ -2115,10 +2125,23 @@  static void realize(GtkWidget *widget)
 
 static void unrealize(GtkWidget *widget)
 {
-    spice_cairo_image_destroy(SPICE_DISPLAY(widget));
+    SpiceDisplay *display = SPICE_DISPLAY(widget);
+
+    spice_cairo_image_destroy(display);
 #if HAVE_EGL
-    if (SPICE_DISPLAY(widget)->priv->egl.context_ready)
-        spice_egl_unrealize_display(SPICE_DISPLAY(widget));
+    if (display->priv->egl.context_ready)
+        spice_egl_unrealize_display(display);
+#endif
+#ifdef HAVE_GSTVIDEO
+    SpiceDisplayPrivate *d = display->priv;
+
+    if (d->overlay)
+        gst_object_unref(d->overlay);
+    d->overlay = NULL;
+
+    if (d->pipeline)
+        gst_object_unref(d->pipeline);
+    d->pipeline = NULL;
 #endif
 
     GTK_WIDGET_CLASS(spice_display_parent_class)->unrealize(widget);
@@ -2547,21 +2570,89 @@  static void queue_draw_area(SpiceDisplay *display, gint x, gint y,
                                x, y, width, height);
 }
 
-static void* prepare_streaming_mode(SpiceChannel *channel, bool streaming_mode, gpointer data)
+#if defined(HAVE_GSTVIDEO) && defined(GDK_WINDOWING_X11)
+static GstBusSyncReply handle_pipeline_message_sync(GstBus *bus, GstMessage *msg, gpointer data)
 {
-#ifdef GDK_WINDOWING_X11
+    switch(GST_MESSAGE_TYPE(msg)) {
+    case GST_MESSAGE_ELEMENT: {
+        if (gst_is_video_overlay_prepare_window_handle_message(msg)) {
+            if (!g_getenv("DISABLE_GSTVIDEOOVERLAY") &&
+                GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
+                SpiceDisplay *display = data;
+                GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(display));
+
+                if (window && gdk_window_ensure_native(window)) {
+                    SpiceDisplayPrivate *d = display->priv;
+
+                    d->overlay = gst_object_ref(GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg)));
+                    gst_video_overlay_set_window_handle(d->overlay, (uintptr_t)GDK_WINDOW_XID(window)); // moving to widget
+                    gst_video_overlay_handle_events(d->overlay, false);
+                    return GST_BUS_DROP;
+                }
+            }
+        }
+        break;
+    }
+    default:
+        /* not being handled */
+        break;
+    }
+    return GST_BUS_PASS;
+}
+#endif
+
+#ifdef HAVE_GSTVIDEO
+static gboolean gst_draw_event(GtkWidget *widget, cairo_t *cr, gpointer data)
+{
+    SpiceDisplay *display = SPICE_DISPLAY(data);
+    SpiceDisplayPrivate *d = display->priv;
+    g_return_val_if_fail(d != NULL, false);
+
+    if (d->overlay) {
+        gst_video_overlay_expose(d->overlay);
+        update_mouse_pointer(display);
+        return true;
+    }
+    return false;
+}
+
+void gst_size_allocate(GtkWidget *widget, GdkRectangle *a, gpointer data)
+{
+    SpiceDisplay *display = SPICE_DISPLAY(data);
+    SpiceDisplayPrivate *d = display->priv;
+
+    if (d->overlay) {
+        gint scale = 1;
+
+        scale = gtk_widget_get_scale_factor(widget);
+        gst_video_overlay_set_render_rectangle(d->overlay, a->x * scale, a->y * scale, a->width * scale, a->height * scale);
+    }
+}
+#endif
+
+/* This callback should pass to the widget a pointer of the pipeline
+ * so that we can set pipeline and overlay related calls from here.
+ */
+static void* set_overlay(SpiceChannel *channel, void* pipeline_ptr, gpointer data)
+{
+#if defined(HAVE_GSTVIDEO) && defined(GDK_WINDOWING_X11)
     SpiceDisplay *display = data;
     SpiceDisplayPrivate *d = display->priv;
 
-    /* GstVideoOverlay will currently be used only under x */
+    /* GstVideoOverlay is currently used only under x */
     if (!g_getenv("DISABLE_GSTVIDEOOVERLAY") &&
-        streaming_mode &&
         GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
         GdkWindow *window;
 
+        gtk_stack_set_visible_child_name(d->stack, "gst-area");
         window = gtk_widget_get_window(GTK_WIDGET(display));
         if (window && gdk_window_ensure_native(window)) {
-            gtk_stack_set_visible_child_name(d->stack, "gst-area");
+            GstBus *bus;
+
+            d->pipeline = GST_ELEMENT(gst_object_ref(pipeline_ptr));
+            bus = gst_pipeline_get_bus(GST_PIPELINE(d->pipeline));
+            gst_bus_set_sync_handler (bus, (GstBusSyncHandler) handle_pipeline_message_sync, data, NULL);
+            gst_object_unref(bus);
             return (void*)(uintptr_t)GDK_WINDOW_XID(window);
         }
     }
@@ -2936,8 +3027,8 @@  static void channel_new(SpiceSession *s, SpiceChannel *channel, SpiceDisplay *di
         spice_g_signal_connect_object(channel, "notify::monitors",
                                       G_CALLBACK(spice_display_widget_update_monitor_area),
                                       display, G_CONNECT_AFTER | G_CONNECT_SWAPPED);
-        spice_g_signal_connect_object(channel, "streaming-mode",
-                                      G_CALLBACK(prepare_streaming_mode), display, G_CONNECT_AFTER);
+        spice_g_signal_connect_object(channel, "gst-video-overlay",
+                                      G_CALLBACK(set_overlay), display, G_CONNECT_AFTER);
         if (spice_display_channel_get_primary(channel, 0, &primary)) {
             primary_create(channel, primary.format, primary.width, primary.height,
                            primary.stride, primary.shmid, primary.data, display);

Comments

Hey,

On Sun, Dec 09, 2018 at 03:26:30PM +0200, Snir Sheriber wrote:
> This patch is changing the way gstvideooverlay is being set.
> Once pipeline is created a pointer is passed to the widget using
> GObject signal, so we can set there the overlay interface and call
> its functions from widget callbacks. By doing that, issues like
> resizing the window were solved.
> 
> Signed-off-by: Snir Sheriber <ssheribe@redhat.com>
> ---
> 
> This patch is replacing the streaming-mode signal but this signal was never documented
> so it should not be an issue.
> 
> ---
>  src/channel-display-gst.c  |  24 ++------
>  src/channel-display-priv.h |   1 +
>  src/channel-display.c      |  35 ++++++------
>  src/spice-marshal.txt      |   2 +-
>  src/spice-widget-priv.h    |   8 +++
>  src/spice-widget.c         | 111 +++++++++++++++++++++++++++++++++----
>  6 files changed, 135 insertions(+), 46 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 2c07f35..f079132 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -334,20 +334,6 @@ static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer v
>          g_free(filename);
>          break;
>      }
> -    case GST_MESSAGE_ELEMENT: {
> -        if (gst_is_video_overlay_prepare_window_handle_message(msg)) {
> -            GstVideoOverlay *overlay;
> -
> -            SPICE_DEBUG("prepare-window-handle msg received (handle: %" G_GUINTPTR_FORMAT")",
> -                        decoder->win_handle);
> -            if (decoder->win_handle != 0) {
> -                overlay = GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg));
> -                gst_video_overlay_set_window_handle(overlay, decoder->win_handle);
> -                gst_video_overlay_handle_events(overlay, false);
> -            }
> -        }
> -        break;
> -    }
>      default:
>          /* not being handled */
>          break;
> @@ -396,11 +382,11 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>          return FALSE;
>      }
>  
> -    /* Will try to get window handle in order to apply the GstVideoOverlay
> -     * interface, setting overlay to this window will happen only when
> -     * prepare-window-handle message is received
> +    /* Passing the pipeline to widget, try to get window handle and
> +     * set the GstVideoOverlay interface, setting overlay to the window
> +     * will happen only when prepare-window-handle message is received
>       */
> -    decoder->win_handle = get_window_handle(decoder->base.stream);
> +    decoder->win_handle = hand_pipeline_to_widget(decoder->base.stream, GST_PIPELINE(playbin));

As far as I can tell, you don't really need the value of the win_handle,
you only need to know whether spice-gtk is going to use an overlay or
not, so hand_pipeline_to_widget could probably just return a boolean.

> @@ -453,26 +453,27 @@ static void spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
>                       G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT);
>  
>      /**
> -     * SpiceDisplayChannel::streaming-mode:
> +     * SpiceDisplayChannel::gst-video-overlay
>       * @display: the #SpiceDisplayChannel that emitted the signal
> -     * @streaming_mode: %TRUE when it's streaming mode
> +     * @pipeline: pointer to gstreamer's pipeline
>       *
> -     * Return: handle for the display window if possible
> +     * Return: valid window handle on success
>       *
> -     * The #SpiceDisplayChannel::streaming-mode signal is emitted when
> -     * spice server is working in streaming mode.
> +     * The #SpiceDisplayChannel::gst-video-overlay signal is emitted when
> +     * pipeline is ready and can be passed to widget to regeister gstreamer
> +     * overlay interface and other gstreamer callbacks.
>       *
> -     * Since 0.35
> +     * Since 0.36
>       **/
> -    signals[SPICE_DISPLAY_STREAMING_MODE] =
> -        g_signal_new("streaming-mode",
> +    signals[SPICE_DISPLAY_OVERLAY] =
> +        g_signal_new("gst-video-overlay",
>                       G_OBJECT_CLASS_TYPE(gobject_class),
>                       0, 0,
>                       NULL, NULL,
> -                     g_cclosure_user_marshal_POINTER__BOOLEAN,
> +                     g_cclosure_user_marshal_POINTER__POINTER,
>                       G_TYPE_POINTER,
>                       1,
> -                     G_TYPE_BOOLEAN);
> +                     G_TYPE_POINTER);

This should be GST_TYPE_PIPELINE.

>  
>      channel_set_handlers(SPICE_CHANNEL_CLASS(klass));
>  }
> @@ -1391,13 +1392,15 @@ void stream_display_frame(display_stream *st, SpiceFrame *frame,
>      }
>  }
>  
> -guintptr get_window_handle(display_stream *st)
> +guintptr hand_pipeline_to_widget(display_stream *st, gpointer pipeline)
>  {
> -   void* handle = 0;
> +    void* handle = NULL;
>  
> -   g_signal_emit(st->channel, signals[SPICE_DISPLAY_STREAMING_MODE], 0,
> -                 st->surface->streaming_mode, &handle);
> -   return st->surface->streaming_mode ? (guintptr)handle : 0;
> +    if (st->surface->streaming_mode) {
> +        g_signal_emit(st->channel, signals[SPICE_DISPLAY_OVERLAY], 0,
> +                      pipeline, &handle);
> +    }
> +    return (guintptr)handle;
>  }
>  
>  /* after a sequence of 3 drops, push a report to the server, even
> @@ -2115,10 +2125,23 @@ static void realize(GtkWidget *widget)
>  
>  static void unrealize(GtkWidget *widget)
>  {
> -    spice_cairo_image_destroy(SPICE_DISPLAY(widget));
> +    SpiceDisplay *display = SPICE_DISPLAY(widget);
> +
> +    spice_cairo_image_destroy(display);
>  #if HAVE_EGL
> -    if (SPICE_DISPLAY(widget)->priv->egl.context_ready)
> -        spice_egl_unrealize_display(SPICE_DISPLAY(widget));
> +    if (display->priv->egl.context_ready)
> +        spice_egl_unrealize_display(display);
> +#endif
> +#ifdef HAVE_GSTVIDEO
> +    SpiceDisplayPrivate *d = display->priv;
> +
> +    if (d->overlay)
> +        gst_object_unref(d->overlay);
> +    d->overlay = NULL;
> +
> +    if (d->pipeline)
> +        gst_object_unref(d->pipeline);
> +    d->pipeline = NULL;

This could be

g_clear_pointer(&d->overlay, gst_object_unref);
g_clear_pointer(&d->pipeline, gst_object_unref);



>  #endif
>  
>      GTK_WIDGET_CLASS(spice_display_parent_class)->unrealize(widget);
> @@ -2547,21 +2570,89 @@ static void queue_draw_area(SpiceDisplay *display, gint x, gint y,
>                                 x, y, width, height);
>  }
>  
> -static void* prepare_streaming_mode(SpiceChannel *channel, bool streaming_mode, gpointer data)
> +#if defined(HAVE_GSTVIDEO) && defined(GDK_WINDOWING_X11)
> +static GstBusSyncReply handle_pipeline_message_sync(GstBus *bus, GstMessage *msg, gpointer data)
>  {
> -#ifdef GDK_WINDOWING_X11
> +    switch(GST_MESSAGE_TYPE(msg)) {
> +    case GST_MESSAGE_ELEMENT: {
> +        if (gst_is_video_overlay_prepare_window_handle_message(msg)) {
> +            if (!g_getenv("DISABLE_GSTVIDEOOVERLAY") &&
> +                GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
> +                SpiceDisplay *display = data;
> +                GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(display));
> +
> +                if (window && gdk_window_ensure_native(window)) {
> +                    SpiceDisplayPrivate *d = display->priv;
> +
> +                    d->overlay = gst_object_ref(GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg)));
> +                    gst_video_overlay_set_window_handle(d->overlay, (uintptr_t)GDK_WINDOW_XID(window)); // moving to widget
> +                    gst_video_overlay_handle_events(d->overlay, false);
> +                    return GST_BUS_DROP;
> +                }
> +            }
> +        }
> +        break;
> +    }
> +    default:
> +        /* not being handled */
> +        break;
> +    }
> +    return GST_BUS_PASS;
> +}
> +#endif
> +
> +#ifdef HAVE_GSTVIDEO
> +static gboolean gst_draw_event(GtkWidget *widget, cairo_t *cr, gpointer data)
> +{
> +    SpiceDisplay *display = SPICE_DISPLAY(data);
> +    SpiceDisplayPrivate *d = display->priv;
> +    g_return_val_if_fail(d != NULL, false);
> +
> +    if (d->overlay) {
> +        gst_video_overlay_expose(d->overlay);
> +        update_mouse_pointer(display);
> +        return true;
> +    }
> +    return false;
> +}
> +
> +void gst_size_allocate(GtkWidget *widget, GdkRectangle *a, gpointer data)
> +{
> +    SpiceDisplay *display = SPICE_DISPLAY(data);
> +    SpiceDisplayPrivate *d = display->priv;
> +
> +    if (d->overlay) {
> +        gint scale = 1;
> +
> +        scale = gtk_widget_get_scale_factor(widget);
> +        gst_video_overlay_set_render_rectangle(d->overlay, a->x * scale, a->y * scale, a->width * scale, a->height * scale);
> +    }
> +}

I'm under the impression these 2 callbacks could be added in a follow-up
patch? Or is there going to be regression in spice-gtk behaviour if they
are not part of the patch adding the gst-video-overlay signal?

Christophe

> +#endif
> +
> +/* This callback should pass to the widget a pointer of the pipeline
> + * so that we can set pipeline and overlay related calls from here.
> + */
> +static void* set_overlay(SpiceChannel *channel, void* pipeline_ptr, gpointer data)
> +{
> +#if defined(HAVE_GSTVIDEO) && defined(GDK_WINDOWING_X11)
>      SpiceDisplay *display = data;
>      SpiceDisplayPrivate *d = display->priv;
>  
> -    /* GstVideoOverlay will currently be used only under x */
> +    /* GstVideoOverlay is currently used only under x */
>      if (!g_getenv("DISABLE_GSTVIDEOOVERLAY") &&
> -        streaming_mode &&
>          GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
>          GdkWindow *window;
>  
> +        gtk_stack_set_visible_child_name(d->stack, "gst-area");
>          window = gtk_widget_get_window(GTK_WIDGET(display));
>          if (window && gdk_window_ensure_native(window)) {
> -            gtk_stack_set_visible_child_name(d->stack, "gst-area");
> +            GstBus *bus;
> +
> +            d->pipeline = GST_ELEMENT(gst_object_ref(pipeline_ptr));
> +            bus = gst_pipeline_get_bus(GST_PIPELINE(d->pipeline));
> +            gst_bus_set_sync_handler (bus, (GstBusSyncHandler) handle_pipeline_message_sync, data, NULL);
> +            gst_object_unref(bus);
>              return (void*)(uintptr_t)GDK_WINDOW_XID(window);
>          }
>      }
> @@ -2936,8 +3027,8 @@ static void channel_new(SpiceSession *s, SpiceChannel *channel, SpiceDisplay *di
>          spice_g_signal_connect_object(channel, "notify::monitors",
>                                        G_CALLBACK(spice_display_widget_update_monitor_area),
>                                        display, G_CONNECT_AFTER | G_CONNECT_SWAPPED);
> -        spice_g_signal_connect_object(channel, "streaming-mode",
> -                                      G_CALLBACK(prepare_streaming_mode), display, G_CONNECT_AFTER);
> +        spice_g_signal_connect_object(channel, "gst-video-overlay",
> +                                      G_CALLBACK(set_overlay), display, G_CONNECT_AFTER);
>          if (spice_display_channel_get_primary(channel, 0, &primary)) {
>              primary_create(channel, primary.format, primary.width, primary.height,
>                             primary.stride, primary.shmid, primary.data, display);
> -- 
> 2.19.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> This patch is changing the way gstvideooverlay is being set.

I would use the camelcase GstVideoOverlay, not strong about it.

> Once pipeline is created a pointer is passed to the widget using
> GObject signal, so we can set there the overlay interface and call
> its functions from widget callbacks. By doing that, issues like
> resizing the window were solved.

I think more exactly that this new interface allows to fix the
resize, not that solve by itself.

> 
> Signed-off-by: Snir Sheriber <ssheribe@redhat.com>
> ---
> 
> This patch is replacing the streaming-mode signal but this signal was never
> documented
> so it should not be an issue.
> 

Agreed

> ---
>  src/channel-display-gst.c  |  24 ++------
>  src/channel-display-priv.h |   1 +
>  src/channel-display.c      |  35 ++++++------
>  src/spice-marshal.txt      |   2 +-
>  src/spice-widget-priv.h    |   8 +++
>  src/spice-widget.c         | 111 +++++++++++++++++++++++++++++++++----
>  6 files changed, 135 insertions(+), 46 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 2c07f35..f079132 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -334,20 +334,6 @@ static gboolean handle_pipeline_message(GstBus *bus,
> GstMessage *msg, gpointer v
>          g_free(filename);
>          break;
>      }
> -    case GST_MESSAGE_ELEMENT: {
> -        if (gst_is_video_overlay_prepare_window_handle_message(msg)) {
> -            GstVideoOverlay *overlay;
> -
> -            SPICE_DEBUG("prepare-window-handle msg received (handle: %"
> G_GUINTPTR_FORMAT")",
> -                        decoder->win_handle);
> -            if (decoder->win_handle != 0) {
> -                overlay = GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg));
> -                gst_video_overlay_set_window_handle(overlay,
> decoder->win_handle);
> -                gst_video_overlay_handle_events(overlay, false);
> -            }
> -        }
> -        break;
> -    }
>      default:
>          /* not being handled */
>          break;
> @@ -396,11 +382,11 @@ static gboolean create_pipeline(SpiceGstDecoder
> *decoder)
>          return FALSE;
>      }
>  
> -    /* Will try to get window handle in order to apply the GstVideoOverlay
> -     * interface, setting overlay to this window will happen only when
> -     * prepare-window-handle message is received
> +    /* Passing the pipeline to widget, try to get window handle and
> +     * set the GstVideoOverlay interface, setting overlay to the window
> +     * will happen only when prepare-window-handle message is received
>       */
> -    decoder->win_handle = get_window_handle(decoder->base.stream);
> +    decoder->win_handle = hand_pipeline_to_widget(decoder->base.stream,
> GST_PIPELINE(playbin));
>      SPICE_DEBUG("Creating Gstreamer pipeline (handle for overlay %s)\n",
>                  decoder->win_handle ? "received" : "not received");
>      if (decoder->win_handle == 0) {
> @@ -576,7 +562,7 @@ static void spice_gst_decoder_destroy(VideoDecoder
> *video_decoder)
>   * 3) As soon as the GStreamer pipeline no longer needs the compressed frame
>   it
>   *    will call frame->unref_data() to free it.
>   *
> - * If GstVideoOverlay is used (win_handle was obtained by pipeline
> creation):
> + * If GstVideoOverlay is used (win_handle was obtained successfully):
>   *   4) Decompressed frames will be renderd to widget directly from
>   gstreamer's pipeline
>   *      using some gstreamer sink plugin which implements the
>   GstVideoOverlay interface
>   *      (last step).
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index c1b3fe5..29804b4 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -202,6 +202,7 @@ void stream_dropped_frame_on_playback(display_stream
> *st);
>  #define SPICE_UNKNOWN_STRIDE 0
>  void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
>  width, uint32_t height, int stride, uint8_t* data);
>  guintptr get_window_handle(display_stream *st);
> +guintptr hand_pipeline_to_widget(display_stream *st, gpointer pipeline);
>  
>  
>  G_END_DECLS
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 7c663cb..e4d9df9 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -88,7 +88,7 @@ enum {
>      SPICE_DISPLAY_INVALIDATE,
>      SPICE_DISPLAY_MARK,
>      SPICE_DISPLAY_GL_DRAW,
> -    SPICE_DISPLAY_STREAMING_MODE,
> +    SPICE_DISPLAY_OVERLAY,
>  
>      SPICE_DISPLAY_LAST_SIGNAL,
>  };
> @@ -453,26 +453,27 @@ static void
> spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
>                       G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT);
>  
>      /**
> -     * SpiceDisplayChannel::streaming-mode:
> +     * SpiceDisplayChannel::gst-video-overlay
>       * @display: the #SpiceDisplayChannel that emitted the signal
> -     * @streaming_mode: %TRUE when it's streaming mode
> +     * @pipeline: pointer to gstreamer's pipeline
>       *
> -     * Return: handle for the display window if possible
> +     * Return: valid window handle on success
>       *
> -     * The #SpiceDisplayChannel::streaming-mode signal is emitted when
> -     * spice server is working in streaming mode.
> +     * The #SpiceDisplayChannel::gst-video-overlay signal is emitted when
> +     * pipeline is ready and can be passed to widget to regeister gstreamer
> +     * overlay interface and other gstreamer callbacks.
>       *
> -     * Since 0.35
> +     * Since 0.36
>       **/
> -    signals[SPICE_DISPLAY_STREAMING_MODE] =
> -        g_signal_new("streaming-mode",
> +    signals[SPICE_DISPLAY_OVERLAY] =
> +        g_signal_new("gst-video-overlay",
>                       G_OBJECT_CLASS_TYPE(gobject_class),
>                       0, 0,
>                       NULL, NULL,
> -                     g_cclosure_user_marshal_POINTER__BOOLEAN,
> +                     g_cclosure_user_marshal_POINTER__POINTER,
>                       G_TYPE_POINTER,
>                       1,
> -                     G_TYPE_BOOLEAN);
> +                     G_TYPE_POINTER);
>  
>      channel_set_handlers(SPICE_CHANNEL_CLASS(klass));
>  }
> @@ -1391,13 +1392,15 @@ void stream_display_frame(display_stream *st,
> SpiceFrame *frame,
>      }
>  }
>  
> -guintptr get_window_handle(display_stream *st)
> +guintptr hand_pipeline_to_widget(display_stream *st, gpointer pipeline)
>  {
> -   void* handle = 0;
> +    void* handle = NULL;
>  
> -   g_signal_emit(st->channel, signals[SPICE_DISPLAY_STREAMING_MODE], 0,
> -                 st->surface->streaming_mode, &handle);
> -   return st->surface->streaming_mode ? (guintptr)handle : 0;
> +    if (st->surface->streaming_mode) {
> +        g_signal_emit(st->channel, signals[SPICE_DISPLAY_OVERLAY], 0,
> +                      pipeline, &handle);
> +    }
> +    return (guintptr)handle;
>  }
>  
>  /* after a sequence of 3 drops, push a report to the server, even
> diff --git a/src/spice-marshal.txt b/src/spice-marshal.txt
> index b3a8e69..58a2b0e 100644
> --- a/src/spice-marshal.txt
> +++ b/src/spice-marshal.txt
> @@ -13,4 +13,4 @@ BOOLEAN:UINT,POINTER,UINT
>  BOOLEAN:UINT,UINT
>  VOID:OBJECT,OBJECT
>  VOID:BOXED,BOXED
> -POINTER:BOOLEAN
> +POINTER:POINTER
> diff --git a/src/spice-widget-priv.h b/src/spice-widget-priv.h
> index 96f6c1d..81bb089 100644
> --- a/src/spice-widget-priv.h
> +++ b/src/spice-widget-priv.h
> @@ -32,6 +32,10 @@
>  #include "spice-common.h"
>  #include "spice-gtk-session.h"
>  
> +#ifdef HAVE_GSTVIDEO
> +#include <gst/video/videooverlay.h>
> +#endif
> +
>  G_BEGIN_DECLS
>  
>  #define DISPLAY_DEBUG(display, fmt, ...) \
> @@ -149,6 +153,10 @@ struct _SpiceDisplayPrivate {
>      } egl;
>  #endif // HAVE_EGL
>      double scroll_delta_y;
> +#ifdef HAVE_GSTVIDEO
> +    GstVideoOverlay         *overlay;
> +    GstElement              *pipeline;

style: no space indentation

> +#endif
>  };
>  
>  int      spice_cairo_image_create                 (SpiceDisplay *display);
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 9bb4221..7ec17cb 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -120,6 +120,10 @@ static void size_allocate(GtkWidget *widget,
> GtkAllocation *conf, gpointer data)
>  static gboolean draw_event(GtkWidget *widget, cairo_t *cr, gpointer data);
>  static void update_size_request(SpiceDisplay *display);
>  static GdkDevice *spice_gdk_window_get_pointing_device(GdkWindow *window);
> +#ifdef HAVE_GSTVIDEO
> +void gst_size_allocate(GtkWidget *widget, GdkRectangle *a, gpointer data);

Why not static? Looks a mistake if not declared in an header

> +static gboolean gst_draw_event(GtkWidget *widget, cairo_t *cr, gpointer
> data);
> +#endif
>  
>  /* ---------------------------------------------------------------- */
>  
> @@ -649,8 +653,14 @@ static void spice_display_init(SpiceDisplay *display)
>                       NULL);
>      gtk_stack_add_named(d->stack, area, "gl-area");
>  #endif
> +#ifdef HAVE_GSTVIDEO
>      area = gtk_drawing_area_new();
>      gtk_stack_add_named(d->stack, area, "gst-area");
> +    g_object_connect(area,
> +                     "signal::draw", gst_draw_event, display,
> +                     "signal::size-allocate", gst_size_allocate, display,
> +                     NULL);
> +#endif
>  
>      gtk_widget_show_all(widget);
>  
> @@ -2115,10 +2125,23 @@ static void realize(GtkWidget *widget)
>  
>  static void unrealize(GtkWidget *widget)
>  {
> -    spice_cairo_image_destroy(SPICE_DISPLAY(widget));
> +    SpiceDisplay *display = SPICE_DISPLAY(widget);
> +
> +    spice_cairo_image_destroy(display);
>  #if HAVE_EGL
> -    if (SPICE_DISPLAY(widget)->priv->egl.context_ready)
> -        spice_egl_unrealize_display(SPICE_DISPLAY(widget));
> +    if (display->priv->egl.context_ready)
> +        spice_egl_unrealize_display(display);
> +#endif
> +#ifdef HAVE_GSTVIDEO
> +    SpiceDisplayPrivate *d = display->priv;
> +
> +    if (d->overlay)
> +        gst_object_unref(d->overlay);
> +    d->overlay = NULL;
> +
> +    if (d->pipeline)
> +        gst_object_unref(d->pipeline);
> +    d->pipeline = NULL;
>  #endif
>  
>      GTK_WIDGET_CLASS(spice_display_parent_class)->unrealize(widget);
> @@ -2547,21 +2570,89 @@ static void queue_draw_area(SpiceDisplay *display,
> gint x, gint y,
>                                 x, y, width, height);
>  }
>  
> -static void* prepare_streaming_mode(SpiceChannel *channel, bool
> streaming_mode, gpointer data)
> +#if defined(HAVE_GSTVIDEO) && defined(GDK_WINDOWING_X11)
> +static GstBusSyncReply handle_pipeline_message_sync(GstBus *bus, GstMessage
> *msg, gpointer data)
>  {
> -#ifdef GDK_WINDOWING_X11
> +    switch(GST_MESSAGE_TYPE(msg)) {
> +    case GST_MESSAGE_ELEMENT: {
> +        if (gst_is_video_overlay_prepare_window_handle_message(msg)) {
> +            if (!g_getenv("DISABLE_GSTVIDEOOVERLAY") &&
> +                GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
> +                SpiceDisplay *display = data;
> +                GdkWindow *window =
> gtk_widget_get_window(GTK_WIDGET(display));
> +
> +                if (window && gdk_window_ensure_native(window)) {
> +                    SpiceDisplayPrivate *d = display->priv;
> +
> +                    d->overlay =
> gst_object_ref(GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg)));

If overlay is not NULL we'll have a leak as reference is not
decremented.
Looking at the pointers looks like channels owns both the pipeline and
(indirectly) the widget(s) so maybe would be better to have a weak
reference here instead of a strong one.
If the stream is closed and the pipeline is released from the channel
there's no point to keep the pipeline in the widget.

> +                    gst_video_overlay_set_window_handle(d->overlay,
> (uintptr_t)GDK_WINDOW_XID(window)); // moving to widget
> +                    gst_video_overlay_handle_events(d->overlay, false);
> +                    return GST_BUS_DROP;
> +                }
> +            }
> +        }
> +        break;
> +    }
> +    default:
> +        /* not being handled */
> +        break;
> +    }
> +    return GST_BUS_PASS;
> +}
> +#endif
> +
> +#ifdef HAVE_GSTVIDEO
> +static gboolean gst_draw_event(GtkWidget *widget, cairo_t *cr, gpointer
> data)
> +{
> +    SpiceDisplay *display = SPICE_DISPLAY(data);
> +    SpiceDisplayPrivate *d = display->priv;
> +    g_return_val_if_fail(d != NULL, false);

We already used and dereferenced display, if this is NULL we have
a memory corruption, handling with a warning is just creating a security
issue! Either abort or remove the check.

> +
> +    if (d->overlay) {
> +        gst_video_overlay_expose(d->overlay);
> +        update_mouse_pointer(display);
> +        return true;
> +    }
> +    return false;
> +}
> +
> +void gst_size_allocate(GtkWidget *widget, GdkRectangle *a, gpointer data)
> +{
> +    SpiceDisplay *display = SPICE_DISPLAY(data);
> +    SpiceDisplayPrivate *d = display->priv;
> +
> +    if (d->overlay) {
> +        gint scale = 1;
> +
> +        scale = gtk_widget_get_scale_factor(widget);
> +        gst_video_overlay_set_render_rectangle(d->overlay, a->x * scale,
> a->y * scale, a->width * scale, a->height * scale);
> +    }
> +}
> +#endif
> +
> +/* This callback should pass to the widget a pointer of the pipeline
> + * so that we can set pipeline and overlay related calls from here.
> + */
> +static void* set_overlay(SpiceChannel *channel, void* pipeline_ptr, gpointer
> data)
> +{
> +#if defined(HAVE_GSTVIDEO) && defined(GDK_WINDOWING_X11)
>      SpiceDisplay *display = data;
>      SpiceDisplayPrivate *d = display->priv;
>  
> -    /* GstVideoOverlay will currently be used only under x */
> +    /* GstVideoOverlay is currently used only under x */
>      if (!g_getenv("DISABLE_GSTVIDEOOVERLAY") &&
> -        streaming_mode &&
>          GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
>          GdkWindow *window;
>  
> +        gtk_stack_set_visible_child_name(d->stack, "gst-area");

Why do you thing the call is better outside the if?
There's no explanation in the commit message for this.

>          window = gtk_widget_get_window(GTK_WIDGET(display));
>          if (window && gdk_window_ensure_native(window)) {
> -            gtk_stack_set_visible_child_name(d->stack, "gst-area");
> +            GstBus *bus;
> +
> +            d->pipeline = GST_ELEMENT(gst_object_ref(pipeline_ptr));

Same leak problem of the overlay.

> +            bus = gst_pipeline_get_bus(GST_PIPELINE(d->pipeline));
> +            gst_bus_set_sync_handler (bus, (GstBusSyncHandler)
> handle_pipeline_message_sync, data, NULL);

Cast to GstBusSyncHandler is useless here.
Maybe would be better to use gst_bus_enable_sync_message_emission and
connect the signal instead so to leave to spice-glib also the possibility
to handle some of these messages if needed?

> +            gst_object_unref(bus);
>              return (void*)(uintptr_t)GDK_WINDOW_XID(window);
>          }
>      }
> @@ -2936,8 +3027,8 @@ static void channel_new(SpiceSession *s, SpiceChannel
> *channel, SpiceDisplay *di
>          spice_g_signal_connect_object(channel, "notify::monitors",
>                                        G_CALLBACK(spice_display_widget_update_monitor_area),
>                                        display, G_CONNECT_AFTER |
>                                        G_CONNECT_SWAPPED);
> -        spice_g_signal_connect_object(channel, "streaming-mode",
> -                                      G_CALLBACK(prepare_streaming_mode),
> display, G_CONNECT_AFTER);
> +        spice_g_signal_connect_object(channel, "gst-video-overlay",
> +                                      G_CALLBACK(set_overlay), display,
> G_CONNECT_AFTER);
>          if (spice_display_channel_get_primary(channel, 0, &primary)) {
>              primary_create(channel, primary.format, primary.width,
>              primary.height,
>                             primary.stride, primary.shmid, primary.data,
>                             display);

Frediano