[Spice-devel] [spice-gtk 1/2] Fix possible multimedia time overflow

[Frediano Ziglio]

The multimedia time can easily overflow as is encoded in an
unsigned 32 bit and have a unit of milliseconds so it wrap
up every 49 days. Use some math that allow the number to overflow
without issues.
This could caused the client to stop handling streaming and
starting only queueing.

Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
---
 src/channel-display-gst.c   | 11 ++++++-----
 src/channel-display-mjpeg.c | 14 ++++++++------
 src/channel-display-priv.h  | 15 +++++++++++++++
 3 files changed, 29 insertions(+), 11 deletions(-)

This patch should be applied independently on 2/2 as intended to be
merged upstream as a fix while 2/2 depends on this as it change same
code portions.

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index c4190b2..9c62e67 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -183,8 +183,9 @@ static void schedule_frame(SpiceGstDecoder *decoder)
         }
 
         SpiceStreamDataHeader *op = spice_msg_in_parsed(frame->msg);
-        if (now < op->multi_media_time) {
-            decoder->timer_id = g_timeout_add(op->multi_media_time - now,
+        gint32 time_diff = compute_mm_time_diff(op->multi_media_time, now);
+        if (time_diff >= 0) {
+            decoder->timer_id = g_timeout_add(time_diff,
                                               display_frame, decoder);
         } else if (g_queue_get_length(decoder->display_queue) == 1) {
             /* Still attempt to display the least out of date frame so the
@@ -192,8 +193,8 @@ static void schedule_frame(SpiceGstDecoder *decoder)
              */
             decoder->timer_id = g_timeout_add(0, display_frame, decoder);
         } else {
-            SPICE_DEBUG("%s: rendering too late by %u ms (ts: %u, mmtime: %u), dropping",
-                        __FUNCTION__, now - op->multi_media_time,
+            SPICE_DEBUG("%s: rendering too late by %d ms (ts: %u, mmtime: %u), dropping",
+                        __FUNCTION__, -time_diff,
                         op->multi_media_time, now);
             stream_dropped_frame_on_playback(decoder->base.stream);
             g_queue_pop_head(decoder->display_queue);
@@ -431,7 +432,7 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
     }
 
     SpiceStreamDataHeader *frame_op = spice_msg_in_parsed(frame_msg);
-    if (frame_op->multi_media_time < decoder->last_mm_time) {
+    if (compute_mm_time_diff(frame_op->multi_media_time, decoder->last_mm_time) < 0) {
         SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
                     " resetting stream, id %u",
                     frame_op->multi_media_time,
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 722494e..1b7373b 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -195,15 +195,15 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
     do {
         if (frame_msg) {
             SpiceStreamDataHeader *op = spice_msg_in_parsed(frame_msg);
-            if (time <= op->multi_media_time) {
-                guint32 d = op->multi_media_time - time;
+            gint32 time_diff = compute_mm_time_diff(op->multi_media_time, time);
+            if (time_diff >= 0) {
                 decoder->cur_frame_msg = frame_msg;
-                decoder->timer_id = g_timeout_add(d, mjpeg_decoder_decode_frame, decoder);
+                decoder->timer_id = g_timeout_add(time_diff, mjpeg_decoder_decode_frame, decoder);
                 break;
             }
 
-            SPICE_DEBUG("%s: rendering too late by %u ms (ts: %u, mmtime: %u), dropping ",
-                        __FUNCTION__, time - op->multi_media_time,
+            SPICE_DEBUG("%s: rendering too late by %d ms (ts: %u, mmtime: %u), dropping ",
+                        __FUNCTION__, -time_diff,
                         op->multi_media_time, time);
             stream_dropped_frame_on_playback(decoder->base.stream);
             spice_msg_in_unref(frame_msg);
@@ -249,7 +249,9 @@ static gboolean mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
         SpiceStreamDataHeader *last_op, *frame_op;
         last_op = spice_msg_in_parsed(last_msg);
         frame_op = spice_msg_in_parsed(frame_msg);
-        if (frame_op->multi_media_time < last_op->multi_media_time) {
+        gint32 time_diff =
+            compute_mm_time_diff(frame_op->multi_media_time, last_op->multi_media_time);
+        if (time_diff < 0) {
             /* This should really not happen */
             SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
                         " resetting stream, id %u",
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index b9c08a3..3cd0727 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -141,6 +141,21 @@ void stream_dropped_frame_on_playback(display_stream *st);
 void stream_display_frame(display_stream *st, SpiceMsgIn *frame_msg, uint32_t width, uint32_t height, uint8_t *data);
 uint32_t spice_msg_in_frame_data(SpiceMsgIn *frame_msg, uint8_t **data);
 
+/* Compute the difference between 2 multimedia times.
+ * Multimedia times are subject to overflow so the check
+ * for time1 < time2 does not always indicate that time2
+ * happens after time1.
+ * So define a function to compute the difference and
+ * use as documentation for the code.
+ */
+static inline gint32 compute_mm_time_diff(guint32 time1, guint32 time2)
+{
+    /* Fast not fully portable version.
+     * If you are paranoid
+     * (gint32) ((((time1 - time2) & 0xffffffffu) ^ 0x80000000) - 0x80000000u)
+     * is more portable */
+    return (gint32) (guint32) (time1 - time2);
+}
 
 G_END_DECLS
 
-- 
2.9.3