> > On 17 Jul 2017, at 14:20, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: > > On Mon, Jul 17, 2017 at 02:00:01PM +0200, Christophe de Dinechin wrote: >> >>> On 17 Jul 2017, at 13:49, Christophe Fergeau <cfergeau@xxxxxxxxxx <mailto:cfergeau@xxxxxxxxxx>> wrote: >>> >>> On Mon, Jul 17, 2017 at 11:44:04AM +0200, Christophe de Dinechin wrote: >>>> From: Christophe de Dinechin <dinechin@xxxxxxxxxx <mailto:dinechin@xxxxxxxxxx>> >>>> >>>> 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. >>>> >>>> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx <mailto:dinechin@xxxxxxxxxx>> >>>> --- >>>> 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 | 2 ++ >>>> src/spice-session.c | 4 ++-- >>>> 6 files changed, 10 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c >>>> index adcd6ef..3f51361 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_mmtime_diff(now, gstframe->frame->mm_time) < 0) { >>>> decoder->timer_id = g_timeout_add(gstframe->frame->mm_time - now, >>>> display_frame, decoder); >>>> } else if (g_queue_get_length(decoder->display_queue) == 1) { >>>> @@ -511,7 +511,7 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder, >>>> return TRUE; >>>> } >>>> >>>> - if (frame->mm_time < decoder->last_mm_time) { >>>> + if (spice_mmtime_diff(frame->mm_time, decoder->last_mm_time) < 0) { >>>> 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..3ba098c 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_mmtime_diff(time, frame->mm_time) <= 0) { >>>> 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_mmtime_diff(frame->mm_time, last_frame->mm_time) < 0) { >>>> /* 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..06ed18a 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_mmtime_diff(now - st->report_start_time, st->report_timeout) >= 0 || >>>> 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..afc9059 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_mmtime_diff(c->last_time, packet->time) > 0) >>>> 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..20d3495 100644 >>>> --- a/src/spice-channel-priv.h >>>> +++ b/src/spice-channel-priv.h >>>> @@ -43,6 +43,8 @@ G_BEGIN_DECLS >>>> #define CHANNEL_DEBUG(channel, fmt, ...) \ >>>> SPICE_DEBUG("%s: " fmt, SPICE_CHANNEL(channel)->priv->name, ## __VA_ARGS__) >>>> >>>> +#define spice_mmtime_diff(now, time) ((int32_t) ((now)-(time))) >>> >>> Nit: I think the 'now' and 'time' naming here could be misleading, what >>> about just "mmtime0" and "mmtime1" for example? >> >> Well, that was to match the typical usage where ‘now’ would be the >> current time, and ‘time’ would be some other reference time. To me, >> that’s more readable that way. But YMMV… > > Dunno, looking at spice_mmtime_diff(c->last_time, packet->time), none of > the args strike me as the obvious "current" time. And the function is > generic, spice_mmtime_diff(old_time, current_time) would work equally > well as spice_mmtime_diff(current_time, old_time). So the "now" name > seems very artificial to me. OK. Since you seem to feel more strongly about this than me, I changed it. Pushed to freedesktop.org. Since this is the first time I push myself to this repo, would you please be kind enough to check that what I pushed looks sane? Thanks Christophe > > Christophe > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel