<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Sep 24, 2014 at 8:06 PM, Jeremy White <<a href="mailto:jwhite@codeweavers.com" target="_blank">jwhite@codeweavers.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">A MsgMainMultiMediaTime message on the main channel will be relayed through main_dispatcher so as to be fired in the context of the main (not worker) thread. In qemu, that thread happens to also be the thread that drives the audio channel, so it works. In XSpice, it is a different thread, which leads to unpleasant side effects. The predominant side effect I noticed was an infinite loop in snd_send_data. </blockquote><div> </div><div>Is this difficult to solve? It seems the thread is reading from a fd and pushing to spice server. Couldn't this be done from X main loop? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> This patch uses a mutex to prevent such collisions. </blockquote><div> </div><div>Is the mutex supposed to protect the whole SndChannel struct?It seems there are various code path that could be called with or without the lock held (snd_playback_send callback, for example). I would rather move the protected bits into a substructure, and make sure only access is made with the lock. </div><div>But I think we are going in a dangerous place if we start making spice server MT-safe api </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Signed-off-by: Jeremy White <<a href="mailto:jwhite@codeweavers.com">jwhite@codeweavers.com</a>> --- server/snd_worker.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/server/snd_worker.c b/server/snd_worker.c index 70148b7..b977bed 100644 --- a/server/snd_worker.c +++ b/server/snd_worker.c @@ -115,6 +115,8 @@ struct SndChannel { snd_channel_handle_message_proc handle_message; snd_channel_on_message_done_proc on_message_done; snd_channel_cleanup_channel_proc cleanup; + + pthread_mutex_t lock; }; typedef struct PlaybackChannel PlaybackChannel; @@ -220,8 +222,21 @@ static void snd_disconnect_channel(SndChannel *channel) spice_marshaller_destroy(channel->send_data.marshaller); snd_channel_put(channel); worker->connection = NULL; + + pthread_mutex_destroy(&channel->lock); +} + +static int snd_lock(SndChannel *channel) +{ + return pthread_mutex_lock(&channel->lock); +} + +static void snd_unlock(SndChannel *channel) +{ + pthread_mutex_unlock(&channel->lock); } + static void snd_playback_free_frame(PlaybackChannel *playback_channel, AudioFrame *frame) { frame->channel = playback_channel; @@ -967,6 +982,8 @@ static SndChannel *__new_channel(SndWorker *worker, int size, uint32_t channel_i if (!channel->channel_client) { goto error2; } + + pthread_mutex_init(&channel->lock, NULL); return channel; error2: @@ -1105,10 +1122,15 @@ SPICE_GNUC_VISIBLE void spice_server_playback_put_samples(SpicePlaybackInstance frame = SPICE_CONTAINEROF(samples, AudioFrame, samples); playback_channel = frame->channel; spice_assert(playback_channel); + + if (snd_lock(&playback_channel->base) != 0) + return; + if (!snd_channel_put(&playback_channel->base) || sin->st->worker.connection != &playback_channel->base) { /* lost last reference, channel has been destroyed previously */ spice_info("audio samples belong to a disconnected channel"); + snd_unlock(&playback_channel->base); return; } spice_assert(playback_channel->base.active); @@ -1121,6 +1143,7 @@ SPICE_GNUC_VISIBLE void spice_server_playback_put_samples(SpicePlaybackInstance playback_channel->pending_frame = frame; snd_set_command(&playback_channel->base, SND_PLAYBACK_PCM_MASK); snd_playback_send(&playback_channel->base); + snd_unlock(&playback_channel->base); } void snd_set_playback_latency(RedClient *client, uint32_t latency) @@ -1136,8 +1159,11 @@ void snd_set_playback_latency(RedClient *client, uint32_t latency) PlaybackChannel* playback = (PlaybackChannel*)now->connection; playback->latency = latency; - snd_set_command(now->connection, SND_PLAYBACK_LATENCY_MASK); - snd_playback_send(now->connection); + if (snd_lock(now->connection) == 0) { + snd_set_command(now->connection, SND_PLAYBACK_LATENCY_MASK); + snd_playback_send(now->connection); + snd_unlock(now->connection); + } } else { spice_debug("client doesn't not support SPICE_PLAYBACK_CAP_LATENCY"); } -- 1.7.10.4 _______________________________________________ Spice-devel mailing list <a href="mailto:Spice-devel@lists.freedesktop.org">Spice-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/spice-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/spice-devel</a> </blockquote></div> -- Marc-André Lureau </div></div>