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