[Spice-devel] [spice-gtk v1 4/6] session: gst_deinit() GStreamer if we initialize it

[Frediano Ziglio]

> 
> Hi,
> 
> On 9/2/19 7:04 PM, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> >
> > Let's gst_deinit() if we initialize it for the same rationale pointed out
> > in 0381e62 "spicy: Add call of gst_deinit at program exit" in
> > 2017-10-20 by Christophe de Dinechin <dinechin at redhat.com>
> >
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >   src/spice-session.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index db40a53..2135348 100644
> > --- a/src/spice-session.c
> > +++ b/src/spice-session.c
> > @@ -123,6 +123,8 @@ struct _SpiceSessionPrivate {
> >       gchar             *name;
> >       SpiceImageCompression preferred_compression;
> >   
> > +    bool              gst_init_by_spice;
> > +
> >       /* associated objects */
> >       SpiceAudio        *audio_manager;
> >       SpiceUsbDeviceManager *usb_manager;
> > @@ -343,6 +345,10 @@ spice_session_dispose(GObject *gobject)
> >       g_warn_if_fail(s->channels_destroying == 0);
> >       g_warn_if_fail(s->channels == NULL);
> >   
> > +    if (session->priv->gst_init_by_spice) {
> > +        gst_deinit();
> 
> 
> Wouldn't it deinit on migration? (IIRC a new session is created)
> 
> 
> Actually gstreamer documentation states:
> 
> "It is normally not needed to call this function in a normal application
>   as the resources will automatically be freed when the program terminates.
>   This function is therefore mostly used by testsuites and other memory
>   profiling tools."
> 
> Before it was used only by spicy which i guess it's considered more of a
> test
> client, I'm not sure we would like to deinit by the session (on the
> other hand
> i'm also not sure how harmful it would be)
> 
> 
> Snir.
> 

Simply don't do it. Gstreamer is not well designed to call that function.

It leads to potential memory bugs. The check should be

if (I_initialized_gstreamer && nobody_is_using_or_assuming_is_initialized)
   gst_deinit()

If A initialize Gstreamer and B don't but just check is initialized then
when A call deinit the objects used by B will contain potential dangling
pointers. One right interface would be simply init/deinit and use a counter
to track the number of initialization.

Frediano


> 
> > +    }
> > +
> >       g_clear_object(&s->audio_manager);
> >       g_clear_object(&s->usb_manager);
> >       g_clear_object(&s->proxy);
> > @@ -2888,5 +2894,7 @@ spice_session_enable_gstreamer(SpiceSession *session)
> >       if (!gst_init_check(NULL, NULL, &err)) {
> >           spice_warning("Disabling GStreamer video support: %s",
> >           err->message);
> >           g_clear_error(&err);
> > +    } else {
> > +        session->priv->gst_init_by_spice = true;
> >       }
> >   }