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

Submitted by Frediano Ziglio on March 16, 2017, 10:34 a.m.

Details

Message ID 20170316103442.4591-1-fziglio@redhat.com
State Superseded
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio March 16, 2017, 10:34 a.m.
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@redhat.com>
---
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.
---
 src/channel-display-gst.c   | 11 ++++++-----
 src/channel-display-mjpeg.c | 14 ++++++++------
 src/channel-display-priv.h  | 19 +++++++++++++++++++
 3 files changed, 33 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

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..eced6af 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -141,6 +141,25 @@  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)
+{
+    /* Compute time difference returning a signed value
+     * For instance:
+     * - if time1 == 3 and time2 == UINT32_MAX this means that
+     *   time1 overflowed and is 4 milliseconds after time2;
+     * - if time1 == UINT32_MAX and time2 == 3 this means that
+     *   time2 overflowed and is 4 milliseconds before time2
+     *   (so the difference is -4)
+     */
+    return (gint32) (time1 - time2);
+}
 
 G_END_DECLS
 

Comments

Hi

----- Original Message -----
> 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@redhat.com>
> ---
> 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.
> ---
>  src/channel-display-gst.c   | 11 ++++++-----
>  src/channel-display-mjpeg.c | 14 ++++++++------
>  src/channel-display-priv.h  | 19 +++++++++++++++++++
>  3 files changed, 33 insertions(+), 11 deletions(-)
> 
> 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..eced6af 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -141,6 +141,25 @@ 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)
> +{
> +    /* Compute time difference returning a signed value
> +     * For instance:
> +     * - if time1 == 3 and time2 == UINT32_MAX this means that
> +     *   time1 overflowed and is 4 milliseconds after time2;
> +     * - if time1 == UINT32_MAX and time2 == 3 this means that
> +     *   time2 overflowed and is 4 milliseconds before time2
> +     *   (so the difference is -4)

time2 didn't necessarily overflow, It could also be that time is scheduled for 42d later, right? I am afraid the diff alone isn't enough.

> +     */
> +    return (gint32) (time1 - time2);
> +}
>  
>  G_END_DECLS
>  
> --
> 2.9.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> 
> Hi
> 
> ----- Original Message -----
> > 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@redhat.com>
> > ---
> > 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.
> > ---
> >  src/channel-display-gst.c   | 11 ++++++-----
> >  src/channel-display-mjpeg.c | 14 ++++++++------
> >  src/channel-display-priv.h  | 19 +++++++++++++++++++
> >  3 files changed, 33 insertions(+), 11 deletions(-)
> > 
> > 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..eced6af 100644
> > --- a/src/channel-display-priv.h
> > +++ b/src/channel-display-priv.h
> > @@ -141,6 +141,25 @@ 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)
> > +{
> > +    /* Compute time difference returning a signed value
> > +     * For instance:
> > +     * - if time1 == 3 and time2 == UINT32_MAX this means that
> > +     *   time1 overflowed and is 4 milliseconds after time2;
> > +     * - if time1 == UINT32_MAX and time2 == 3 this means that
> > +     *   time2 overflowed and is 4 milliseconds before time2
> > +     *   (so the difference is -4)
> 
> time2 didn't necessarily overflow, It could also be that time is scheduled
> for 42d later, right? I am afraid the diff alone isn't enough.
> 

Usually we use remote-viewer or similar to see a remote interactive VM,
there would be some issues if the server send a frame to be displayed
42 days in the future... or that is expired 42 days in the past
(I think using signed 32 bit would reduce to 24 days but even 24 days
are too much).

> > +     */
> > +    return (gint32) (time1 - time2);
> > +}
> >  
> >  G_END_DECLS
> >  

Frediano
Hi

----- Original Message -----
> > 
> > Hi
> > 
> > ----- Original Message -----
> > > 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@redhat.com>
> > > ---
> > > 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.
> > > ---
> > >  src/channel-display-gst.c   | 11 ++++++-----
> > >  src/channel-display-mjpeg.c | 14 ++++++++------
> > >  src/channel-display-priv.h  | 19 +++++++++++++++++++
> > >  3 files changed, 33 insertions(+), 11 deletions(-)
> > > 
> > > 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..eced6af 100644
> > > --- a/src/channel-display-priv.h
> > > +++ b/src/channel-display-priv.h
> > > @@ -141,6 +141,25 @@ 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)
> > > +{
> > > +    /* Compute time difference returning a signed value
> > > +     * For instance:
> > > +     * - if time1 == 3 and time2 == UINT32_MAX this means that
> > > +     *   time1 overflowed and is 4 milliseconds after time2;
> > > +     * - if time1 == UINT32_MAX and time2 == 3 this means that
> > > +     *   time2 overflowed and is 4 milliseconds before time2
> > > +     *   (so the difference is -4)

I guess you mean time1 is 4ms before

> > 
> > time2 didn't necessarily overflow, It could also be that time is scheduled
> > for 42d later, right? I am afraid the diff alone isn't enough.
> > 
> 
> Usually we use remote-viewer or similar to see a remote interactive VM,
> there would be some issues if the server send a frame to be displayed
> 42 days in the future... or that is expired 42 days in the past
> (I think using signed 32 bit would reduce to 24 days but even 24 days
> are too much).

So any diff beyond ~UINT32_MAX/2 will be considered negative and thus late (negative delay), right? This is not following the protocol strictly, but I guess it is acceptable.

I think another reasonable option is to stop the streaming after 42d ;) And work on a protocol extension to using uint64! which would provide us with millions years of streaming ;)

I am looking forward to a real test set though.

> 
> > > +     */
> > > +    return (gint32) (time1 - time2);
> > > +}
> > >  
> > >  G_END_DECLS
> > >  
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> On 16 Mar 2017, at 11:49, Marc-André Lureau <mlureau@redhat.com <mailto:mlureau@redhat.com>> wrote:
> 
> Hi
> 
> ----- Original Message -----
>> 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@redhat.com <mailto:fziglio@redhat.com>>
>> ---
>> 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.
>> ---
>> src/channel-display-gst.c   | 11 ++++++-----
>> src/channel-display-mjpeg.c | 14 ++++++++------
>> src/channel-display-priv.h  | 19 +++++++++++++++++++
>> 3 files changed, 33 insertions(+), 11 deletions(-)
>> 
>> 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..eced6af 100644
>> --- a/src/channel-display-priv.h
>> +++ b/src/channel-display-priv.h
>> @@ -141,6 +141,25 @@ 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)
>> +{
>> +    /* Compute time difference returning a signed value
>> +     * For instance:
>> +     * - if time1 == 3 and time2 == UINT32_MAX this means that
>> +     *   time1 overflowed and is 4 milliseconds after time2;
>> +     * - if time1 == UINT32_MAX and time2 == 3 this means that
>> +     *   time2 overflowed and is 4 milliseconds before time2
>> +     *   (so the difference is -4)
> 
> time2 didn't necessarily overflow, It could also be that time is scheduled for 42d later, right? I am afraid the diff alone isn't enough.

Does this really happen? If that’s the case, the only “real” solution is to move to 64-bit, because otherwise, it may work at 42 days but will fail at 49.


> 
>> +     */
>> +    return (gint32) (time1 - time2);
>> +}
>> 
>> G_END_DECLS
>> 
>> --
>> 2.9.3
>> 
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org <mailto:Spice-devel@lists.freedesktop.org>
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
>> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org <mailto:Spice-devel@lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>