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

[Frediano Ziglio]

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

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

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

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