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@xxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel