[Spice-devel] [RFC spice-gtk v3 1/1] Gstreamer: Use GstVideoOverlay if possible

[Christophe Fergeau]

On Wed, May 16, 2018 at 10:43:20AM +0200, Christophe Fergeau wrote:
> On Wed, May 16, 2018 at 10:23:01AM +0300, Snir Sheriber wrote:
> > > > 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?).
> 
> Yes, indeed, this is similar to what you were doing in earlier
> iterations.
> 
> > 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.
> 
> Using signals as some generic way of calling a getter on some unknown
> class is rather unusual, and feels like something you should not really
> be using signals for.

Just realized, this change is most likely an ABI break:

diff --git a/src/channel-display.h b/src/channel-display.h
index 5b48d2ff..9c51aa2a 100644
--- a/src/channel-display.h
+++ b/src/channel-display.h
@@ -140,6 +141,8 @@ struct _SpiceDisplayChannelClass {
                                gint x, gint y, gint w, gint h);
     void (*display_mark)(SpiceChannel *channel,
                          gboolean mark);
+    void (*streaming_mode)(SpiceChannel *channel,
+                           gboolean streaming_mode);
 
     /*< private >*/
 };


However, I think you don't need to add a field to the class in order to
define a signal, so this added field can probably be removed somehow.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180516/e6c7999c/attachment.sig>