Hey,
On Sun, Dec 09, 2018 at 03:26:30PM +0200, Snir Sheriber wrote:
> 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. By doing that, issues like
> resizing the window were solved.
>
> Signed-off-by: Snir Sheriber <ssheribe@xxxxxxxxxx>
> ---
>
> This patch is replacing the streaming-mode signal but this signal was never documented
> so it should not be an issue.
>
> ---
> 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));
As far as I can tell, you don't really need the value of the win_handle,
you only need to know whether spice-gtk is going to use an overlay or
not, so hand_pipeline_to_widget could probably just return a boolean.
> @@ -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);
This should be GST_TYPE_PIPELINE.
>
> 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
> @@ -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;
This could be
g_clear_pointer(&d->overlay, gst_object_unref);
g_clear_pointer(&d->pipeline, gst_object_unref);
> #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)));
> + 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);
> +
> + 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);
> + }
> +}
I'm under the impression these 2 callbacks could be added in a follow-up
patch? Or is there going to be regression in spice-gtk behaviour if they
are not part of the patch adding the gst-video-overlay signal?
Christophe
> +#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");
> 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));
> + bus = gst_pipeline_get_bus(GST_PIPELINE(d->pipeline));
> + gst_bus_set_sync_handler (bus, (GstBusSyncHandler) handle_pipeline_message_sync, data, NULL);
> + 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);
> --
> 2.19.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel