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

[Victor Toso]

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.

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

 - toso
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190909/187bcd5d/attachment.sig>