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

[Frediano Ziglio]

> 
> Hi
> 
> On Thu, May 17, 2018 at 3:01 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>
> >> Hi
> >>
> >> On Thu, May 17, 2018 at 1:50 PM, Frediano Ziglio <fziglio at redhat.com>
> >> wrote:
> >> >>
> >> >> On Wed, May 16, 2018 at 3:54 PM, Frediano Ziglio <fziglio at redhat.com>
> >> >> wrote:
> >> >> >>
> >> >> >> On Wed, May 16, 2018 at 10:46:35AM +0200, Christophe Fergeau wrote:
> >> >> >> > 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:
> >> >> >> > > 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:
> >> >> >>
> >> >> >> (I promise I'll stop replying to myself :d). Not sure we are
> >> >> >> supporting
> >> >> >> inheritance from SpiceDisplayChannel, nor that it makes sense, so
> >> >> >> maybe
> >> >> >> not a big problem.
> >> >> >>
> >> >> >> Christophe
> >> >> >>
> >> >> >
> >> >> > Then class should be declare final is possible and details removed
> >> >> > from
> >> >> > public interface.
> >> >> > Looking also at 941f4adf1322a5d2548af676059003df17613b47 seems to
> >> >> > confirm
> >> >> > that this class was never meant to be inherited.
> >> >>
> >> >> That's a commit from 0.1 era ;)
> >> >>
> >> >
> >> > Yes, you are right.
> >> > I though I looked in the past and found another ABI breakage more
> >> > recent.
> >> >
> >> >> We may have made some mistakes, but in theory, we bumped
> >> >> major/minor/etc correctly enough whenever adding/modifying/removing
> >> >> interface (breaking ABI).
> >> >>
> >> >> During next break (some day), we may use the opportunity to make make
> >> >> most channel classes final. This is not a priority though. In the
> >> >> meantime, we don't need to add class signal handlers, and simply use
> >> >> the private struct for data.
> >> >>
> >> >
> >> > From g_signal_new documentation:
> >> >
> >> > class_offset
> >> > The offset of the function pointer in the class structure for this type.
> >> > Used to invoke a class method generically.
> >> > Pass 0 to not associate a class method slot with this signal.
> >> >
> >> > I suggested as a follow up to make this signal "private" not making the
> >> > documentation
> >> > public. I think we don't need to use this signal as a method so I think
> >> > we
> >> > can remove
> >> > the field in SpiceDisplayChannelClass and pass 0 to avoid ABI break,
> >> > kind
> >> > of:
> >> >
> >>
> >> I'd rather follow Christophe plan to call
> >> channel_display_set_window_handle() early on. This API should be
> >> public.
> >>
> >
> > IMO should be private, the SpiceWidget has the window and can communicate
> > its ID to SpiceChannel without adding a public interface, they are both on
> > the same shared object.
> 
> They are not, channel-display is in libspice-client-glib, and can be
> reused for Qt, Android etc..
> 
> spice-widget is in  libspice-client-gtk, gtk specific code.
> 

Forgot this separation. So yes, is a kind of public interface.
Nothing however forbid to document that this interface is intended
for spice-client-gtk <-> spice-client-glib only.
A signal is less bounding than a function call, removing either
the signal or its connection would just decrease a bit the performance. 

> >
> >> > diff --git a/src/channel-display.c b/src/channel-display.c
> >> > index 3f2fb3dc..1c0021e8 100644
> >> > --- a/src/channel-display.c
> >> > +++ b/src/channel-display.c
> >> > @@ -470,9 +470,7 @@ static void
> >> > spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
> >> >      signals[SPICE_DISPLAY_STREAMING_MODE] =
> >> >          g_signal_new("streaming-mode",
> >> >                       G_OBJECT_CLASS_TYPE(gobject_class),
> >> > -                     0,
> >> > -                     G_STRUCT_OFFSET(SpiceDisplayChannelClass,
> >> > -                                     streaming_mode),
> >> > +                     0, 0,
> >> >                       NULL, NULL,
> >> >                       g_cclosure_user_marshal_POINTER__BOOLEAN,
> >> >                       G_TYPE_POINTER,
> >> > diff --git a/src/channel-display.h b/src/channel-display.h
> >> > index 9c51aa2a..31293094 100644
> >> > --- a/src/channel-display.h
> >> > +++ b/src/channel-display.h
> >> > @@ -141,8 +141,6 @@ 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 >*/
> >> >  };
> >> >
> >

Frediano