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

[Snir Sheriber]

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.


>
>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-channel-display-set-0-latency-if-there-is-no-playbac.patch
Type: text/x-patch
Size: 2889 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190103/133f105d/attachment.bin>