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

Submitted by Victor Toso on Sept. 2, 2019, 4:04 p.m.

Details

Message ID 20190902160449.19589-5-victortoso@redhat.com
State New
Headers show
Series "Initialize GStreamer on SpiceSession" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Sept. 2, 2019, 4:04 p.m.
From: Victor Toso <me@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@redhat.com>

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/spice-session.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Patch hide | download patch | download mbox

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();
+    }
+
     g_clear_object(&s->audio_manager);
     g_clear_object(&s->usb_manager);
     g_clear_object(&s->proxy);
@@ -2888,5 +2894,7 @@  spice_session_enable_gstreamer(SpiceSession *session)
     if (!gst_init_check(NULL, NULL, &err)) {
         spice_warning("Disabling GStreamer video support: %s", err->message);
         g_clear_error(&err);
+    } else {
+        session->priv->gst_init_by_spice = true;
     }
 }

Comments

Hi,

On 9/2/19 7:04 PM, Victor Toso wrote:
> From: Victor Toso <me@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@redhat.com>
>
> Signed-off-by: Victor Toso <victortoso@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)


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

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.


> +    }
> +
>       g_clear_object(&s->audio_manager);
>       g_clear_object(&s->usb_manager);
>       g_clear_object(&s->proxy);
> @@ -2888,5 +2894,7 @@ spice_session_enable_gstreamer(SpiceSession *session)
>       if (!gst_init_check(NULL, NULL, &err)) {
>           spice_warning("Disabling GStreamer video support: %s", err->message);
>           g_clear_error(&err);
> +    } else {
> +        session->priv->gst_init_by_spice = true;
>       }
>   }
> 
> Hi,
> 
> On 9/2/19 7:04 PM, Victor Toso wrote:
> > From: Victor Toso <me@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@redhat.com>
> >
> > Signed-off-by: Victor Toso <victortoso@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)
> 
> 
> 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."
> 
> 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()

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.

Frediano


> 
> > +    }
> > +
> >       g_clear_object(&s->audio_manager);
> >       g_clear_object(&s->usb_manager);
> >       g_clear_object(&s->proxy);
> > @@ -2888,5 +2894,7 @@ spice_session_enable_gstreamer(SpiceSession *session)
> >       if (!gst_init_check(NULL, NULL, &err)) {
> >           spice_warning("Disabling GStreamer video support: %s",
> >           err->message);
> >           g_clear_error(&err);
> > +    } else {
> > +        session->priv->gst_init_by_spice = true;
> >       }
> >   }

> 
> 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@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@redhat.com>
> > > >
> > > > Signed-off-by: Victor Toso <victortoso@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.
> 

Yes, it is. But this won't fix Gstreamer API.
Nobody forbid users to use spice-gtk library in another program
to provide for instance multiple VM access using multiple GUI tabs.
If the other part will use Gstreamer calling gst_deinit without making
sure nobody else will use it will potentially lead to memory issues.
That's my suggestion "don't do it". Calling that from spicy is fine,
because you know nobody else is using Gstreamer, not in a library.

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

Frediano