[Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

[Frediano Ziglio]

> > On 19 Oct 2017, at 13:49, Marc-André Lureau < marcandre.lureau at redhat.com >
> > wrote:
> 

> > ----- Original Message -----
> 

> > > > On 19 Oct 2017, at 13:15, Marc-André Lureau <
> > > > marcandre.lureau at redhat.com
> > > > >
> > > 
> > 
> 
> > > > wrote:
> > > 
> > 
> 

> > > > Hi
> > > 
> > 
> 

> > > > ----- Original Message -----
> > > 
> > 
> 

> > > > > > From: Christophe de Dinechin < dinechin at redhat.com >
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > This is useful for some instrumentation, e.g. the leaks tracer,
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > that perform some of their operations within gst_deinit.
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > Without this patch, if you run spicy with
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > the leak tracer does not run at exit, because it runs in
> > > > > > gst_deinit.
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > Signed-off-by: Christophe de Dinechin < dinechin at redhat.com >
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > ---
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > spice-common | 2 +-
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > src/channel-display-gst.c | 1 +
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > 2 files changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > diff --git a/spice-common b/spice-common
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > index 429ad96..ba11de3 160000
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > --- a/spice-common
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > +++ b/spice-common
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > @@ -1 +1 @@
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> > > > > 
> > > > 
> > > 
> > 
> 

> > probably a mistake
> 

> Yes.

> > > > > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > index f978602..c9ab9bf 100644
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > --- a/src/channel-display-gst.c
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > +++ b/src/channel-display-gst.c
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > GError *err = NULL;
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > if (gst_init_check(NULL, NULL, &err)) {
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > success = 1;
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > + atexit(gst_deinit);
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > } else {
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > spice_warning("Disabling GStreamer video support: %s",
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > err->message);
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > g_clear_error(&err);
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > Calling atexit from a library is a bad idea.
> > > > 
> > > 
> > 
> 

> > > > And calling gst_deinit() from a library is also wrong.
> > > 
> > 
> 

> > > Why? What would be a better way?
> > 
> 

> > Not calling it:
> 

> But as pointed in the commit log, this means that some gstreamer tools
> that rely on gst_deinit being called are broken for all spice clients.

> > /**
> 
> > * gst_deinit:
> 
> > *
> 
> > * Clean up any resources created by GStreamer in gst_init().
> 
> > *
> 
> > * 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.
> 
> > *
> 
> > * After this call GStreamer (including this method) should not be used
> > anymore.
> 
> > */
> 

> > spice-gtk is a library, that happen to use gstreamer.
> 

> If this library did init gstreamer, it is responsible to deinit it.
> Just like if it opens a file or allocates memory, it’s responsible to close
> it or deallocate it.

> > If something else in the client app is also using gstreamer this will cause
> > troubles.
> 

> And if someone unloads our library presently, it leaks.

> > Well calling atexit() should be fine, even if called multiple times, Why
> > it's
> > not done in gstreamer in this case? Why would every application have to add
> > gst_deinit() themself?
> 

> Well, the gstreamer init / deinit design reeks of big globals lurking
> somewhere anyway.
> So if you ask me, that interface should have been more like:

> gst_id id = gst_init();
> gst_deinit(id);

> That would have made it very clear that you could init multiple times and
> that each caller
> was responsible for calling deinit. But of course, that would have also meant
> passing the
> id around all the time. Much less convenient.

Or you could use a static counter incremented by gst_init and decremented by gst_deinit, kind 
of reference counting :-) 

> Since we don’t have that kind of interface, we can only second-guess wha the
> intent of the
> orignal developers was.

> Globals are evil. Pthread globals are worse. That’s not a reason to not call
> deinit.

> > One reason is that you may use a library dynamically, and you may dlclose()
> > it, and then atexit() will likely crash.
> 

> Is that a real or theoretical scenario? Who loads this library dynamically
> currently?

Was actually thinking at the Windows case. 
Could be a problem there (not sure). 

> > > The scenario where we call gst_init is a bit complicated,
> > 
> 
> > > and there are multiple clients that may call it. I see
> > 
> 
> > > no case where, when we have called gst_init, we are not
> > 
> 
> > > responsible for also calling gst_deinit.
> > 
> 

> > > In any case, it’s even more wrong to not call gst_deinit at all,
> > 
> 
> > > and I don’t see a simple way to do that from main with the
> > 
> 
> > > existing code without elaborate tests that have somewhat
> > 
> 
> > > deep knowledge of the library internals.
> > 
> 

> > > How would you do it?
> > 
> 
Frediano 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171019/269a77a9/attachment-0001.html>