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).
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel