Hi,
On Mon, Sep 09, 2019 at 06:45:57AM -0400, Frediano Ziglio wrote:
> >
> > From: Victor Toso <me@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel