----- 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@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.
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 samesource. 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.
DoneThanks
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 listSpice-devel@xxxxxxxxxxxxxxxxxxxxxhttps://lists.freedesktop.org/mailman/listinfo/spice-devel
|