[Spice-devel] [PATCH spice-gtk] spice-gtk: Use time comparisons that still work after wraparound
Christophe de Dinechin
christophe.de.dinechin at gmail.com
Mon Jul 17 08:46:06 UTC 2017
> On 17 Jul 2017, at 10:33, Christophe Fergeau <cfergeau at redhat.com> wrote:
>
> On Thu, Jul 13, 2017 at 04:21:36PM +0200, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin at redhat.com>
>>
>> The mm timer is a millisecond timer that wraps around after ~49 days.
>> All comparisons that look like a<b will fail once this happens.
>> Instead, use signed ((int)(a-b)<0), which may fail if there is more than
>> 25 days between a and b (should not be happening under normal conditions),
>> but is robust to the timer wrapping around.
>
> Have you considered having a spice_mm_difftime() helper rather than
> introducing 4 different ones? (less than, strictly less than, more than,
> strictly more than)
Well, you are right, spice_mm_difftime(now, then) < 0 may be easier to read… It also opens options with things like spice_mm_difftime(now, then) < threshold. Will do that.
Christophe
>
> Christophe
>
>>
>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
>> ---
>> src/channel-display-gst.c | 4 ++--
>> src/channel-display-mjpeg.c | 4 ++--
>> src/channel-display.c | 2 +-
>> src/channel-playback.c | 2 +-
>> src/spice-channel-priv.h | 5 +++++
>> src/spice-session.c | 4 ++--
>> 6 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
>> index 3cf451b..495036f 100644
>> --- a/src/channel-display-gst.c
>> +++ b/src/channel-display-gst.c
>> @@ -166,7 +166,7 @@ static void schedule_frame(SpiceGstDecoder *decoder)
>> break;
>> }
>>
>> - if (now < gstframe->frame->mm_time) {
>> + if (spice_mm_strictly_before(now, gstframe->frame->mm_time)) {
>> decoder->timer_id = g_timeout_add(gstframe->frame->mm_time - now,
>> display_frame, decoder);
>> } else if (g_queue_get_length(decoder->display_queue) == 1) {
>> @@ -509,7 +509,7 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>> return TRUE;
>> }
>>
>> - if (frame->mm_time < decoder->last_mm_time) {
>> + if (spice_mm_strictly_before(frame->mm_time, decoder->last_mm_time)) {
>> SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
>> " resetting stream",
>> frame->mm_time, decoder->last_mm_time);
>> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
>> index ee33b01..c91fa53 100644
>> --- a/src/channel-display-mjpeg.c
>> +++ b/src/channel-display-mjpeg.c
>> @@ -201,7 +201,7 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
>> decoder->cur_frame = NULL;
>> do {
>> if (frame) {
>> - if (time <= frame->mm_time) {
>> + if (spice_mm_before(time, frame->mm_time)) {
>> guint32 d = frame->mm_time - time;
>> decoder->cur_frame = frame;
>> decoder->timer_id = g_timeout_add(d, mjpeg_decoder_decode_frame, decoder);
>> @@ -251,7 +251,7 @@ static gboolean mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
>>
>> last_frame = g_queue_peek_tail(decoder->msgq);
>> if (last_frame) {
>> - if (frame->mm_time < last_frame->mm_time) {
>> + if (spice_mm_strictly_before(frame->mm_time, last_frame->mm_time)) {
>> /* This should really not happen */
>> SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
>> " resetting stream",
>> diff --git a/src/channel-display.c b/src/channel-display.c
>> index 3c98d79..fe7506e 100644
>> --- a/src/channel-display.c
>> +++ b/src/channel-display.c
>> @@ -1366,7 +1366,7 @@ static void display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
>> }
>>
>> if (st->report_num_frames >= st->report_max_window ||
>> - now - st->report_start_time >= st->report_timeout ||
>> + spice_mm_after(now - st->report_start_time, st->report_timeout) ||
>> st->report_drops_seq_len >= STREAM_REPORT_DROP_SEQ_LEN_LIMIT) {
>> SpiceMsgcDisplayStreamReport report;
>> SpiceSession *session = spice_channel_get_session(SPICE_CHANNEL(channel));
>> diff --git a/src/channel-playback.c b/src/channel-playback.c
>> index ca14b96..451dfaf 100644
>> --- a/src/channel-playback.c
>> +++ b/src/channel-playback.c
>> @@ -313,7 +313,7 @@ static void playback_handle_data(SpiceChannel *channel, SpiceMsgIn *in)
>> packet->time, packet->data, packet->data_size);
>> #endif
>>
>> - if (c->last_time > packet->time)
>> + if (spice_mm_strictly_after(c->last_time, packet->time))
>> g_warn_if_reached();
>>
>> c->last_time = packet->time;
>> diff --git a/src/spice-channel-priv.h b/src/spice-channel-priv.h
>> index 7288920..b896aaa 100644
>> --- a/src/spice-channel-priv.h
>> +++ b/src/spice-channel-priv.h
>> @@ -43,6 +43,11 @@ G_BEGIN_DECLS
>> #define CHANNEL_DEBUG(channel, fmt, ...) \
>> SPICE_DEBUG("%s: " fmt, SPICE_CHANNEL(channel)->priv->name, ## __VA_ARGS__)
>>
>> +#define spice_mm_after(now, time) ((int32_t) ((now)-(time)) >= 0)
>> +#define spice_mm_before(now, time) ((int32_t) ((now)-(time)) <= 0)
>> +#define spice_mm_strictly_after(now, time) ((int32_t) ((now)-(time)) > 0)
>> +#define spice_mm_strictly_before(now, time) ((int32_t) ((now)-(time)) < 0)
>> +
>> struct _SpiceMsgOut {
>> int refcount;
>> SpiceChannel *channel;
>> diff --git a/src/spice-session.c b/src/spice-session.c
>> index 6f8cf5e..62b041e 100644
>> --- a/src/spice-session.c
>> +++ b/src/spice-session.c
>> @@ -2336,8 +2336,8 @@ void spice_session_set_mm_time(SpiceSession *session, guint32 time)
>> s->mm_time = time;
>> s->mm_time_at_clock = g_get_monotonic_time();
>> SPICE_DEBUG("set mm time: %u", spice_session_get_mm_time(session));
>> - if (time > old_time + MM_TIME_DIFF_RESET_THRESH ||
>> - time < old_time) {
>> + if (spice_mm_strictly_after(time, old_time + MM_TIME_DIFF_RESET_THRESH) ||
>> + spice_mm_strictly_before(time, old_time)) {
>> SPICE_DEBUG("%s: mm-time-reset, old %u, new %u", __FUNCTION__, old_time, s->mm_time);
>> g_coroutine_signal_emit(session, signals[SPICE_SESSION_MM_TIME_RESET], 0);
>> }
>> --
>> 2.11.0 (Apple Git-81)
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list