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

[Marc-André Lureau]

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.

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



-- 
Marc-André Lureau