[Spice-devel] [spice-gtk 1/2] Fix possible multimedia time overflow
Frediano Ziglio
fziglio at redhat.com
Fri Mar 17 14:26:34 UTC 2017
> > On 16 Mar 2017, at 11:56, Frediano Ziglio < 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 >
> > > > >
> > > >
> > >
> >
>
> > > > > > ---
> > > > >
> > > >
> > >
> >
>
> > > > > > 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.
Yes, but the initial comment did not have the cast.
> 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.
Removed in v2. Beside the gint32 (which should be int_least32_t) this would support platforms not having 32bit ints (not that I know one and I don't think our code can support them either). You can find similar code in binutils.
> The necessary and sufficient code is:
> (gint32) (time2 - time1)
Yes, this is in v2.
> 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.
In some paths we need the difference, for instance to schedule the rendering.
> > 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 ;-)
I never met such platform and surely we are not going to support any <32bit platforms.
As far as I know int are from 16 bits to 64 bits.
> 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)
> > > >
> > >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170317/db4c0075/attachment-0001.html>
More information about the Spice-devel
mailing list