[Spice-devel] [spice-gtk v2 1/2] Fix possible multimedia time overflow
Marc-André Lureau
mlureau at redhat.com
Thu Mar 16 10:49:30 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>
> ---
> 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.
> + */
> + return (gint32) (time1 - time2);
> +}
>
> G_END_DECLS
>
> --
> 2.9.3
>
> _______________________________________________
> 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