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

[Christophe de Dinechin]

> On 17 Jul 2017, at 13:49, Christophe Fergeau <cfergeau at redhat.com <mailto:cfergeau at redhat.com>> wrote:
> 
> On Mon, Jul 17, 2017 at 11:44:04AM +0200, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin at redhat.com <mailto: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 <mailto: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?

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…


> 
> Acked-by: Christophe Fergeau <cfergeau at redhat.com <mailto:cfergeau at redhat.com>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170717/26dc3540/attachment-0001.html>