Re: [PATCH v2 spice-gtk 1/2] Gstreamer: Control GstVideoOverlay from the widget

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

 



> 
> 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. This allows to solve issues
> like resizing the window.
> 
> Signed-off-by: Snir Sheriber <ssheribe@xxxxxxxxxx>
> ---
> 
> Main changes from previous patch (according to comments):
> -Split patch
> -Move to weak refs
> -Do not return window handle back to decoder
> 
> 
> Since unref of the overlay element is done from gstreamer's
> thread it is not safe to use GObject's weak ptr for it from
> other threads, this is the reason i used GWeakRef for it.
> AFAIU for the pipeline it should be fine to use regular weak
> reference so i used weak pointer. (would be better to be
> consistent?)
> 

I actually not guarantee that spice-glib will be the last to release
that element (although likely), but I don't think would be a big deal.

> ---
>  src/channel-display-gst.c  | 30 ++++------------
>  src/channel-display-priv.h |  7 ++++
>  src/channel-display.c      | 40 ++++++++++++---------
>  src/spice-marshal.txt      |  2 +-
>  src/spice-widget-priv.h    |  8 +++++
>  src/spice-widget.c         | 73 ++++++++++++++++++++++++++++++++------
>  6 files changed, 108 insertions(+), 52 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 2c07f35..767f6b1 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -43,8 +43,6 @@ typedef struct SpiceGstDecoder {
>      GstElement *pipeline;
>      GstClock *clock;
>  
> -    guintptr win_handle;
> -
>      /* ---------- Decoding and display queues ---------- */
>  
>      uint32_t last_mm_time;
> @@ -334,20 +332,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,14 +380,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);
> -    SPICE_DEBUG("Creating Gstreamer pipeline (handle for overlay %s)\n",
> -                decoder->win_handle ? "received" : "not received");
> -    if (decoder->win_handle == 0) {
> +    if (!hand_pipeline_to_widget(decoder->base.stream,
> GST_PIPELINE(playbin))) {
>          sink = gst_element_factory_make("appsink", "sink");
>          if (sink == NULL) {
>              spice_warning("error upon creation of 'appsink' element");
> @@ -432,6 +413,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>          GstRegistry *registry = NULL;
>          GstPluginFeature *vaapisink = NULL;
>  
> +        SPICE_DEBUG("Video is presented using gstreamer's GstVideoOverlay
> interface");
>          registry = gst_registry_get();
>          if (registry) {
>              vaapisink = gst_registry_lookup_feature(registry, "vaapisink");
> @@ -576,7 +558,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 (window handle was obtained successfully at
> the widget):
>   *   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..8d9ff8d 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -32,6 +32,10 @@
>  #include "common/quic.h"
>  #include "common/rop3.h"
>  
> +#ifdef HAVE_GSTVIDEO
> +#include "gst/gst.h"
> +#endif
> +
>  G_BEGIN_DECLS
>  
>  typedef struct display_stream display_stream;
> @@ -202,6 +206,9 @@ 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);
> +#ifdef HAVE_GSTVIDEO
> +gboolean hand_pipeline_to_widget(display_stream *st,  GstPipeline
> *pipeline);
> +#endif
>  
>  
>  G_END_DECLS
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 7c663cb..6b6a172 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -29,6 +29,7 @@
>  #include "spice-session-priv.h"
>  #include "channel-display-priv.h"
>  #include "decode.h"
> +#include "gst/gst.h"

#ifdef HAVE_GSTVIDEO

>  
>  /**
>   * SECTION:channel-display
> @@ -88,7 +89,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 +454,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_TYPE_POINTER,
> +                     g_cclosure_user_marshal_BOOLEAN__POINTER,
> +                     G_TYPE_BOOLEAN,
>                       1,
> -                     G_TYPE_BOOLEAN);
> +                     GST_TYPE_PIPELINE);
>  
>      channel_set_handlers(SPICE_CHANNEL_CLASS(klass));
>  }
> @@ -1391,14 +1393,18 @@ void stream_display_frame(display_stream *st,
> SpiceFrame *frame,
>      }
>  }
>  
> -guintptr get_window_handle(display_stream *st)
> +#ifdef HAVE_GSTVIDEO
> +gboolean hand_pipeline_to_widget(display_stream *st, GstPipeline *pipeline)
>  {
> -   void* handle = 0;
> +    gboolean res = false;
>  
> -   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, &res);
> +    }
> +    return res;
>  }
> +#endif
>  
>  /* after a sequence of 3 drops, push a report to the server, even
>   * if the report window is bigger */
> diff --git a/src/spice-marshal.txt b/src/spice-marshal.txt
> index b3a8e69..92087c5 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
> +BOOLEAN:POINTER
> diff --git a/src/spice-widget-priv.h b/src/spice-widget-priv.h
> index 96f6c1d..651d306 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
> +    GWeakRef                overlay_element_weak_ref;
> +    GstPipeline             *pipeline;
> +#endif

Minor: style, no space indentation

>  };
>  
>  int      spice_cairo_image_create                 (SpiceDisplay *display);
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 9bb4221..ae83420 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -2115,10 +2115,18 @@ 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);

Style: brackets

> +#endif
> +#ifdef HAVE_GSTVIDEO
> +    SpiceDisplayPrivate *d = display->priv;
> +
> +    g_weak_ref_set(&d->overlay_element_weak_ref, NULL);
> +    g_clear_weak_pointer(&d->pipeline);
>  #endif
>  
>      GTK_WIDGET_CLASS(spice_display_parent_class)->unrealize(widget);
> @@ -2547,26 +2555,71 @@ 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 void gst_sync_bus_call(GstBus *bus, GstMessage *msg, gpointer data)
> +{
> +    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())) {

Using a single if would reduce indentation here

> +                SpiceDisplay *display = data;

Minor: maybe would be better to put this at the beginning of the function?

Or just declare the function as

static void gst_sync_bus_call(GstBus *bus, GstMessage *msg, SpiceDisplay *display)

> +                GdkWindow *window =
> gtk_widget_get_window(GTK_WIDGET(display));
> +
> +                if (window && gdk_window_ensure_native(window)) {
> +                    SpiceDisplayPrivate *d = display->priv;
> +                    GstElement *overlay_element;
> +
> +                    g_weak_ref_set(&d->overlay_element_weak_ref,
> GST_ELEMENT(GST_MESSAGE_SRC(msg)));
> +                    overlay_element =
> g_weak_ref_get(&d->overlay_element_weak_ref);
> +
> gst_video_overlay_set_window_handle(GST_VIDEO_OVERLAY(overlay_element),
> (uintptr_t)GDK_WINDOW_XID(window));
> +
> gst_video_overlay_handle_events(GST_VIDEO_OVERLAY(overlay_element),
> false);
> +                    gst_object_unref(overlay_element);

Overlay is always used as overlay and we already have a strong reference so
this sounds simpler:

    GstVideoOverlay *overlay = GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg));
    g_weak_ref_set(&d->overlay_element_weak_ref, overlay);
    gst_video_overlay_set_window_handle(overlay, (uintptr_t)GDK_WINDOW_XID(window));
    gst_video_overlay_handle_events(overlay, false);

> +                    return;
> +                }
> +            }
> +        }
> +        break;
> +    }
> +    default:
> +        /* not being handled */
> +        break;
> +    }
> +}
> +#endif
> +
>  {

I suppose this is a split error, patch does not compile with this.

>  #ifdef GDK_WINDOWING_X11
> +/* 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 gboolean 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;
>  
>          window = gtk_widget_get_window(GTK_WIDGET(display));
>          if (window && gdk_window_ensure_native(window)) {
> +            GstBus *bus;
> +
>              gtk_stack_set_visible_child_name(d->stack, "gst-area");
> -            return (void*)(uintptr_t)GDK_WINDOW_XID(window);
> +            d->pipeline = pipeline_ptr;
> +            g_object_add_weak_pointer(pipeline_ptr,
> (gpointer*)&(d->pipeline)); // Taking a weak ref although pipeline is not
> used again

Why retaining a pointer you are not using? This is not used in either patches,
are you planning to use it? If not easier to remove.
Would make some difference if it was a strong pointer (so lifetime would be
extended) but not much a weak one.

> +            bus = gst_pipeline_get_bus(GST_PIPELINE(d->pipeline));
> +            gst_bus_enable_sync_message_emission(bus);
> +            g_signal_connect (bus, "sync-message", G_CALLBACK
> (gst_sync_bus_call), data);

Minor: style, spaces before parenthesis

> +            gst_object_unref(bus);
> +            return true;
>          }
>      }
>  #endif
> -    return NULL;
> +    return false;
>  }
>  
>  static void invalidate(SpiceChannel *channel,
> @@ -2936,8 +2989,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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




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