> On 2 Oct 2017, at 15:43, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > > Related problem with audio. > > Look at that video > https://www.youtube.com/watch?v=bn5iczqLBmk > basically for a short while while playing a video with audio network is disconnected. > This create a delay (see the delay when moving the mouse, I had 2 cursor so show > the delay). > The server cursor follow with a huge delay which disappears when audio is > disabled. > This is caused by some reasons: > - communication is using tcp so packets are retransmitted, not discarded; > - audio packets are never discarded; That part seems the easiest to address, no? > - video is synchronized on audio. In case we decide to drop audio packets and re-sync, we also need an I-frame. > You can understand that the delay can became so high that is hard to > control the VM. Yes ;-) > > Usually for communications (like mobile phones or similar) when a network > problem happens we ears noises or silences, just packets are discarded > (or corrupted). I think that dropping audio packets makes sense. But then, should we send audio and video over UDP instead of TCP? > I think this should be the behaviour instead of introduce long buffering > and delays. Agreed, though that depends on the delay. Some apps like BlueJeans deal with relatively short interruption by pausing and then playing catch-up (i.e. playback is faster than real-time until it catches actual time) > Basically means that in the client we should limit the > queue to 1/2 audio packets and a new packet will replace the queued one. I think that the number of audio packets should still be larger than 1 or 2. The lower that number, the higher the risk of hearing noticeable clicks. I’d say we should probably allow for ~500ms of buffered audio. Christophe > > Frediano > > >> >> Do not offset the time attempting to fix client latency. >> Client should handle it by itself. >> >> This patch is not designed to be merged but more for discussion. >> >> This remove entirely the delay introduced by the server. >> This avoids surely possible time drifts in the client. >> The server just sends it's concept of time without trying to force any >> delay. I think only one end should handle this delay in an attempt to >> synchronize audio and video instead that doing it in both ends. >> >> I'm currently trying it and the responsiveness is much better. >> >> One think I noted is however what happens if I block the connection. >> Think about a momentary disconnection like when you disconnect the a >> cable between the client and the server or when you drop every packets >> between them with a firewall for a short while (this can happens in >> many cases like a restart of a network device, a small fault of one or >> simply you loose signal to your wifi network). The audio restart when >> the connection is fixed (TCP take care of it) but everything then is >> delayed. Looks like instead of catching up the delay is maintained. >> Actually stopping the audio/video remove the delay. >> >> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> >> --- >> server/main-dispatcher.c | 28 ---------------------------- >> server/main-dispatcher.h | 1 - >> server/reds-private.h | 2 -- >> server/reds.c | 25 +++---------------------- >> server/reds.h | 1 - >> server/sound.c | 26 -------------------------- >> server/sound.h | 2 -- >> server/stream.c | 7 ------- >> 8 files changed, 3 insertions(+), 89 deletions(-) >> >> diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c >> index 43f8b79a7..7ecb53f4b 100644 >> --- a/server/main-dispatcher.c >> +++ b/server/main-dispatcher.c >> @@ -146,7 +146,6 @@ main_dispatcher_init(MainDispatcher *self) >> enum { >> MAIN_DISPATCHER_CHANNEL_EVENT = 0, >> MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE, >> - MAIN_DISPATCHER_SET_MM_TIME_LATENCY, >> MAIN_DISPATCHER_CLIENT_DISCONNECT, >> >> MAIN_DISPATCHER_NUM_MESSAGES >> @@ -214,15 +213,6 @@ static void main_dispatcher_handle_migrate_complete(void >> *opaque, >> g_object_unref(mig_complete->client); >> } >> >> -static void main_dispatcher_handle_mm_time_latency(void *opaque, >> - void *payload) >> -{ >> - MainDispatcher *self = opaque; >> - MainDispatcherMmTimeLatencyMessage *msg = payload; >> - reds_set_client_mm_time_latency(self->priv->reds, msg->client, >> msg->latency); >> - g_object_unref(msg->client); >> -} >> - >> static void main_dispatcher_handle_client_disconnect(void *opaque, >> void *payload) >> { >> @@ -249,21 +239,6 @@ void >> main_dispatcher_seamless_migrate_dst_complete(MainDispatcher *self, >> &msg); >> } >> >> -void main_dispatcher_set_mm_time_latency(MainDispatcher *self, RedClient >> *client, uint32_t latency) >> -{ >> - MainDispatcherMmTimeLatencyMessage msg; >> - >> - if (pthread_self() == dispatcher_get_thread_id(DISPATCHER(self))) { >> - reds_set_client_mm_time_latency(self->priv->reds, client, latency); >> - return; >> - } >> - >> - msg.client = g_object_ref(client); >> - msg.latency = latency; >> - dispatcher_send_message(DISPATCHER(self), >> MAIN_DISPATCHER_SET_MM_TIME_LATENCY, >> - &msg); >> -} >> - >> void main_dispatcher_client_disconnect(MainDispatcher *self, RedClient >> *client) >> { >> MainDispatcherClientDisconnectMessage msg; >> @@ -318,9 +293,6 @@ void main_dispatcher_constructed(GObject *object) >> dispatcher_register_handler(DISPATCHER(self), >> MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE, >> main_dispatcher_handle_migrate_complete, >> sizeof(MainDispatcherMigrateSeamlessDstCompleteMessage), >> false); >> - dispatcher_register_handler(DISPATCHER(self), >> MAIN_DISPATCHER_SET_MM_TIME_LATENCY, >> - main_dispatcher_handle_mm_time_latency, >> - sizeof(MainDispatcherMmTimeLatencyMessage), >> false); >> dispatcher_register_handler(DISPATCHER(self), >> MAIN_DISPATCHER_CLIENT_DISCONNECT, >> main_dispatcher_handle_client_disconnect, >> sizeof(MainDispatcherClientDisconnectMessage), >> false); >> diff --git a/server/main-dispatcher.h b/server/main-dispatcher.h >> index 088a5c216..c49d40677 100644 >> --- a/server/main-dispatcher.h >> +++ b/server/main-dispatcher.h >> @@ -51,7 +51,6 @@ GType main_dispatcher_get_type(void) G_GNUC_CONST; >> >> void main_dispatcher_channel_event(MainDispatcher *self, int event, >> SpiceChannelEventInfo *info); >> void main_dispatcher_seamless_migrate_dst_complete(MainDispatcher *self, >> RedClient *client); >> -void main_dispatcher_set_mm_time_latency(MainDispatcher *self, RedClient >> *client, uint32_t latency); >> /* >> * Disconnecting the client is always executed asynchronously, >> * in order to protect from expired references in the routines >> diff --git a/server/reds-private.h b/server/reds-private.h >> index 259496c64..056264c22 100644 >> --- a/server/reds-private.h >> +++ b/server/reds-private.h >> @@ -29,7 +29,6 @@ >> #include "red-record-qxl.h" >> >> #define MIGRATE_TIMEOUT (MSEC_PER_SEC * 10) >> -#define MM_TIME_DELTA 400 /*ms*/ >> >> typedef struct TicketAuthentication { >> char password[SPICE_MAX_PASSWORD_LENGTH]; >> @@ -123,7 +122,6 @@ struct RedsState { >> SpiceBuffer client_monitors_config; >> >> int mm_time_enabled; >> - uint32_t mm_time_latency; >> >> SpiceCharDeviceInstance *vdagent; >> SpiceMigrateInstance *migration_interface; >> diff --git a/server/reds.c b/server/reds.c >> index 97f9219fe..633fdcba4 100644 >> --- a/server/reds.c >> +++ b/server/reds.c >> @@ -1838,7 +1838,7 @@ static void reds_handle_main_link(RedsState *reds, >> RedLinkInfo *link) >> if (!mig_target) { >> main_channel_client_push_init(mcc, >> g_list_length(reds->qxl_instances), >> reds->mouse_mode, reds->is_client_mouse_allowed, >> - reds_get_mm_time() - MM_TIME_DELTA, >> + reds_get_mm_time(), >> reds_qxl_ram_size(reds)); >> if (reds->config->spice_name) >> main_channel_client_push_name(mcc, reds->config->spice_name); >> @@ -1971,7 +1971,7 @@ void >> reds_on_client_semi_seamless_migrate_complete(RedsState *reds, RedClient *c >> main_channel_client_push_init(mcc, g_list_length(reds->qxl_instances), >> reds->mouse_mode, >> reds->is_client_mouse_allowed, >> - reds_get_mm_time() - MM_TIME_DELTA, >> + reds_get_mm_time(), >> reds_qxl_ram_size(reds)); >> reds_link_mig_target_channels(reds, client); >> main_channel_client_migrate_dst_complete(mcc); >> @@ -2670,25 +2670,7 @@ static void reds_send_mm_time(RedsState *reds) >> return; >> } >> spice_debug("trace"); >> - main_channel_push_multi_media_time(reds->main_channel, >> - reds_get_mm_time() - >> reds->mm_time_latency); >> -} >> - >> -void reds_set_client_mm_time_latency(RedsState *reds, RedClient *client, >> uint32_t latency) >> -{ >> - // TODO: multi-client support for mm_time >> - if (reds->mm_time_enabled) { >> - // TODO: consider network latency >> - if (latency > reds->mm_time_latency) { >> - reds->mm_time_latency = latency; >> - reds_send_mm_time(reds); >> - } else { >> - spice_debug("new latency %u is smaller than existing %u", >> - latency, reds->mm_time_latency); >> - } >> - } else { >> - snd_set_playback_latency(client, latency); >> - } >> + main_channel_push_multi_media_time(reds->main_channel, >> reds_get_mm_time()); >> } >> >> static int reds_init_net(RedsState *reds) >> @@ -3083,7 +3065,6 @@ uint32_t reds_get_mm_time(void) >> void reds_enable_mm_time(RedsState *reds) >> { >> reds->mm_time_enabled = TRUE; >> - reds->mm_time_latency = MM_TIME_DELTA; >> reds_send_mm_time(reds); >> } >> >> diff --git a/server/reds.h b/server/reds.h >> index 264ef205c..9b6c1e6ae 100644 >> --- a/server/reds.h >> +++ b/server/reds.h >> @@ -94,7 +94,6 @@ void >> reds_on_client_semi_seamless_migrate_complete(RedsState *reds, RedClient *c >> void reds_on_client_seamless_migrate_complete(RedsState *reds, RedClient >> *client); >> void reds_on_main_channel_migrate(RedsState *reds, MainChannelClient *mcc); >> >> -void reds_set_client_mm_time_latency(RedsState *reds, RedClient *client, >> uint32_t latency); >> uint32_t reds_get_streaming_video(const RedsState *reds); >> GArray* reds_get_video_codecs(const RedsState *reds); >> spice_wan_compression_t reds_get_jpeg_state(const RedsState *reds); >> diff --git a/server/sound.c b/server/sound.c >> index 9073626cd..996bb5208 100644 >> --- a/server/sound.c >> +++ b/server/sound.c >> @@ -972,32 +972,6 @@ SPICE_GNUC_VISIBLE void >> spice_server_playback_put_samples(SpicePlaybackInstance >> snd_send(SND_CHANNEL_CLIENT(playback_client)); >> } >> >> -void snd_set_playback_latency(RedClient *client, uint32_t latency) >> -{ >> - GList *l; >> - >> - for (l = snd_channels; l != NULL; l = l->next) { >> - SndChannel *now = l->data; >> - SndChannelClient *scc = snd_channel_get_client(now); >> - uint32_t type; >> - g_object_get(RED_CHANNEL(now), "channel-type", &type, NULL); >> - if (type == SPICE_CHANNEL_PLAYBACK && scc && >> - red_channel_client_get_client(RED_CHANNEL_CLIENT(scc)) == >> client) { >> - >> - if (red_channel_client_test_remote_cap(RED_CHANNEL_CLIENT(scc), >> - SPICE_PLAYBACK_CAP_LATENCY)) { >> - PlaybackChannelClient* playback = >> (PlaybackChannelClient*)scc; >> - >> - playback->latency = latency; >> - snd_set_command(scc, SND_PLAYBACK_LATENCY_MASK); >> - snd_send(scc); >> - } else { >> - spice_debug("client doesn't not support >> SPICE_PLAYBACK_CAP_LATENCY"); >> - } >> - } >> - } >> -} >> - >> static int snd_desired_audio_mode(bool playback_compression, int frequency, >> bool client_can_celt, bool >> client_can_opus) >> { >> diff --git a/server/sound.h b/server/sound.h >> index 2f0a2b93d..75d6c78cd 100644 >> --- a/server/sound.h >> +++ b/server/sound.h >> @@ -30,6 +30,4 @@ void snd_detach_record(SpiceRecordInstance *sin); >> >> void snd_set_playback_compression(bool on); >> >> -void snd_set_playback_latency(struct RedClient *client, uint32_t latency); >> - >> #endif /* SOUND_H_ */ >> diff --git a/server/stream.c b/server/stream.c >> index 71755ea1f..ca0b49170 100644 >> --- a/server/stream.c >> +++ b/server/stream.c >> @@ -650,9 +650,6 @@ static void update_client_playback_delay(void *opaque, >> uint32_t delay_ms) >> { >> StreamAgent *agent = opaque; >> DisplayChannelClient *dcc = agent->dcc; >> - RedChannel *channel = >> red_channel_client_get_channel(RED_CHANNEL_CLIENT(dcc)); >> - RedClient *client = >> red_channel_client_get_client(RED_CHANNEL_CLIENT(dcc)); >> - RedsState *reds = red_channel_get_server(channel); >> >> dcc_update_streams_max_latency(dcc, agent); >> >> @@ -660,10 +657,6 @@ static void update_client_playback_delay(void *opaque, >> uint32_t delay_ms) >> if (delay_ms > dcc_get_max_stream_latency(dcc)) { >> dcc_set_max_stream_latency(dcc, delay_ms); >> } >> - spice_debug("resetting client latency: %u", >> dcc_get_max_stream_latency(dcc)); >> - main_dispatcher_set_mm_time_latency(reds_get_main_dispatcher(reds), >> - client, >> - >> dcc_get_max_stream_latency(agent->dcc)); >> } >> >> static void bitmap_ref(gpointer data) > > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel