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

[Frediano Ziglio]

> 
> On 1/3/19 10:43 AM, Frediano Ziglio wrote:
> >> 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).
> 
> 
> Fix attached.
> 

Acked-by: Frediano Ziglio <fziglio at redhat.com>

> 
> >
> >> 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.
> 
> This is not a big deal but currently if you ignore the latency, frames
> will be presented
> according to their arrival time, so it may mismatch the stream framerate
> a bit.
> So the reason behind this was that if video stream is just part of the
> screen
> you won't feel the latency as much as in streaming mode, so just let it
> sync, although
> there's no audio, so it will play the video frames in timestamps that
> match the framerate.
> 
> 
> Snir.
> 
> 
> >
> >> +            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
>