> > On 19 Oct 2017, at 13:49, Marc-André Lureau < marcandre.lur...@redhat.com > > > wrote: >
> > ----- Original Message ----- > > > > > On 19 Oct 2017, at 13:15, Marc-André Lureau < > > > > marcandre.lur...@redhat.com > > > > > > > > > > > > > > > wrote: > > > > > > > > > > Hi > > > > > > > > > > ----- Original Message ----- > > > > > > > > > > > > From: Christophe de Dinechin < dinec...@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 < dinec...@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
_______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel