[Spice-devel] [client v3 1/6] channel-display: Minimize the stream lag by ignoring the server one

[Kevin Pouget]

Hello,

I looked at the patch series and tried to describe its modifications
to better understand it; you'll find below my comments. François, in
the middle of it you'll find a few nit suggestions to improve the
readability.

the patch for display_handle_stream_data() is obviously the most
complex, I'll have to get back to it, it was too complicated for my
first glance


best regards,

Kevin

---


# [client,v3,1/6] channel-display: Minimize the stream lag by ignoring
the server one

display_handle_stream_data: complex modifications.

these two aspects of the commit message are complex to understand in
the code:

> - Delay adjustments are gradual, speeding up or slowing down video
>   playback until the average margin matches the target lag.
> - Changes in the average margin are tracked (see margin_spread) to
>   avoid nudging the delay in response to minor random variations.


this function is called 4 times, it could be factorized to improve the
readability:

> spice_mmtime_diff(op->multi_media_time, st->last_frame_mm_time)


+        /* Initialize the time offset.
+         * Note that UNSET_CLIENT_TIME_OFFSET can be any value including 0
+         * which is common on low-latency LANs. But an unlikely one helps when
+         * adding a trace in this codepath.
+         */

I think s/UNSET_CLIENT_TIME_OFFSET/s->client_time_offset/ would make
more sense in this comment.


stream_get_time(stream): function modified to return the system
monotonic time instead of spice_session_get_mm_time(session), which
was the system time + session->mm_time_offset.


spice_session_mmtime2client_time: new function that (a) update
session->priv->client_time_offset, which is the offset between the
client (local) time and the packets (server) time; (b) returns the
client-adjusted packet mmtime.

The default offset (now-mmtime) is used if
- no offset is currently set
- the current offset is > +/- 1s
- the current offset is > the default offset

---

# [client,v3,2/6] playback: Use the audio timestamps for the global
mmtime conversion

this patch calls spice_session_mmtime2client_time() when audio packets
arrive, to improve the accuracy of session->mm_time_offset

---

# [client,v3,3/6] channel-display: No need to rechedule on mmtime offset
changes

this patch removes the rescheduling functions:

struct VideoDecoder::void (*reschedule)(): this function pointer was
  used to force the reschedule of a frame (= modify the timer that
  will call display_frame() at the right time)

spice_gst_decoder_reschedule(): idem
mjpeg_decoder_reschedule(): idem

display_session_mm_time_reset_cb(): handler for "mm-time-reset"
signal. This signal was used when a migration between two spice
servers with different mm-times occured. In this case,
video_decoder->reschedule() was called to reschedule the display of
the current frame.

The "mm-time-reset" signal is emitted when current_time < old_time or
current_time > old_time+MM_TIME_DIFF_RESET_THRESH (0.5 sec).

The "mm-time-reset" signal might still be relevant, although at the
moment is has no client.

---

# [client,v3,4/6] channel-display: Remove playback_sync_drops_seq_len

this patch removes "struct
display_stream::playback_sync_drops_seq_len" field and the operations
related to it:

define STREAM_PLAYBACK_SYNC_DROP_SEQ_LEN_LIMIT 5

display_stream_stats_save(): this function was counting the number of
frame dropped in sequence.

display_handle_stream_data(): if the adaptive streaming was enabled
and the count of consecutive frames dropped was above a threshold, it
called spice_session_sync_playback_latency() and reset the drop count.

spice_session_sync_playback_latency(): removed. If there were an
active playback channel, it called
spice_playback_channel_sync_latency().

spice_playback_channel_sync_latency(): removed. This function
triggered the "min-latency" channel signal.

This "min-latency" channel signal is used for spice-pulse to react to
the min-latency attribute change.

It is not clear to me when reading this patch where the min-latency
was changed, in regards to the frame dropped.

---

# [client,v3,5/6] spice-session: Keep track of the global streams lag

struct VideoDecoder::decoding_time: new field

mjpeg_decoder_decode_frame: compute and save the moving average of the
decoding time (decoder->base.decoding_time)
sink_event_probe: idem

spice_display_channel_get_lag: new function computing the max
decoder_lag for all the channel streams. Could be modified to use
guards instead of big if() {...}

display_handle_stream_data: if spice_session_lag_needs_update(), call
spice_session_update_lag(). Has a 'FIXME'.

spice_playback_channel_get_lag: renamed from
spice_playback_channel_get_latency.

spice_playback_channel_set_delay: updated to update the lag if
necessary.

spice_session_get_lag: new function. Returns session->priv->lag.
spice_session_update_lag: new function. Could be modified to use
guards instead of big if() {...}. If the session_lag_needs_update(),
computes the max channel lag and calls
spice_playback_channel_update_lag() if the lag value has changed.

spice_session_get_playback_lag: renamed from
spice_session_get_playback_latency.

spice_playback_channel_update_lag: new function. Sets the min_latency
and triggers the "min-latency" signal if the channel is active.

spice_session_lag_needs_update(old_lag, new_lag): new macro. Returns
true is there is no old lag, or if the distance between old and new is
> 10.

---

# [client,v3,6/6] mjpeg: Take the decoding time into account to
  display frames

mjpeg_decoder_schedule: modified to substract the frame decoding time
when setting the the frame decoding timer (only if the wait time is
greater than the decoding time).