[Spice-devel] [spice-gtk v2 1/3] session: initialize gstreamer once

[Victor Toso]

Hi,

On Mon, Sep 09, 2019 at 06:45:57AM -0400, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me at victortoso.com>
> > 
> > GStreamer is required since v0.36 with 83ab7ca "build-sys: drop
> > gstvideo option, make it required" in 2019-01-15 by Marc-André Lureau
> > <marcandre.lureau at redhat.com>
> > 
> > Both channel-display-gst.c and spice-gstaudio.c have to double check
> > that GStreamer was initialized with gst_init_check() but this can be
> > done once per SpiceSession and make those code path a little bit
> > lighter with simpler check gst_is_initialized()
> > 
> 
> Calling gstvideo_init or gst_is_initialized is not much difference,
> potentially calling gstvideo_init is faster as the function is in
> the same module (you can cache initialization done).
> This series seems to reuse code to initialize GStreamer but this
> can simply be achieved calling gstvideo_init from the audio side
> (or write a better initialization function).
> This patch increase SpiceSession code for not great reasons,
> SpiceSession does nothing direct with Gstreamer.

Well, the initial idea really was to init + deinit so it might be
lacking purpose now. So we can drop to avoid not important
discussions.

> > This first patch does initialize a SpiceSession on it's init. As the
> > current code path does not call gst_deinit(), we are not doing it so
> > here as well but it can be later optimized to be sure resources are
> > cleaned correctly on GStreamer side.
> > 
> This sentence is wrong. We can't do it.

Leftover from v1, thanks.

> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  src/spice-session.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index d0d9e54..2f44816 100644
> > --- a/src/spice-session.c
> > +++ b/src/spice-session.c
> > @@ -21,6 +21,7 @@
> >  #include <gio/gnetworking.h>
> >  #include <gio/gio.h>
> >  #include <glib.h>
> > +#include <gst/gst.h>
> >  #ifdef G_OS_UNIX
> >  #include <gio/gunixsocketaddress.h>
> >  #endif
> > @@ -234,6 +235,7 @@
> > G_STATIC_ASSERT(G_N_ELEMENTS(_spice_image_compress_values) ==
> > SPICE_IMAGE_COMPRE
> >  
> >  static const gchar* spice_session_get_shared_dir(SpiceSession *session);
> >  static void spice_session_set_shared_dir(SpiceSession *session, const gchar
> >  *dir);
> > +static void spice_session_enable_gstreamer(SpiceSession *session);
> >  
> >  GType
> >  spice_image_compress_get_type (void)
> > @@ -295,6 +297,7 @@ static void spice_session_init(SpiceSession *session)
> >      s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
> >      s->glz_window = glz_decoder_window_new();
> >      update_proxy(session, NULL);
> > +    spice_session_enable_gstreamer(session);
> >  }
> >  
> >  static void
> > @@ -2865,3 +2868,22 @@ gboolean
> > spice_session_set_migration_session(SpiceSession *session, SpiceSession
> >  
> >      return TRUE;
> >  }
> > +
> > +static void
> > +spice_session_enable_gstreamer(SpiceSession *session)
> > +{
> > +    g_return_if_fail(SPICE_IS_SESSION(session));
> 
> session argument is used only here,

Leftover from v1, thanks.

> to me it seems this utility function is more gstreamer related
> then SpiceSession related. The fact that you have to include
> gstreamer header just for that utility confirms to me that this
> utility should be in another, more gstreamer related, source
> file.

Not sure what would be the issue with initializing GStreamer on
Session init and everywhere else, that is, channel-display-gst
and spice-gstaudio, we only check that gst functions can be used.
Having multiple/different channels to potentially init gstreamer
seems weirder than initialize in the session object. Another way
to put it, gst_init_check() should have argc/argv arguments for
command line parsing and giving that to spice-display-gst and
spice-gstaudio  (...)

Maybe a spice_client_init() would make sense but I don't feel
this changes are really a requirement for a new API so, again,
just dropping it.

> > +    if (gst_is_initialized()) {
> > +        /* Either called by spice client or in previous SpiceSession */
> > +        return;
> > +    }
> > +
> > +    /* TODO: Provide argc and argv to GStreamer for command line filtering
> > on
> > +     * spice-gtk level, otherwise only applications that run gst_init()
> > first
> > +     * would filter command line options */
> > +    GError *err = NULL;
> > +    if (!gst_init_check(NULL, NULL, &err)) {
> > +        spice_warning("Disabling GStreamer video support: %s",
> > err->message);
> > +        g_clear_error(&err);
> > +    }
> > +}
> 
> Frediano
-------------- 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/20190909/cf60b7d9/attachment-0001.sig>