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

[Christophe de Dinechin]

> On 18 Jul 2017, at 17:54, Pavel Grunt <pgrunt at redhat.com> wrote:
> 
> 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.

It fixes the case under light load, yes. It does not fix the issue under heavy load. For this one, I need the other patch (the one that resets the queue length. To me, that patch was not about that problem, though, and I did not expect it to address memory growth under load, so I’m still satisfied.


Christophe

> 
> Pavel
> 
>> 
>> Acked-by: Christophe de Dinechin <dinechin at redhat.com <mailto:dinechin at redhat.com>>
>> 
>>> 
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170718/c0e1bc0f/attachment.html>