On Tue, 2017-07-18 at 17:04 +0200, Christophe de Dinechin wrote:On 14 Jul 2017, at 14:27, Christophe de Dinechin <dinechin@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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
|