[Spice-devel] [RFC PATCH spice-server] Send real time to client

Christophe de Dinechin cdupontd at redhat.com
Tue Oct 3 13:07:28 UTC 2017


> On 2 Oct 2017, at 15:43, Frediano Ziglio <fziglio at redhat.com> 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 at redhat.com>
>> ---
>> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



More information about the Spice-devel mailing list