[Spice-devel] [PATCH spice-gtk] gst-audio: Do not update mmtime without real audio channel
Christophe de Dinechin
dinechin at redhat.com
Tue Jul 18 16:55:12 UTC 2017
> On 18 Jul 2017, at 17:59, Christophe de Dinechin <dinechin at redhat.com> wrote:
>
>>
>> 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.
Interestingly, I just discovered something I did not really expect: a hot CPU
is more than enough to make the spice client lack “oxygen”, even if nothing else
is running.
If I start under light load and keep running that way, it can run a long time
without a problem. The queue length typically runs up and down a little,
but it does not diverge. See http://blackbox.dinechin.org/images/170718-NotDiverging.png.
There is even plenty of CPU left to converge back if we temporarily accumulate
things in the queue: http://blackbox.dinechin.org/images/170718-Reconverge.png.
However, if the machine gets hot, then it does not reconverge, because the machine
spends over 50% “in the kernel” just trying to cool down. Starting the client
under these conditions, it rapidly diverges: http://blackbox.dinechin.org/images/170718-WithHotCPU.png.
Also, without enough I-frames to recover, when this starts happening, we are a bit stuck.
This is all documented in more details here: http://blackbox.dinechin.org/170718.html.
Tons of messages that look like:
channel-display-gst.c:266: [783802 1768.364819] spice_warning: got an unexpected decoded buffer!
So I think we will need the mechanism we discussed with Uri, where we send
something back to tell the server we need an I-frame. Or we close/reopen the channel.
Christophe
>
>
> Christophe
>
>>
>> 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
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list