Created attachment 302180 [details] [review]
W32: Detect ticks wraparound

Review of attachment 302180 [details] [review]:

See comments.

::: gdk/win32/gdkevents-win32.c
@@ +176,2 @@
   if (suggested_tick == 0)
     suggested_tick = GetTickCount ();

leave empty line between different ifs. I know it was like that before but anyway.

@@ +178,3 @@
+  /* Ticks eventually wrap around.
+   * This works as long as the interval between ticks is < 2147483648ms */
+  if (suggested_tick <= cur_tick && ((cur_tick - suggested_tick) < 0x7FFFFFFF))

after reading this comment and the code I do still do not understand it. Can you please specify a bit more in the comment?

_gdk_win32_get_next_tick() is called with a reported timestamp most of the time (and only once - with 0, initially).

Originally code checked reported timestamp (suggested_tick) and returned statically stored previous result if suggested_tick was lower (to avoid jumping back in time).

However, ticks *do* jump back in time once the variable overflows. I modified the check to detect such big jumps and don't consider them erroneous. If that is not done, this function will keep returning the last forward-time timestamp it got indefinitely.

I should probably also initialize our_tick to GetTickCount() too.
Also, i should probably switch from 0 in the initial call to GetMessageTime().

Review of attachment 302180 [details] [review]:

::: gdk/win32/gdkevents-win32.c
@@ +178,3 @@
+  /* Ticks eventually wrap around.
+   * This works as long as the interval between ticks is < 2147483648ms */
+  if (suggested_tick <= cur_tick && ((cur_tick - suggested_tick) < 0x7FFFFFFF))

This does not look right. Say we're overflowing and cur_tick is MAXINT, and the new suggested_tick is 1, then suggested_tick <= cur_tick, and (cur_tick - suggested_tick) is (MAXINT - 1), which is > 0x7fffffff. So, we will return cur_tick even thought we *should* wrap.

Shouldn't this just be something like the macro in the X11 code:
#define XSERVER_TIME_IS_LATER(time1, time2)                        \
  ( (( time1 > time2 ) && ( time1 - time2 < ((guint32)-1)/2 )) ||  \
    (( time1 < time2 ) && ( time2 - time1 > ((guint32)-1)/2 ))     \

(In reply to Alexander Larsson from comment #4)
> Review of attachment 302180 [details] [review] [review]:
> 
> ::: gdk/win32/gdkevents-win32.c
> @@ +178,3 @@
> +  /* Ticks eventually wrap around.
> +   * This works as long as the interval between ticks is < 2147483648ms */
> +  if (suggested_tick <= cur_tick && ((cur_tick - suggested_tick) <
> 0x7FFFFFFF))
> 
> This does not look right. Say we're overflowing and cur_tick is MAXINT, and
> the new suggested_tick is 1, then suggested_tick <= cur_tick, and (cur_tick
> - suggested_tick) is (MAXINT - 1), which is > 0x7fffffff. So, we will return
> cur_tick even thought we *should* wrap.
> 
> Shouldn't this just be something like the macro in the X11 code:
> #define XSERVER_TIME_IS_LATER(time1, time2)                        \
>   ( (( time1 > time2 ) && ( time1 - time2 < ((guint32)-1)/2 )) ||  \
>     (( time1 < time2 ) && ( time2 - time1 > ((guint32)-1)/2 ))     \

Um, okay, let's go at this again. Here's the code:

>  if (suggested_tick <= cur_tick && ((cur_tick - suggested_tick) < 0x7FFFFFFF))
>    return cur_tick;
>  else
>    return cur_tick = suggested_tick;

Ticks wrapped, which means that cur_tick is 0xFFFFFFFF (or nearly as much), and suggested_tick is 0 or 1 or some kind of small integer.

(suggested_tick <= cur_tick) is TRUE
(cur_tick - suggested_tick) is (0xFFFFFFFF-or-almost-it minus some-kind-of-small-integer), and is also almost 0xFFFFFFFF.
(0xFFFFFFFF < 0x7FFFFFFF) would be FALSE in that case, and the second branch, which uses suggested_tick (and makes it the new cur_tick), will be chosen.

I don't see anything wrong here.

I've considered other ways of testing for wraps, and this is the sanest i could come up with.

Removing the dependency on bug 748325. Damn, why do i always use this feature and always get it so *wrong*?

Yeah, you're right. The only problem i can see with your patch is if suggested_tick happens to be 0 during a wrap, but in that case we should be calling GetTickCount and get back something like 0, which is probably ok.

Review of attachment 302180 [details] [review]:

please commit

Now that i can push this, i'm starting to have doubts. While in theory this code looks correct, i haven't tested some of its branches. That is, i ran GTK apps with this patch applied for some time, and it works OK in normal circumstances, but i haven't had a chance to see how it handles the actual wraparound (i.e. run a GTK app shortly before GetTickCount() wraps, observe its reaction to the wraparound). I will try to test it (it takes 49.7 days to achieve wraparound).

How does GTK/GDK handle this on other platforms, by the way? I've noticed that GdkEvent time fields are generally spec'ed as guint32, which means they have the same problem.

xserver event times do indeed wrap around, so, the X11 backend uses the above XSERVER_TIME_IS_LATER every time it compares two timestamps.

Does this handle *every* case anyone ever compares to timestamps? Who knows...
We certainly don't have any test case for this.

Sorry, i blew the test (was asleep). I'll commit this now, and maybe
test it some other time.

Attachment 302180 [details] pushed as f74f81f - W32: Detect ticks wraparound