[Spice-devel] [spice-gtk 1/2] Fix possible multimedia time overflow
Frediano Ziglio
fziglio at redhat.com
Thu Mar 16 10:20:43 UTC 2017
>
> 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
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
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.
> ts before the last frame received to check if we overlapped, I think
> (otherwise we should consider this frame as a late frame)
>
>
Frediano
More information about the Spice-devel
mailing list