[Spice-devel] [spice-gtk 1/2] Fix possible multimedia time overflow
Christophe de Dinechin
dinechin at redhat.com
Fri Mar 17 12:15:50 UTC 2017
> On 16 Mar 2017, at 11:56, Frediano Ziglio <fziglio at redhat.com <mailto:fziglio at redhat.com>> wrote:
>
>>
>> ----- Original Message -----
>>>>
>>>> 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>>
>>>>> ---
>>>>> 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
>>>
>>
>> Ok your solution is also fine to me.
>>
>> I think git blame could give that kind of information, or just a comment
>> above the line.
>>
>
> yes, but this require to git blame every time you look at the source.
>
>> Obviously we have similar code in mjpeg and gst units, so some refactoring
>> would avoid the duplication, not only of the diff.
>>
>
> definitively, but there are also multiple arithmetic on the same
> source.
>
>>> 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
>>
>> Ok, I am not an expert in portability, do you have an example where this
>> would give different results?
>>
>
> If int is 64 bit (uint32_t) 3 - (uint32_t) 4294967295 == -4294967292.
> It seems weird but is due to the integral promotion rules.
Yes, but it does not matter. Once you cast that back down to 32-bits, you get 4. The 64-bit pattern is 0xffffffff00000004, which will convert to 0x4 whether you do a signed or unsigned 32->64 bit extension.
Back to the original code, I think the function comment is fine:
+/* 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.
+ */
However, the comment inside the function is very confusing:
/* Fast not fully portable version.
+ * If you are paranoid
+ * (gint32) ((((time1 - time2) & 0xffffffffu) ^ 0x80000000) - 0x80000000u)
This is not better in particular because you added a mix of signed and unsigned ints (the first 0x80000000 is signed), and you introduce additional potential integer promotions in the middle. So I would just get rid of that comment.
The necessary and sufficient code is:
(gint32) (time2 - time1)
That being said, I would rewrite the function as follows for a different reason:
/* Return true if time2 is after time1 */
static inline bool mm_time_after(guint32 time1, guint32 time2)
{
return (gint32) (time2 - time1) > 0;
}
The reason is that we really care about “are we after”, and I find this clarifies the intent.
> Similar to
> printf("%d\n", (uint16_t) 10 - (uint16_t) 16);
> which give -6 (but maybe some people could point out that this result is
> not portable either... from my knowledge it is).
As far as I know, it is, except if you find a machine where int is 8 bits, in which case it’s the %d that is not portable ;-)
Christophe
>
>>>
>>> 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.
>>
>> Yeah, some test cases would be useful to understand the problem and the
>> solution.
>>
>
> Done
>
>> Thanks
>>
>>>
>>>> ts before the last frame received to check if we overlapped, I think
>>>> (otherwise we should consider this frame as a late frame)
>>>>
>>>>
>>>
>
> Frediano
> _______________________________________________
> 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/07034908/attachment-0001.html>
More information about the Spice-devel
mailing list