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/09/2018 06:39 PM, Frediano Ziglio 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.
Acked-by: Frediano Ziglio <fziglio@xxxxxxxxxx>

Took some time to properly test, tested combination of:
- using normal drawing and not;
- using mjpeg or h264;
- using h264 full screen;
- xorg or wayland;
- using vaapi (gstreamer-vaapi and intel drivers) and software.

Had some issues on Wayland with RHEL 7, on client connection graphics
on VM fails, but happens also without this patch (checked multiple
times, is consistent), seems related to fast resolution changes due
to some issue with Wayland.

Thanks for your tests, currently in wayland environment
old rendering path is used, so any issues with wayland
are likely to exist also without this patch.



Maybe there were some minor missing point but can be follows up,
the patch is a good step ahead, thanks for it.

There is one patch regarding the signal's description comment (as we talked on irc)
so it won't be documented, do we want to apply it? (see attached)


Snir.


---
  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);
+        }
+    }
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 */
@@ -383,7 +427,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;
@@ -415,8 +458,11 @@ static gboolean create_pipeline(SpiceGstDecoder
*decoder)
      decoder->appsink =
      GST_APP_SINK(gst_bin_get_by_name(GST_BIN(decoder->pipeline), "sink"));
  #endif
- appsink_cbs.new_sample = new_sample;
-    gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs, decoder,
NULL);
+    if (decoder->appsink) {
+        GstAppSinkCallbacks appsink_cbs = { NULL };
+        appsink_cbs.new_sample = new_sample;
+        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);
@@ -565,13 +611,17 @@ static gboolean
spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
      GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
      GST_BUFFER_PTS(buffer) = gst_clock_get_time(decoder->clock) -
      gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0,
      latency)) * 1000 * 1000;
- SpiceGstFrame *gst_frame = create_gst_frame(buffer, frame);
-    g_mutex_lock(&decoder->queues_mutex);
-    g_queue_push_tail(decoder->decoding_queue, gst_frame);
-    g_mutex_unlock(&decoder->queues_mutex);
+    if (decoder->appsink != NULL) {
+        SpiceGstFrame *gst_frame = create_gst_frame(buffer, frame);
+        g_mutex_lock(&decoder->queues_mutex);
+        g_queue_push_tail(decoder->decoding_queue, gst_frame);
+        g_mutex_unlock(&decoder->queues_mutex);
+    } else {
+        frame->free(frame);
+    }
if (gst_app_src_push_buffer(decoder->appsrc, buffer) != GST_FLOW_OK) {
-        SPICE_DEBUG("GStreamer error: unable to push frame of size %u",
frame->size);
+        SPICE_DEBUG("GStreamer error: unable to push frame");
          stream_dropped_frame_on_playback(decoder->base.stream);
      }
      return TRUE;
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 6a90a78..76d4dd0 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;
@@ -195,6 +196,7 @@ guint32 stream_get_time(display_stream *st);
  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);
G_END_DECLS
diff --git a/src/channel-display.c b/src/channel-display.c
index ed003be..cc0a4a1 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -91,6 +91,7 @@ enum {
      SPICE_DISPLAY_INVALIDATE,
      SPICE_DISPLAY_MARK,
      SPICE_DISPLAY_GL_DRAW,
+    SPICE_DISPLAY_STREAMING_MODE,
SPICE_DISPLAY_LAST_SIGNAL,
  };
@@ -454,6 +455,30 @@ static void
spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
                       4,
                       G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT);
+ /**
+     * SpiceDisplayChannel::streaming-mode:
+     * @display: the #SpiceDisplayChannel that emitted the signal
+     * @streaming_mode: %TRUE when it's streaming mode
+     *
+     * Return: handle for the display window if possible
+     *
+     * The #SpiceDisplayChannel::streaming_mode signal is emitted when
+     * spice server is working in streaming mode.
+     *
+     * Since 0.35
+     **/
+    signals[SPICE_DISPLAY_STREAMING_MODE] =
+        g_signal_new("streaming-mode",
+                     G_OBJECT_CLASS_TYPE(gobject_class),
+                     0,
+                     G_STRUCT_OFFSET(SpiceDisplayChannelClass,
+                                     streaming_mode),
+                     NULL, NULL,
+                     g_cclosure_user_marshal_POINTER__BOOLEAN,
+                     G_TYPE_POINTER,
+                     1,
+                     G_TYPE_BOOLEAN);
+
      g_type_class_add_private(klass, sizeof(SpiceDisplayChannelPrivate));
channel_set_handlers(SPICE_CHANNEL_CLASS(klass));
@@ -1370,6 +1395,15 @@ void stream_display_frame(display_stream *st,
SpiceFrame *frame,
      }
  }
+guintptr get_window_handle(display_stream *st)
+{
+   void* handle = 0;
+
+   g_signal_emit(st->channel, signals[SPICE_DISPLAY_STREAMING_MODE], 0,
+                 st->surface->streaming_mode, &handle);
+   return st->surface->streaming_mode ? (guintptr)handle : 0;
+}
+
  /* after a sequence of 3 drops, push a report to the server, even
   * if the report window is bigger */
  #define STREAM_REPORT_DROP_SEQ_LEN_LIMIT 3
@@ -1775,6 +1809,7 @@ 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("surface flags: %x", create->flags);
diff --git a/src/channel-display.h b/src/channel-display.h
index 5b48d2f..9c51aa2 100644
--- a/src/channel-display.h
+++ b/src/channel-display.h
@@ -125,6 +125,7 @@ struct _SpiceDisplayChannel {
   * @display_primary_destroy: Signal class handler for the
   #SpiceDisplayChannel::display-primary-destroy signal.
   * @display_invalidate: Signal class handler for the
   #SpiceDisplayChannel::display-invalidate signal.
   * @display_mark: Signal class handler for the
   #SpiceDisplayChannel::display-mark signal.
+ * @streaming_mode: Signal class handler for the
#SpiceDisplayChannel::streaming-mode signal.
   *
   * Class structure for #SpiceDisplayChannel.
   */
@@ -140,6 +141,8 @@ struct _SpiceDisplayChannelClass {
                                 gint x, gint y, gint w, gint h);
      void (*display_mark)(SpiceChannel *channel,
                           gboolean mark);
+    void (*streaming_mode)(SpiceChannel *channel,
+                           gboolean streaming_mode);
/*< private >*/
  };
diff --git a/src/spice-marshal.txt b/src/spice-marshal.txt
index 86673bd..b3a8e69 100644
--- a/src/spice-marshal.txt
+++ b/src/spice-marshal.txt
@@ -13,3 +13,4 @@ BOOLEAN:UINT,POINTER,UINT
  BOOLEAN:UINT,UINT
  VOID:OBJECT,OBJECT
  VOID:BOXED,BOXED
+POINTER:BOOLEAN
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
@@ -660,6 +660,10 @@ G_GNUC_BEGIN_IGNORE_DEPRECATIONS
  G_GNUC_END_IGNORE_DEPRECATIONS
  #endif
  #endif
+    area = gtk_drawing_area_new();
+    gtk_stack_add_named(d->stack, area, "gst-area");
+    gtk_widget_set_double_buffered(area, true);
+
      gtk_widget_show_all(widget);
g_signal_connect(display, "grab-broken-event", G_CALLBACK(grab_broken),
      NULL);
@@ -2589,6 +2593,33 @@ 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)
+{
+#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);
          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

>From 91250b1a1c60bfd7ee582c43f7a4c584c9198647 Mon Sep 17 00:00:00 2001
From: Snir Sheriber <ssheribe@xxxxxxxxxx>
Date: Thu, 26 Apr 2018 11:14:02 +0300
Subject: [PATCH] Avoid of having the streaming-mode signal documented

---
 src/channel-display.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/channel-display.c b/src/channel-display.c
index cc0a4a1..bbf4859 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -455,18 +455,18 @@ static void spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
                      4,
                      G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT);
 
-    /**
+    /*
      * SpiceDisplayChannel::streaming-mode:
      * @display: the #SpiceDisplayChannel that emitted the signal
      * @streaming_mode: %TRUE when it's streaming mode
      *
-     * Return: handle for the display window if possible
+     * Returns: handle for the display window if possible
      *
      * The #SpiceDisplayChannel::streaming_mode signal is emitted when
      * spice server is working in streaming mode.
      *
      * Since 0.35
-     **/
+     */
     signals[SPICE_DISPLAY_STREAMING_MODE] =
         g_signal_new("streaming-mode",
                      G_OBJECT_CLASS_TYPE(gobject_class),
-- 
2.17.0

_______________________________________________
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]