[Spice-devel] [PATCH spice-gtk v2] spice-gtk: Use time comparisons that still work after wraparound

[Christophe Fergeau]

On Mon, Jul 17, 2017 at 11:44:04AM +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.
> 
> 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    | 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?

Acked-by: Christophe Fergeau <cfergeau at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170717/143315e2/attachment-0001.sig>