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

[Christophe de Dinechin]

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

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?

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171019/5c513f19/attachment-0001.html>