[Spice-devel] [PATCH spice-gtk] channel-display: set 0 latency if there is no playback

[Frediano Ziglio]

> 
> If playback is not active and it's streaming mode set latency to 0
> so frames will not be synchronized with mm time.
> 
> Signed-off-by: Snir Sheriber <ssheribe at redhat.com>
> ---
> 
> This patch is a suggestion to improve current state.
> 
> Latency in display channel is the difference between multimedia time (mm)
> at frame's arrival time and its timestamp, this latency is added to current
> gstreamer's clock and attached to a gst buffer that is pushed into
> gstreamer's
> so it will be played in sync at the right time.
> 
> Currently the mm time is being set when session begins, by the server with
> 400ms delay for buffering so as a result in streaming mode you feel at least
> 400ms latency.
> But when playback starts mm time is set according to playback's timestamp at
> its
> arrival time (so no 400ms delay anymore (a bug? maybe), there is another
> delay
> which is usually smaller)

Yes, kind of a bug, really weird behaviour causing some issue on
bandwidth management on the server.
And possibly this patch will make thing worse as the stream_report is
using this "latency" you are setting, would be better to save the computed
value before resetting it and passing the not 0 one to display_update_stream_report
(after queue_frame call on the same function).

> 
> With this patch, when there is no playback, latency is 0 so buffer's
> timestamp
> is set to only gst clock, means, it won't be synced.
> 
> Theoretically it should be safer to set buffer's timestamp to
> GST_CLOCK_TIME_NONE
> if there is no playback but i tried it and it feels faster and simpler this
> way
> and there were no issues by now.
> 
> ---
>  src/channel-display.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 7c663cb..e31a19c 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1561,7 +1561,15 @@ static void display_handle_stream_data(SpiceChannel
> *channel, SpiceMsgIn *in)
>          st->cur_drops_seq_stats.len++;
>          st->playback_sync_drops_seq_len++;
>      } else {
> -        CHANNEL_DEBUG(channel, "video latency: %d", latency);
> +        SpiceSession *s = spice_channel_get_session(channel);
> +
> +        if (st->surface->streaming_mode &&
> !spice_session_is_playback_active(s)) {

Why also checking for streaming mode? I would just check the playback.

> +            CHANNEL_DEBUG(channel, "video latency: %d, set to 0 since there
> is no playback", latency);
> +            latency = 0;
> +        } else {
> +            CHANNEL_DEBUG(channel, "video latency: %d", latency);
> +        }
> +
>          if (st->cur_drops_seq_stats.len) {
>              st->cur_drops_seq_stats.duration = op->multi_media_time -
>                                                 st->cur_drops_seq_stats.start_mm_time;

Frediano