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. Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel