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

[Marc-André Lureau]

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.

> 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