[Spice-devel] [spice-gtk v2 1/2] Fix possible multimedia time overflow
Christophe de Dinechin
dinechin at redhat.com
Fri Mar 17 16:26:45 UTC 2017
> On 16 Mar 2017, at 11:49, Marc-André Lureau <mlureau at redhat.com <mailto:mlureau at redhat.com>> wrote:
>
> 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 <mailto: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.
Does this really happen? If that’s the case, the only “real” solution is to move to 64-bit, because otherwise, it may work at 42 days but will fail at 49.
>
>> + */
>> + return (gint32) (time1 - time2);
>> +}
>>
>> G_END_DECLS
>>
>> --
>> 2.9.3
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
>>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170317/5ae46e10/attachment-0001.html>
More information about the Spice-devel
mailing list