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

[Frediano Ziglio]

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

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