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

[Frediano Ziglio]

> 
> 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 at 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