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

[Frediano Ziglio]

> > On 16 Mar 2017, at 11:56, Frediano Ziglio < fzig...@redhat.com > wrote:
> 

> > > - 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 < fzig...@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 

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

[Christophe de Dinechin]

> On 16 Mar 2017, at 11:56, Frediano Ziglio  > wrote:
> 
>> 
>> - 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  >
> ---
> 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, 

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

[Marc-André Lureau]

Hi

- Original Message -
> > 
> > Hi
> > 
> > - Original Message -
> > > > > > > 
> > > > > > > 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 int is 64 bit (uint32_t) 3 - (uint32_t) 4294967295 == -4294967292.
> > > > > It seems weird but is due to the integral promotion rules.
> > > > 
> > > > 
> > > > According to C standard, there is no integer promotion when both
> > > > operands
> > > > are
> > > > of the same type:
> > > > http://c0x.coding-guidelines.com/6.3.1.8.html:
> > > >  712 If both operands have the same type, then no further conversion is
> > > >  needed.
> > > > 
> > > 
> > > You are skipping 710 and 711.
> > 
> > right, but then all platforms we care about use 32bit int afaik, so no
> > promotion occurs. I think we could add a strict check in configure, that
> > would make it clear. Then we don't have to half the max delay. But couldn't
> > we find a more solid solution to handle time warp, by remembering the last
> > time?
> > 
> 
> Yes, currently all platforms we support have int of 32 bit and ILP64/SILP64
> platforms are quite rare however being client is more likely to require
> a port to new systems in the future.
> Usually integers are not supposed to overflow (if an error does not happen)
> so it's not a problem to port to such platforms unless, like in this case,
> you have overflows. I don't see much issue to maintain the patch I proposed,
> is enough documented (and if not comments can be added) and not so big.
> Creating a monotonic multimedia time can be a bit challenging as the time
> can drift a bit so potentially you could have sequences like 1, 5, 2, 8, 11.
> The time depends on a server monotonic timer and a client monotonic one,
> if you have large drifts (days) you are having some severe system bugs in
> either the server or the client. In case of migration the reference get
> updated so it's not an issue (the frame timers get dropped during VM
> migration),
> in case of suspend/resume the client get disconnected and the timers on
> the server frames get dropped.
> I actually was thinking you were joking about the 42/24 days... why would
> we need such constraint to require such changes?

This fix is quite theorical, so is the limitation or new constrains you add.

I have 2 questions I would like to answer before accepting your change:
- is there a better way to handle time warping without involving integer 
arithmetic and adding new constrains
- is the current code good enough in case the arch has 32bit int? (no integer 
promotion)  
 
> 
> > > 
> > > > 
> > > > > Similar to
> > > > >printf("%d\n", (uint16_t) 10 - (uint16_t) 16);
> > > > > which give -6 (but maybe some people could point out that this result
> > > > > is
> > > > > not portable either... from my knowledge it is).
> > > > > 
> > > 
> > > This would be a gcc bug then, right ?
> > > I personally tested with different compilers too. Unless they all are
> > > not following C rules integer promotion happens.
> > > 
> > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > 
> > > > > Done
> > > > > 
> > > > > > 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> 
> Hi
> 
> - Original Message -
> > > > > > 
> > > > > > 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 int is 64 bit (uint32_t) 3 - (uint32_t) 4294967295 == -4294967292.
> > > > It seems weird but is due to the integral promotion rules.
> > > 
> > > 
> > > According to C standard, there is no integer promotion when both operands
> > > are
> > > of the same type:
> > > http://c0x.coding-guidelines.com/6.3.1.8.html:
> > >  712 If both operands have the same type, then no further conversion is
> > >  needed.
> > > 
> > 
> > You are skipping 710 and 711.
> 
> right, but then all platforms we care about use 32bit int afaik, so no
> promotion occurs. I think we could add a strict check in configure, that
> would make it clear. Then we don't have to half the max delay. But couldn't
> we find a more solid solution to handle time warp, by remembering the last
> time?
> 

Yes, currently all platforms we support have int of 32 bit and ILP64/SILP64
platforms are quite rare however being client is more likely to require
a port to new systems in the future.
Usually integers are not supposed to overflow (if an error does not happen)
so it's not a problem to port to such platforms unless, like in this case,
you have overflows. I don't see much issue to maintain the patch I proposed,
is enough documented (and if not comments can be added) and not so big.
Creating a monotonic multimedia time can be a bit challenging as the time
can drift a bit so potentially you could have sequences like 1, 5, 2, 8, 11.
The time depends on a server monotonic timer and a client monotonic one,
if you have large drifts (days) you are having some severe system bugs in
either the server or the client. In case of migration the reference get
updated so it's not an issue (the frame timers get dropped during VM migration),
in case of suspend/resume the client get disconnected and the timers on
the server frames get dropped.
I actually was thinking you were joking about the 42/24 days... why would
we need such constraint to require such changes?

> > 
> > > 
> > > > Similar to
> > > >printf("%d\n", (uint16_t) 10 - (uint16_t) 16);
> > > > which give -6 (but maybe some people could point out that this result
> > > > is
> > > > not portable either... from my knowledge it is).
> > > > 
> > 
> > This would be a gcc bug then, right ?
> > I personally tested with different compilers too. Unless they all are
> > not following C rules integer promotion happens.
> > 
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > 
> > > > Done
> > > > 
> > > > > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Marc-André Lureau]

Hi

- Original Message -
> > 
> > - 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 
> > > > > ---
> > > > >  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 

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

[Frediano Ziglio]

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

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

[Marc-André Lureau]

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

Re: [Spice-devel] [spice-gtk 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 
> > ---
> >  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 */
> >  

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

[Marc-André Lureau]

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

[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 
---
 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,