[Spice-devel] [PATCH spice-gtk] Gstreamer: Control GstVideoOverlay from the widget
Frediano Ziglio
fziglio at redhat.com
Mon Dec 10 13:12:35 UTC 2018
>
> This patch is changing the way gstvideooverlay is being set.
I would use the camelcase GstVideoOverlay, not strong about it.
> 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. By doing that, issues like
> resizing the window were solved.
I think more exactly that this new interface allows to fix the
resize, not that solve by itself.
>
> Signed-off-by: Snir Sheriber <ssheribe at redhat.com>
> ---
>
> This patch is replacing the streaming-mode signal but this signal was never
> documented
> so it should not be an issue.
>
Agreed
> ---
> src/channel-display-gst.c | 24 ++------
> src/channel-display-priv.h | 1 +
> src/channel-display.c | 35 ++++++------
> src/spice-marshal.txt | 2 +-
> src/spice-widget-priv.h | 8 +++
> src/spice-widget.c | 111 +++++++++++++++++++++++++++++++++----
> 6 files changed, 135 insertions(+), 46 deletions(-)
>
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 2c07f35..f079132 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -334,20 +334,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,11 +382,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);
> + decoder->win_handle = hand_pipeline_to_widget(decoder->base.stream,
> GST_PIPELINE(playbin));
> SPICE_DEBUG("Creating Gstreamer pipeline (handle for overlay %s)\n",
> decoder->win_handle ? "received" : "not received");
> if (decoder->win_handle == 0) {
> @@ -576,7 +562,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 (win_handle was obtained successfully):
> * 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..29804b4 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -202,6 +202,7 @@ 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);
> +guintptr hand_pipeline_to_widget(display_stream *st, gpointer pipeline);
>
>
> G_END_DECLS
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 7c663cb..e4d9df9 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -88,7 +88,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 +453,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_cclosure_user_marshal_POINTER__POINTER,
> G_TYPE_POINTER,
> 1,
> - G_TYPE_BOOLEAN);
> + G_TYPE_POINTER);
>
> channel_set_handlers(SPICE_CHANNEL_CLASS(klass));
> }
> @@ -1391,13 +1392,15 @@ void stream_display_frame(display_stream *st,
> SpiceFrame *frame,
> }
> }
>
> -guintptr get_window_handle(display_stream *st)
> +guintptr hand_pipeline_to_widget(display_stream *st, gpointer pipeline)
> {
> - void* handle = 0;
> + void* handle = NULL;
>
> - 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, &handle);
> + }
> + return (guintptr)handle;
> }
>
> /* after a sequence of 3 drops, push a report to the server, even
> diff --git a/src/spice-marshal.txt b/src/spice-marshal.txt
> index b3a8e69..58a2b0e 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
> +POINTER:POINTER
> diff --git a/src/spice-widget-priv.h b/src/spice-widget-priv.h
> index 96f6c1d..81bb089 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
> + GstVideoOverlay *overlay;
> + GstElement *pipeline;
style: no space indentation
> +#endif
> };
>
> int spice_cairo_image_create (SpiceDisplay *display);
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 9bb4221..7ec17cb 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -120,6 +120,10 @@ static void size_allocate(GtkWidget *widget,
> GtkAllocation *conf, gpointer data)
> static gboolean draw_event(GtkWidget *widget, cairo_t *cr, gpointer data);
> static void update_size_request(SpiceDisplay *display);
> static GdkDevice *spice_gdk_window_get_pointing_device(GdkWindow *window);
> +#ifdef HAVE_GSTVIDEO
> +void gst_size_allocate(GtkWidget *widget, GdkRectangle *a, gpointer data);
Why not static? Looks a mistake if not declared in an header
> +static gboolean gst_draw_event(GtkWidget *widget, cairo_t *cr, gpointer
> data);
> +#endif
>
> /* ---------------------------------------------------------------- */
>
> @@ -649,8 +653,14 @@ static void spice_display_init(SpiceDisplay *display)
> NULL);
> gtk_stack_add_named(d->stack, area, "gl-area");
> #endif
> +#ifdef HAVE_GSTVIDEO
> area = gtk_drawing_area_new();
> gtk_stack_add_named(d->stack, area, "gst-area");
> + g_object_connect(area,
> + "signal::draw", gst_draw_event, display,
> + "signal::size-allocate", gst_size_allocate, display,
> + NULL);
> +#endif
>
> gtk_widget_show_all(widget);
>
> @@ -2115,10 +2125,23 @@ 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);
> +#endif
> +#ifdef HAVE_GSTVIDEO
> + SpiceDisplayPrivate *d = display->priv;
> +
> + if (d->overlay)
> + gst_object_unref(d->overlay);
> + d->overlay = NULL;
> +
> + if (d->pipeline)
> + gst_object_unref(d->pipeline);
> + d->pipeline = NULL;
> #endif
>
> GTK_WIDGET_CLASS(spice_display_parent_class)->unrealize(widget);
> @@ -2547,21 +2570,89 @@ 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 GstBusSyncReply handle_pipeline_message_sync(GstBus *bus, GstMessage
> *msg, gpointer data)
> {
> -#ifdef GDK_WINDOWING_X11
> + 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())) {
> + SpiceDisplay *display = data;
> + GdkWindow *window =
> gtk_widget_get_window(GTK_WIDGET(display));
> +
> + if (window && gdk_window_ensure_native(window)) {
> + SpiceDisplayPrivate *d = display->priv;
> +
> + d->overlay =
> gst_object_ref(GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg)));
If overlay is not NULL we'll have a leak as reference is not
decremented.
Looking at the pointers looks like channels owns both the pipeline and
(indirectly) the widget(s) so maybe would be better to have a weak
reference here instead of a strong one.
If the stream is closed and the pipeline is released from the channel
there's no point to keep the pipeline in the widget.
> + gst_video_overlay_set_window_handle(d->overlay,
> (uintptr_t)GDK_WINDOW_XID(window)); // moving to widget
> + gst_video_overlay_handle_events(d->overlay, false);
> + return GST_BUS_DROP;
> + }
> + }
> + }
> + break;
> + }
> + default:
> + /* not being handled */
> + break;
> + }
> + return GST_BUS_PASS;
> +}
> +#endif
> +
> +#ifdef HAVE_GSTVIDEO
> +static gboolean gst_draw_event(GtkWidget *widget, cairo_t *cr, gpointer
> data)
> +{
> + SpiceDisplay *display = SPICE_DISPLAY(data);
> + SpiceDisplayPrivate *d = display->priv;
> + g_return_val_if_fail(d != NULL, false);
We already used and dereferenced display, if this is NULL we have
a memory corruption, handling with a warning is just creating a security
issue! Either abort or remove the check.
> +
> + if (d->overlay) {
> + gst_video_overlay_expose(d->overlay);
> + update_mouse_pointer(display);
> + return true;
> + }
> + return false;
> +}
> +
> +void gst_size_allocate(GtkWidget *widget, GdkRectangle *a, gpointer data)
> +{
> + SpiceDisplay *display = SPICE_DISPLAY(data);
> + SpiceDisplayPrivate *d = display->priv;
> +
> + if (d->overlay) {
> + gint scale = 1;
> +
> + scale = gtk_widget_get_scale_factor(widget);
> + gst_video_overlay_set_render_rectangle(d->overlay, a->x * scale,
> a->y * scale, a->width * scale, a->height * scale);
> + }
> +}
> +#endif
> +
> +/* 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 void* 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;
>
> + gtk_stack_set_visible_child_name(d->stack, "gst-area");
Why do you thing the call is better outside the if?
There's no explanation in the commit message for this.
> window = gtk_widget_get_window(GTK_WIDGET(display));
> if (window && gdk_window_ensure_native(window)) {
> - gtk_stack_set_visible_child_name(d->stack, "gst-area");
> + GstBus *bus;
> +
> + d->pipeline = GST_ELEMENT(gst_object_ref(pipeline_ptr));
Same leak problem of the overlay.
> + bus = gst_pipeline_get_bus(GST_PIPELINE(d->pipeline));
> + gst_bus_set_sync_handler (bus, (GstBusSyncHandler)
> handle_pipeline_message_sync, data, NULL);
Cast to GstBusSyncHandler is useless here.
Maybe would be better to use gst_bus_enable_sync_message_emission and
connect the signal instead so to leave to spice-glib also the possibility
to handle some of these messages if needed?
> + gst_object_unref(bus);
> return (void*)(uintptr_t)GDK_WINDOW_XID(window);
> }
> }
> @@ -2936,8 +3027,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
More information about the Spice-devel
mailing list