Re: [RFC spice-gtk v3 1/1] Gstreamer: Use GstVideoOverlay if possible

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,


On 05/15/2018 06:09 PM, Christophe Fergeau wrote:
On Wed, Apr 25, 2018 at 03:43:14PM +0300, Snir Sheriber wrote:
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.

Setting the DISABLE_GSTVIDEOOVERLAY environment variable will
make gstreamer to avoid of using the gstvideooverlay interface.
---
  src/channel-display-gst.c  | 92 +++++++++++++++++++++++++++++++++++-----------
  src/channel-display-priv.h |  2 +
  src/channel-display.c      | 35 ++++++++++++++++++
  src/channel-display.h      |  3 ++
  src/spice-marshal.txt      |  1 +
  src/spice-widget.c         | 33 +++++++++++++++++
  6 files changed, 145 insertions(+), 21 deletions(-)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 0d7aabb..8029e34 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -41,6 +41,8 @@ typedef struct SpiceGstDecoder {
      GstElement *pipeline;
      GstClock *clock;
+ guintptr win_handle;
+
      /* ---------- Decoding and display queues ---------- */
uint32_t last_mm_time;
@@ -265,7 +267,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;
@@ -306,6 +309,18 @@ 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: %" PRIuPTR ")", 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);
+            }
+        }
+        break;
+    }
      default:
          /* not being handled */
          break;
@@ -342,7 +357,6 @@ static void app_source_setup(GstElement *pipeline G_GNUC_UNUSED,
static gboolean create_pipeline(SpiceGstDecoder *decoder)
  {
-    GstAppSinkCallbacks appsink_cbs = { NULL };
      GstBus *bus;
  #if GST_CHECK_VERSION(1,9,0)
      GstElement *playbin, *sink;
@@ -355,26 +369,56 @@ 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,
+    /* 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
+     */
+    decoder->win_handle = get_window_handle(decoder->base.stream);
+    SPICE_DEBUG("Creating Gstreamer pipline (handle for overlay %s)\n",
+                decoder->win_handle ? "received" : "not received");
+    if (decoder->win_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 gstvideoooverlay 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);
+        }
+    }
This part seems to set some gstreamer global state, I don't think this
needs to be done every time "create_pipeline()" is called. This looks
like this could go in gstvideo_init().

Yes, could be done there too, it would set the vaapisink rank to NONE
also when gstvideooverlay is not used, but shouldn't harm since
appsink is used in that case.


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 */
[snip]

diff --git a/src/spice-widget.c b/src/spice-widget.c
index 73db593..251b29c 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
+static void* prepare_streaming_mode(SpiceChannel *channel, bool streaming_mode, gpointer data)
+{
+#ifdef GDK_WINDOWING_X11
+    SpiceDisplay *display = data;
+    SpiceDisplayPrivate *d = display->priv;
+
+    /* GstVideoOverlay will currently be used only under x */
+    if (!g_getenv("DISABLE_GSTVIDEOOVERLAY") &&
+        streaming_mode &&
+        GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
+        GdkWindow *window;
+
+        window = gtk_widget_get_window(GTK_WIDGET(display));
+        if (window) {
+#if GTK_CHECK_VERSION(2,18,0)
+            if (gdk_window_ensure_native (window))
+#endif
+            {
+                gtk_stack_set_visible_child_name(d->stack, "gst-area");
+                return (void*)GDK_WINDOW_XID(window);
+            }
+        }
+    }
+#endif
+    return NULL;
+}
+
  static void invalidate(SpiceChannel *channel,
                         gint x, gint y, gint w, gint h, gpointer data)
  {
@@ -2962,6 +2993,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, "streaming-mode",
+                                      G_CALLBACK(prepare_streaming_mode), display, G_CONNECT_AFTER);
Rather than having this signal which is used by ChannelDisplay to call a
method on a GtkWidget it should not have any access to, have you
considered adding code to spice-widget.c::realize() and
spice-widget.c::unrealize() which would call
channel_display_set_window_handle(display->priv->display, handle);
?

Sound a bit similar to a previous version I've tried, iiuc your
suggestion is to make widget call a function in channel_display
to set the handle (are we doing something like that already?).
I personally really like the current flow of the request for handle
using the signal and getting it as a response, avoiding of setting
and getting an handle from different components.

Snir.


Christophe

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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]