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>
> ---
> 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.
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 ?
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 ts before the last frame received to check if we overlapped, I think (otherwise we should consider this frame as a late frame)
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel