[Spice-devel] [RFC spice-gtk v3 1/1] Gstreamer: Use GstVideoOverlay if possible
Snir Sheriber
ssheribe at redhat.com
Wed May 16 07:23:01 UTC 2018
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
More information about the Spice-devel
mailing list