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

[Frediano Ziglio]

> 
> Hi,
> 
> On Sun, Sep 08, 2019 at 12:44:26PM -0400, Frediano Ziglio wrote:
> > > 
> > > 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)
> 
> Seems like it, should be fixed, thanks!
> 
> > > 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."
> 
> The memory profiling tools might be what we are interested on, as
> mentioned in the commit log, I quote the original commit:
> 
>  | Without this patch, if you run spicy with
>  |  GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
>  | the leak tracer does not show any output,
>  | because it runs in gst_deinit.
> 
> 
> If we are not introducing something that breaks the code and
> might help on debug, I'm keen to have it. Without this patch we
> would rely on clients to implement this in order to gstreamer's
> leak tracer to work, like the patch for spicy 0381e62
> 
> > > 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()
> 
> I agree that the check should be like that
> 
> > 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.
> 
> All channels are related to SpiceSession, that's why I moved it
> there. Seems reasonable to me at least.
> 

Yes, it is. But this won't fix Gstreamer API.
Nobody forbid users to use spice-gtk library in another program
to provide for instance multiple VM access using multiple GUI tabs.
If the other part will use Gstreamer calling gst_deinit without making
sure nobody else will use it will potentially lead to memory issues.
That's my suggestion "don't do it". Calling that from spicy is fine,
because you know nobody else is using Gstreamer, not in a library.

> I'll fix proposed changes and send a v2, many thanks for review!
> 
>  - toso
> 

Frediano