[Spice-devel] [PATCH spice-gtk] gst-audio: Do not update mmtime without real audio channel

[Pavel Grunt]

On Tue, 2017-07-18 at 17:04 +0200, Christophe de Dinechin wrote:
> On 14 Jul 2017, at 14:27, Christophe de Dinechin <dinechin at redhat.com> wrote:
> > 
> > This looks better than my fix. Can’t ack right now (I did not build and test
> > it yet), but that looks like a reasonable approach
> > 
> > > On 14 Jul 2017, at 14:24, Pavel Grunt <pgrunt at redhat.com> wrote:
> > > 
> > > This also fixes the huge memore leak reported by Christophe.
> > > Steps to reproduce:
> > > 1. ./configure --disable-pulse --enable-gstaudio
> > > 2. connect to a vm streaming video with no audio playing
> > > 3. see the memory grow really fast
> > > 
> > > It seems to be due to the fact that the "fake audio" channel updates the
> > > mmtime
> > > in a way that schedules the frames to the future ((unsigned) 0 - delay)
> > > and then
> > > gstreamer starts to queue the frames (they should be played later)
> > > 
> > > Pavel
> > > 
> > > On Fri, 2017-07-14 at 13:19 +0200, Pavel Grunt wrote:
> > > > The fake channel has been introduced to get the audio volume by starting
> > > > the gstreamer's audio pipeline and querring its volume info (see commit
> > > > aa8d044417bbf60685f59163b874ecb4f157c3c9).
> > > > 
> > > > Hovewer starting the pipeline updates the mmtime as a side effect. This
> > > > may cause a (big) delay in displaying a video stream.
> > > > 
> > > > Because the fake channel is only needed to get the volume info, make
> > > > sure to not update the mm-time with it.
> > > > 
> > > > Reported-by: Christophe de Dinechin <dinechin at redhat.com>
> > > > ---
> > > > src/spice-gstaudio.c | 11 +++++++----
> > > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > > > index 014c3a5..715f824 100644
> > > > --- a/src/spice-gstaudio.c
> > > > +++ b/src/spice-gstaudio.c
> > > > @@ -38,6 +38,7 @@ struct stream {
> > > >    GstElement              *sink;
> > > >    guint                   rate;
> > > >    guint                   channels;
> > > > +    gboolean                fake; /* fake channel just for getting info
> > > > about
> > > > audio (volume) */
> > > > };
> > > > 
> > > > struct _SpiceGstaudioPrivate {
> > > > @@ -264,6 +265,8 @@ static gboolean update_mmtime_timeout_cb(gpointer
> > > > data)
> > > >    SpiceGstaudioPrivate *p = gstaudio->priv;
> > > >    GstQuery *q;
> > > > 
> > > > +    g_return_val_if_fail(!p->playback.fake, TRUE);
> > > > +
> > > >    q = gst_query_new_latency();
> > > >    if (gst_element_query(p->playback.pipe, q)) {
> > > >        gboolean live;
> > > > @@ -326,7 +329,7 @@ cleanup:
> > > >    if (p->playback.pipe)
> > > >        gst_element_set_state(p->playback.pipe, GST_STATE_PLAYING);
> > > > 
> > > > -    if (p->mmtime_id == 0) {
> > > > +    if (!p->playback.fake && p->mmtime_id == 0) {
> > > >        update_mmtime_timeout_cb(gstaudio);
> > > >        p->mmtime_id = g_timeout_add_seconds(1, update_mmtime_timeout_cb,
> > > > gstaudio);
> > > >    }
> > > > @@ -569,7 +572,6 @@ static gboolean
> > > > spice_gstaudio_get_playback_volume_info_finish(SpiceAudio *audio
> > > >    GstElement *e;
> > > >    gboolean lmute;
> > > >    gdouble vol;
> > > > -    gboolean fake_channel = FALSE;
> > > >    GTask *task = G_TASK(res);
> > > > 
> > > >    g_return_val_if_fail(g_task_is_valid(task, audio), FALSE);
> > > > @@ -584,9 +586,9 @@ static gboolean
> > > > spice_gstaudio_get_playback_volume_info_finish(SpiceAudio *audio
> > > > 
> > > >    if (p->playback.sink == NULL || p->playback.channels == 0) {
> > > >        SPICE_DEBUG("PlaybackChannel not created yet, force start");
> > > > +        p->playback.fake = TRUE;
> > > >        /* In order to get system volume, we start the pipeline */
> > > >        playback_start(NULL, SPICE_AUDIO_FMT_S16, 2, 48000, audio);
> > > > -        fake_channel = TRUE;
> > > >    }
> > > > 
> > > >    if (GST_IS_BIN(p->playback.sink))
> > > > @@ -604,9 +606,10 @@ static gboolean
> > > > spice_gstaudio_get_playback_volume_info_finish(SpiceAudio *audio
> > > >    }
> > > >    g_object_unref(e);
> > > > 
> > > > -    if (fake_channel) {
> > > > +    if (p->playback.fake) {
> > > >        SPICE_DEBUG("Stop faked PlaybackChannel");
> > > >        playback_stop(SPICE_GSTAUDIO(audio));
> > > > +        p->playback.fake = FALSE;
> > > >    }
> > > > 
> > > >    if (mute != NULL) {
> 
> This fixes the issue I was seeing with a black screen.

Does it fix the memory usage problem for you? it does for me.

Pavel

> 
> Acked-by: Christophe de Dinechin <dinechin at redhat.com>
> 
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
>