> 
> Currently when gstreamer is used to decode a full-screen
> stream sent from the server, the decoding frames are being
> forced to RBGA format and pushed using appsink to be scaled
> and rendered to screen.
> 
> Today most of the gstreamer sinks supports the GstVideoOverlay
> interface which allows to render directly from the pipeline to
> a window by a given windowing system id (i.e. xid). This patch
> makes playbin to use this feature if possible.

Got some time to test, works correctly. Finally I manage to test wayland
and different resolutions/settings.

I have some just minor changes (attached).

Another change to use the new flag would be (didn't still test it):


diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 6a90a78..c44f94e 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -98,6 +98,7 @@ gboolean gstvideo_has_codec(int codec_type);
 typedef struct display_surface {
     guint32                     surface_id;
     bool                        primary;
+    bool                        streaming_mode;
     enum SpiceSurfaceFmt        format;
     int                         width, height, stride, size;
     uint8_t                     *data;
diff --git a/src/channel-display.c b/src/channel-display.c
index a58119d..7d639b5 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1292,9 +1292,7 @@ static display_stream *display_stream_create(SpiceChannel 
*channel,
 #endif
     default:
 #ifdef HAVE_GSTVIDEO
-        if (c->primary->width == (dest->right - dest->left) &&
-            c->primary->height == (dest->bottom - dest->top)) {
-            SPICE_DEBUG("It seems to be a FULL-SCREEN stream");
+        if (c->primary->streaming_mode) {
             /* stream area location is sent but currently not used */
             g_coroutine_signal_emit(channel, 
signals[SPICE_DISPLAY_STREAM_AREA], 0,
                                     dest->left, dest->top,
@@ -1829,9 +1827,10 @@ static void display_handle_surface_create(SpiceChannel 
*channel, SpiceMsgIn *in)
     surface->height = create->height;
     surface->stride = create->width * 4;
     surface->size   = surface->height * surface->stride;
+    surface->streaming_mode = !!(create->flags & 
SPICE_SURFACE_FLAGS_STREAMING_MODE);

     if (create->flags & SPICE_SURFACE_FLAGS_PRIMARY) {
-        SPICE_DEBUG("primary flags: %x", create->flags);
+        SPICE_DEBUG("surface flags: %x", create->flags);
         surface->primary = true;
         create_canvas(channel, surface);
         if (c->mark_false_event_id != 0) {


Frediano

> ---
>  src/channel-display-gst.c | 71
>  +++++++++++++++++++++++++++++++++++++----------
>  src/channel-display.c     | 54 +++++++++++++++++++++++++++++++++++
>  src/spice-widget.c        | 46 ++++++++++++++++++++++++++++++
>  3 files changed, 156 insertions(+), 15 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index c6280d3..6fa19e0 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -265,7 +265,8 @@ static void free_pipeline(SpiceGstDecoder *decoder)
>  
>      gst_element_set_state(decoder->pipeline, GST_STATE_NULL);
>      gst_object_unref(decoder->appsrc);
> -    gst_object_unref(decoder->appsink);
> +    if (decoder->appsink)
> +        gst_object_unref(decoder->appsink);
>      gst_object_unref(decoder->pipeline);
>      gst_object_unref(decoder->clock);
>      decoder->pipeline = NULL;
> @@ -308,7 +309,18 @@ static gboolean handle_pipeline_message(GstBus *bus,
> GstMessage *msg, gpointer v
>          break;
>      }
>      default:
> -        /* not being handled */
> +        if (gst_is_video_overlay_prepare_window_handle_message(msg)) {
> +            GstVideoOverlay *overlay;
> +            guintptr handle = 0;
> +
> +            g_object_get(decoder->base.stream->channel, "handle", &handle,
> NULL);
> +            SPICE_DEBUG("prepare-window-handle msg received (handle: %lu)",
> handle);
> +            if (handle != 0) {
> +                overlay = GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg));
> +                gst_video_overlay_set_window_handle(overlay, handle);
> +            }
> +        }
> +        /* else- not being handled */
>          break;
>      }
>      return TRUE;
> @@ -348,6 +360,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>  #if GST_CHECK_VERSION(1,9,0)
>      GstElement *playbin, *sink;
>      SpiceGstPlayFlags flags;
> +    guintptr handle;
>      GstCaps *caps;
>  
>      playbin = gst_element_factory_make("playbin", "playbin");
> @@ -356,26 +369,53 @@ static gboolean create_pipeline(SpiceGstDecoder
> *decoder)
>          return FALSE;
>      }
>  
> -    sink = gst_element_factory_make("appsink", "sink");
> -    if (sink == NULL) {
> -        spice_warning("error upon creation of 'appsink' element");
> -        gst_object_unref(playbin);
> -        return FALSE;
> -    }
> -
> -    caps = gst_caps_from_string("video/x-raw,format=BGRx");
> -    g_object_set(sink,
> +    g_object_get(decoder->base.stream->channel, "handle", &handle, NULL);
> +    SPICE_DEBUG("Creating Gstreamer pipline (handle for overlay %s)\n",
> +                handle ? "received" : "not received");
> +    if (handle == 0) {
> +        sink = gst_element_factory_make("appsink", "sink");
> +        if (sink == NULL) {
> +            spice_warning("error upon creation of 'appsink' element");
> +            gst_object_unref(playbin);
> +            return FALSE;
> +        }
> +        caps = gst_caps_from_string("video/x-raw,format=BGRx");
> +        g_object_set(sink,
>                   "caps", caps,
>                   "sync", FALSE,
>                   "drop", FALSE,
>                   NULL);
> -    gst_caps_unref(caps);
> +        gst_caps_unref(caps);
> +        g_object_set(playbin,
> +                 "video-sink", gst_object_ref(sink),
> +                 NULL);
> +
> +        decoder->appsink = GST_APP_SINK(sink);
> +    } else {
> +     /*
> +      * handle has received, it means playbin will render directly into
> +      * widget using the gstoverlay interface instead of app-sink.
> +      * Also avoid using vaapisink if exist since vaapisink could be
> +      * buggy when it is combined with playbin. changing its rank to
> +      * none will make playbin to avoid of using it.
> +      */
> +        GstRegistry *registry = NULL;
> +        GstPluginFeature *vaapisink = NULL;
> +
> +        registry = gst_registry_get ();
> +        if (registry) {
> +            vaapisink = gst_registry_lookup_feature(registry, "vaapisink");
> +        }
> +        if (vaapisink) {
> +            gst_plugin_feature_set_rank(vaapisink, GST_RANK_NONE);
> +            gst_object_unref(vaapisink);
> +        }
> +    }
>  
>      g_signal_connect(playbin, "source-setup", G_CALLBACK(app_source_setup),
>      decoder);
>  
>      g_object_set(playbin,
>                   "uri", "appsrc://",
> -                 "video-sink", gst_object_ref(sink),
>                   NULL);
>  
>      /* Disable audio in playbin */
> @@ -384,7 +424,6 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>      g_object_set(playbin, "flags", flags, NULL);
>  
>      g_warn_if_fail(decoder->appsrc == NULL);
> -    decoder->appsink = GST_APP_SINK(sink);
>      decoder->pipeline = playbin;
>  #else
>      gchar *desc;
> @@ -417,7 +456,9 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>  #endif
>  
>      appsink_cbs.new_sample = new_sample;
> -    gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs, decoder,
> NULL);
> +    if (decoder->appsink) {
> +        gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs, decoder,
> NULL);
> +    }
>      bus = gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline));
>      gst_bus_add_watch(bus, handle_pipeline_message, decoder);
>      gst_object_unref(bus);
> diff --git a/src/channel-display.c b/src/channel-display.c
> index b41093b..8e63764 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -70,6 +70,7 @@ struct _SpiceDisplayChannelPrivate {
>      GArray                      *monitors;
>      guint                       monitors_max;
>      gboolean                    enable_adaptive_streaming;
> +    guintptr                    handle;
>      SpiceGlScanout scanout;
>  };
>  
> @@ -83,6 +84,7 @@ enum {
>      PROP_MONITORS,
>      PROP_MONITORS_MAX,
>      PROP_GL_SCANOUT,
> +    PROP_HANDLE,
>  };
>  
>  enum {
> @@ -91,6 +93,7 @@ enum {
>      SPICE_DISPLAY_INVALIDATE,
>      SPICE_DISPLAY_MARK,
>      SPICE_DISPLAY_GL_DRAW,
> +    SPICE_DISPLAY_STREAM_AREA,
>  
>      SPICE_DISPLAY_LAST_SIGNAL,
>  };
> @@ -227,6 +230,10 @@ static void spice_display_get_property(GObject
> *object,
>          g_value_set_static_boxed(value,
>          spice_display_channel_get_gl_scanout(channel));
>          break;
>      }
> +    case PROP_HANDLE: {
> +        g_value_set_ulong(value, c->handle);
> +        break;
> +    }
>      default:
>          G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>          break;
> @@ -238,7 +245,14 @@ static void spice_display_set_property(GObject
> *object,
>                                         const GValue *value,
>                                         GParamSpec   *pspec)
>  {
> +    SpiceDisplayChannel *channel = SPICE_DISPLAY_CHANNEL(object);
> +    SpiceDisplayChannelPrivate *c = channel->priv;
> +
>      switch (prop_id) {
> +    case PROP_HANDLE: {
> +         c->handle = g_value_get_ulong(value);
> +         break;
> +    }
>      default:
>          G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>          break;
> @@ -338,6 +352,16 @@ static void
> spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
>                              G_PARAM_READABLE |
>                              G_PARAM_STATIC_STRINGS));
>  
> +    g_object_class_install_property
> +         (gobject_class, PROP_HANDLE,
> +         g_param_spec_ulong("handle",
> +                            "Display handle",
> +                            "Display handle",
> +                            0, G_MAXULONG, 0,
> +                            G_PARAM_WRITABLE |
> +                            G_PARAM_READABLE |
> +                            G_PARAM_STATIC_STRINGS));
> +
>      /**
>       * SpiceDisplayChannel::display-primary-create:
>       * @display: the #SpiceDisplayChannel that emitted the signal
> @@ -454,6 +478,27 @@ static void
> spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
>                       4,
>                       G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT);
>  
> +    /**
> +     * SpiceDisplayChannel::stream-area:
> +     * @display: the #SpiceDisplayChannel that emitted the signal
> +     * @x: x position
> +     * @y: y position
> +     * @width: width
> +     * @height: height
> +     *
> +     * The #SpiceDisplayChannel::stream-area signal is emitted when
> +     * the rectangular region x/y/w/h is known as an area of a
> +     * video stream to be received.
> +     **/
> +    signals[SPICE_DISPLAY_STREAM_AREA] =
> +        g_signal_new("stream-area",
> +                     G_OBJECT_CLASS_TYPE(gobject_class),
> +                     0, 0, NULL, NULL,
> +                     g_cclosure_user_marshal_VOID__UINT_UINT_UINT_UINT,
> +                     G_TYPE_NONE,
> +                     4,
> +                     G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT);
> +
>      g_type_class_add_private(klass, sizeof(SpiceDisplayChannelPrivate));
>  
>      channel_set_handlers(SPICE_CHANNEL_CLASS(klass));
> @@ -1261,6 +1306,15 @@ static display_stream
> *display_stream_create(SpiceChannel *channel, uint32_t sur
>  #endif
>      default:
>  #ifdef HAVE_GSTVIDEO
> +        if (c->primary->width == (dest->right - dest->left) &&
> +            c->primary->height == (dest->bottom - dest->top)) {
> +            SPICE_DEBUG("It seems to be a FULL-SCREEN stream");
> +            /* stream area location is sent but currently not used */
> +            g_coroutine_signal_emit(channel,
> signals[SPICE_DISPLAY_STREAM_AREA], 0,
> +                                    dest->left, dest->top,
> +                                    c->primary->width, c->primary->height);
> +        //TODO: may not finished before creating decoder? also there is no
> fallback, assuming success
> +        }
>          st->video_decoder = create_gstreamer_decoder(codec_type, st);
>  #endif
>          break;
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 1e7add4..73a77b7 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -612,6 +612,29 @@ G_GNUC_END_IGNORE_DEPRECATIONS
>  #endif
>  #endif
>  
> +static void
> +gst_area_realize(GtkGLArea *area, gpointer user_data)
> +{
> +//TODO: needs rework, currently works only under X
> +#ifdef GDK_WINDOWING_X11
> +    SpiceDisplay *display = SPICE_DISPLAY(user_data);
> +    SpiceDisplayPrivate *d = display->priv;
> +    GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(display));
> +
> +    if (window) {
> +#if GTK_CHECK_VERSION(2,18,0)
> +        if (!gdk_window_ensure_native (window)) {
> +            g_warning("Couldn't create native window needed for
> GstVideoOverlay!");
> +            return;
> +        }
> +#endif
> +        g_object_set(G_OBJECT (d->display),
> +                    "handle", GDK_WINDOW_XID(window),
> +                    NULL);
> +    }
> +#endif
> +}
> +
>  static void
>  drawing_area_realize(GtkWidget *area, gpointer user_data)
>  {
> @@ -660,6 +683,13 @@ G_GNUC_BEGIN_IGNORE_DEPRECATIONS
>  G_GNUC_END_IGNORE_DEPRECATIONS
>  #endif
>  #endif
> +
> +    area = gtk_drawing_area_new();
> +    g_object_connect(area,
> +                     "signal::realize", gst_area_realize, display,
> +                     NULL);
> +    gtk_stack_add_named(d->stack, area, "gst-area");
> +
>      gtk_widget_show_all(widget);
>  
>      g_signal_connect(display, "grab-broken-event", G_CALLBACK(grab_broken),
>      NULL);
> @@ -2589,6 +2619,20 @@ static void queue_draw_area(SpiceDisplay *display,
> gint x, gint y,
>                                 x, y, width, height);
>  }
>  
> +static void pop_stream_area(SpiceChannel *channel, guint x, guint y,
> +                            guint width, guint height, gpointer data)
> +{
> +/* Currently it works only in X11 environment - do not set area visible if
> it's not X11 */
> +#ifdef GDK_WINDOWING_X11
> +    SpiceDisplay *display = data;
> +    SpiceDisplayPrivate *d = display->priv;
> +
> +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
> +        gtk_stack_set_visible_child_name(d->stack, "gst-area");
> +    }
> +#endif
> +}
> +
>  static void invalidate(SpiceChannel *channel,
>                         gint x, gint y, gint w, gint h, gpointer data)
>  {
> @@ -2953,6 +2997,8 @@ static void channel_new(SpiceSession *s, SpiceChannel
> *channel, gpointer data)
>          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, "stream-area",
> +                                      G_CALLBACK(pop_stream_area), display,
> 0);
>          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);
From 2e9c47fdb7d2c75912c6ca1274406d4d1954a006 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fzig...@redhat.com>
Date: Fri, 6 Apr 2018 13:40:46 +0100
Subject: [PATCH spice-gtk 4/4] fixup! Gstreamer: Use GstVideoOverlay if
 possible

Add version information to a comment.
Useful ??
---
 src/channel-display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/channel-display.c b/src/channel-display.c
index e96e75a..a58119d 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -488,6 +488,8 @@ static void spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
      * The #SpiceDisplayChannel::stream-area signal is emitted when
      * the rectangular region x/y/w/h is known as an area of a
      * video stream to be received.
+     *
+     * Since: 0.35
      **/
     signals[SPICE_DISPLAY_STREAM_AREA] =
         g_signal_new("stream-area",
-- 
2.14.3

From 391bf2d7d6f12bc4d74465eefa959de9ff3bc0fb Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fzig...@redhat.com>
Date: Fri, 6 Apr 2018 13:40:28 +0100
Subject: [PATCH spice-gtk 3/4] fixup! Gstreamer: Use GstVideoOverlay if
 possible

Use pointer type for handle property.
The property is stored/retrieved as guintptr so ulong is not
always enough.
---
 src/channel-display.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/channel-display.c b/src/channel-display.c
index 75dd910..e96e75a 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -70,7 +70,7 @@ struct _SpiceDisplayChannelPrivate {
     GArray                      *monitors;
     guint                       monitors_max;
     gboolean                    enable_adaptive_streaming;
-    guintptr                    handle;
+    gpointer                    handle;
     SpiceGlScanout scanout;
 };
 
@@ -231,7 +231,7 @@ static void spice_display_get_property(GObject    *object,
         break;
     }
     case PROP_HANDLE: {
-        g_value_set_ulong(value, c->handle);
+        g_value_set_pointer(value, c->handle);
         break;
     }
     default:
@@ -250,7 +250,7 @@ static void spice_display_set_property(GObject      *object,
 
     switch (prop_id) {
     case PROP_HANDLE: {
-         c->handle = g_value_get_ulong(value);
+         c->handle = g_value_get_pointer(value);
          break;
     }
     default:
@@ -354,13 +354,12 @@ static void spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
 
     g_object_class_install_property
          (gobject_class, PROP_HANDLE,
-         g_param_spec_ulong("handle",
-                            "Display handle",
-                            "Display handle",
-                            0, G_MAXULONG, 0,
-                            G_PARAM_WRITABLE |
-                            G_PARAM_READABLE |
-                            G_PARAM_STATIC_STRINGS));
+         g_param_spec_pointer("handle",
+                              "Display handle",
+                              "Display handle",
+                              G_PARAM_WRITABLE |
+                              G_PARAM_READABLE |
+                              G_PARAM_STATIC_STRINGS));
 
     /**
      * SpiceDisplayChannel::display-primary-create:
-- 
2.14.3

From c4c04d3cf53ace0fbc8c48dc4a9e99059293a00a Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fzig...@redhat.com>
Date: Fri, 6 Apr 2018 13:39:13 +0100
Subject: [PATCH spice-gtk 2/4] fixup! Gstreamer: Use GstVideoOverlay if
 possible

Fix format for guintptr type
---
 src/channel-display-gst.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index b180c27..70aac51 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -313,7 +313,7 @@ static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer v
             guintptr handle = 0;
 
             g_object_get(decoder->base.stream->channel, "handle", &handle, NULL);
-            SPICE_DEBUG("prepare-window-handle msg received (handle: %lu)", handle);
+            SPICE_DEBUG("prepare-window-handle msg received (handle: %" PRIuPTR ")", handle);
             if (handle != 0) {
                 overlay = GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg));
                 gst_video_overlay_set_window_handle(overlay, handle);
-- 
2.14.3

From 43f2d4a9eef6854f5bb8139a4e6d85cdb720ff5d Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fzig...@redhat.com>
Date: Fri, 6 Apr 2018 13:37:06 +0100
Subject: [PATCH spice-gtk 1/4] fixup! Gstreamer: Use GstVideoOverlay if
 possible

Fix some spacing
---
 src/channel-display-gst.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 65f41cf..b180c27 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -391,17 +391,17 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
 
         decoder->appsink = GST_APP_SINK(sink);
     } else {
-     /*
-      * handle has received, it means playbin will render directly into
-      * widget using the gstoverlay interface instead of app-sink.
-      * Also avoid using vaapisink if exist since vaapisink could be
-      * buggy when it is combined with playbin. changing its rank to
-      * none will make playbin to avoid of using it.
-      */
+        /*
+         * handle has received, it means playbin will render directly into
+         * widget using the gstoverlay interface instead of app-sink.
+         * Also avoid using vaapisink if exist since vaapisink could be
+         * buggy when it is combined with playbin. changing its rank to
+         * none will make playbin to avoid of using it.
+         */
         GstRegistry *registry = NULL;
         GstPluginFeature *vaapisink = NULL;
 
-        registry = gst_registry_get ();
+        registry = gst_registry_get();
         if (registry) {
             vaapisink = gst_registry_lookup_feature(registry, "vaapisink");
         }
-- 
2.14.3

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Reply via email to