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

Marc-André Lureau mlureau at redhat.com
Thu Mar 16 10:33:41 UTC 2017


----- 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 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);
> > > +}
> > 
> > Fast? I have hard time to understand how that above version would be
> > faster,
> > and I hope my compiler is smart enough to optimize that simple math and
> > type
> > cast.
> > 
> 
> The comment document the code, what I said is that the code I wrote is
> faster than the alternate (in the comment) version.
> If is confusing  I can remove the alternate version.
> 
> > But what are you trying to compute here? A few examples / tests could help
> > to
> > reason about it.
> > 
> > Why do you need a helper function if you can simply cast time1 - time2 to
> > gint32 ?
> > 
> 
> From the comment:
> 
>  * So define a function to compute the difference and
>  * use as documentation for the code.
> 
> yes, the guint32 cast is not necessary however if you write in the code
> 
>    if ((gint32) (time1 - time2) < 0)
> 
> is not clear why this is not the same as
> 

Ok your solution is also fine to me.

I think git blame could give that kind of information, or just a comment above the line.

Obviously we have similar code in mjpeg and gst units, so some refactoring would avoid the duplication, not only of the diff.

>    if (time1 < time2)
> 
> with the function people reading the code can understand that you need
> some more effort to compute the difference and will look at the function
> and documentation. You could document the inline of the function but
> to make sure to avoid people miscomputing you'll have to have a comment
> for every computation, easier to have in a single place.
> 
> > Let's take an example where it overlaps (I don't know if the server really
> > handles that).
> > 
> > "now" is MAXUINT32, and new frame time is say 3:  3 - 4294967295 = 4.
> > That's
> > what is expected, no? However we would need to check if the new frame has a
> 
> 3 - 4294967295 == 4 is not a portable assumption. Neither is
> (uint32_t) 3 - (uint32_t) 4294967295 == 4

Ok, I am not an expert in portability, do you have an example where this would give different results?

> 
> if time1 == UINT32_MAX and time2 == 3 I expect 4 while if
> time2 == UINT32_MAX and time2 == 3 (this can happen for different reasons)
> I expect -4. This make computation easier.
> 
> I'll add this example.

Yeah, some test cases would be useful to understand the problem and the solution.

Thanks

> 
> > ts before the last frame received to check if we overlapped, I think
> > (otherwise we should consider this frame as a late frame)
> > 
> > 
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 


More information about the Spice-devel mailing list