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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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)

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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]