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

Submitted by Victor Toso on Sept. 9, 2019, 10:29 a.m.

Details

Message ID 20190909102949.17504-2-victortoso@redhat.com
State New
Headers show
Series "Initialize GStreamer on SpiceSession" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Sept. 9, 2019, 10:29 a.m.
From: Victor Toso <me@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@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()

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.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/spice-session.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Patch hide | download patch | download mbox

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));
+    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);
+    }
+}

Comments

> 
> From: Victor Toso <me@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@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@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