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

[Marc-André Lureau]

----- 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

> >>> 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:
/**
 * 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 something else in the client app is also using gstreamer this will cause troubles.

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?

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

> 
> 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?
> 
> > 
> 
>