[PATCH 1/3] Revert "ntdll/tests: Add test to check if GetTickCount is updated after sleeping."
This reverts commit 4617f83fcf0a34fe41b0e38dde1567195395efca. The assumption that GetTickCount() gets updated after Sleep(1) holds only when there is no other software running on the system. If any program uses timeBeginPeriod() with values 1-15 then this assumption breaks. A lot of common user software calls timeBeginPeriod() - this includes browsers, media players, voice communicators, IDEs and store fronts. Any software depending on this assumption would also not work for many Windows users. Wine's default behavior is as if there is a process running that has called timeBeginPeriod(1). Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com> --- dlls/ntdll/tests/time.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index a00d507e4e..d756a8c839 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -199,7 +199,7 @@ static void test_user_shared_data_time(void) { KSHARED_USER_DATA *user_shared_data = (void *)0x7ffe0000; ULONGLONG t1, t2, t3; - int i = 0, changed = 0; + int i = 0; i = 0; do @@ -253,15 +253,6 @@ static void test_user_shared_data_time(void) "USD InterruptTime / RtlQueryUnbiasedInterruptTime are out of order %s %s\n", wine_dbgstr_longlong(t2), wine_dbgstr_longlong(t3)); } - - for (i = 0; i < 100; i++) - { - t1 = GetTickCount(); - Sleep(1); - t2 = GetTickCount(); - if (t1 != t2) changed++; - } - todo_wine ok(changed >= 90, "tick count isn't updated after sleeping one millisecond (%d%% correct)\n", changed); } START_TEST(time) -- 2.28.0
Wine's winmm is built on the assumption that we default to 1ms resolution without ever calling timeBeginPeriod(1). For simplicity timeGetTime() was backed by GetTickCount() which recently has changed its implementation and, in worst case scenario, returns a new value each ~16ms. This breaks the mentioned assumption. This patch introduces a proper timeGetTime() that uses QueryPerformanceCounter(). timeGetTime() is now also used instead of GetTickCount() internally by winmm. The "observations" comment was updated with the latest findings. Fixes: cd215bb49bc2 ("kernel32: Use the user shared data to implement GetTickCount().") Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49564 Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com> --- dlls/winmm/time.c | 47 +++++++++++++++++++++++++++++++++---------- dlls/winmm/winmm.c | 12 +++++------ dlls/winmm/winmm.spec | 2 +- 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/dlls/winmm/time.c b/dlls/winmm/time.c index 22c89852a2..9bb948efbd 100644 --- a/dlls/winmm/time.c +++ b/dlls/winmm/time.c @@ -74,17 +74,30 @@ static inline void link_timer( WINE_TIMERENTRY *timer ) /* * Some observations on the behavior of winmm on Windows. - * First, the call to timeBeginPeriod(xx) can never be used - * to raise the timer resolution, only lower it. + * + * First, the call to timeBeginPeriod(xx) can never be used to + * lower the timer resolution (i.e. increase the update + * interval), only to increase the timer resolution (i.e. lower + * the update interval). * * Second, a brief survey of a variety of Win 2k and Win X * machines showed that a 'standard' (aka default) timer * resolution was 1 ms (Win9x is documented as being 1). However, one * machine had a standard timer resolution of 10 ms. * - * Further, if we set our default resolution to 1, - * the implementation of timeGetTime becomes GetTickCount(), - * and we can optimize the code to reduce overhead. + * Further, timeBeginPeriod(xx) also affects the resolution of + * wait calls such as NtDelayExecution() and + * NtWaitForMultipleObjects() which by default round up their + * timeout to the nearest multiple of 15.625ms across all Windows + * versions. In Wine all of those currently work with sub-1ms + * accuracy. + * + * Effective time resolution is a global value that is the max + * of the resolutions (i.e. min of update intervals) requested by + * all the processes. A lot of programs seem to do + * timeBeginPeriod(1) forcing it onto everyone else. + * + * Defaulting to 1ms accuracy in winmm should be safe. * * Additionally, a survey of Event behaviors shows that * if we request a Periodic event every 50 ms, then Windows @@ -97,6 +110,7 @@ static inline void link_timer( WINE_TIMERENTRY *timer ) * no delays. * * Jeremy White, October 2004 + * Arkadiusz Hiler, August 2020 */ #define MMSYSTIME_MININTERVAL (1) #define MMSYSTIME_MAXINTERVAL (65535) @@ -131,7 +145,7 @@ static int TIME_MMSysTimeCallback(void) } timer = LIST_ENTRY( ptr, WINE_TIMERENTRY, entry ); - delta_time = timer->dwTriggerTime - GetTickCount(); + delta_time = timer->dwTriggerTime - timeGetTime(); if (delta_time > 0) break; list_remove( &timer->entry ); @@ -242,16 +256,27 @@ void TIME_MMTimeStop(void) */ MMRESULT WINAPI timeGetSystemTime(LPMMTIME lpTime, UINT wSize) { - if (wSize >= sizeof(*lpTime)) { - lpTime->wType = TIME_MS; - lpTime->u.ms = GetTickCount(); - + lpTime->wType = TIME_MS; + lpTime->u.ms = timeGetTime(); } return 0; } +/************************************************************************** + * timeGetTime [WINMM.@] + */ +DWORD WINAPI timeGetTime(void) +{ + LARGE_INTEGER now, freq; + + QueryPerformanceCounter(&now); + QueryPerformanceFrequency(&freq); + + return (now.QuadPart * 1000) / freq.QuadPart; +} + /************************************************************************** * timeSetEvent [WINMM.@] */ @@ -272,7 +297,7 @@ MMRESULT WINAPI timeSetEvent(UINT wDelay, UINT wResol, LPTIMECALLBACK lpFunc, return 0; lpNewTimer->wDelay = wDelay; - lpNewTimer->dwTriggerTime = GetTickCount() + wDelay; + lpNewTimer->dwTriggerTime = timeGetTime() + wDelay; /* FIXME - wResol is not respected, although it is not clear that we could change our precision meaningfully */ diff --git a/dlls/winmm/winmm.c b/dlls/winmm/winmm.c index 9075a78f34..eea29ea61e 100644 --- a/dlls/winmm/winmm.c +++ b/dlls/winmm/winmm.c @@ -1030,7 +1030,7 @@ static DWORD midistream_get_playing_position(WINE_MIDIStream* lpMidiStrm) case MSM_STATUS_PAUSED: return lpMidiStrm->dwElapsedMS; case MSM_STATUS_PLAYING: - return GetTickCount() - lpMidiStrm->dwStartTicks; + return timeGetTime() - lpMidiStrm->dwStartTicks; default: FIXME("Unknown playing status %hu\n", lpMidiStrm->status); return 0; @@ -1080,7 +1080,7 @@ static BOOL MMSYSTEM_MidiStream_MessageHandler(WINE_MIDIStream* lpMidiStrm, LPWI /* FIXME: send out cc64 0 (turn off sustain pedal) on every channel */ if (lpMidiStrm->status != MSM_STATUS_PLAYING) { EnterCriticalSection(&lpMidiStrm->lock); - lpMidiStrm->dwStartTicks = GetTickCount() - lpMidiStrm->dwElapsedMS; + lpMidiStrm->dwStartTicks = timeGetTime() - lpMidiStrm->dwElapsedMS; lpMidiStrm->status = MSM_STATUS_PLAYING; LeaveCriticalSection(&lpMidiStrm->lock); } @@ -1090,7 +1090,7 @@ static BOOL MMSYSTEM_MidiStream_MessageHandler(WINE_MIDIStream* lpMidiStrm, LPWI /* FIXME: send out cc64 0 (turn off sustain pedal) on every channel */ if (lpMidiStrm->status != MSM_STATUS_PAUSED) { EnterCriticalSection(&lpMidiStrm->lock); - lpMidiStrm->dwElapsedMS = GetTickCount() - lpMidiStrm->dwStartTicks; + lpMidiStrm->dwElapsedMS = timeGetTime() - lpMidiStrm->dwStartTicks; lpMidiStrm->status = MSM_STATUS_PAUSED; LeaveCriticalSection(&lpMidiStrm->lock); } @@ -1126,7 +1126,7 @@ static BOOL MMSYSTEM_MidiStream_MessageHandler(WINE_MIDIStream* lpMidiStrm, LPWI case WINE_MSM_HEADER: /* sets initial tick count for first MIDIHDR */ if (!lpMidiStrm->dwStartTicks) - lpMidiStrm->dwStartTicks = GetTickCount(); + lpMidiStrm->dwStartTicks = timeGetTime(); lpMidiHdr = (LPMIDIHDR)msg->lParam; lpData = (LPBYTE)lpMidiHdr->lpData; TRACE("Adding %s lpMidiHdr=%p [lpData=0x%p dwBytesRecorded=%u/%u dwFlags=0x%08x size=%lu]\n", @@ -1235,8 +1235,8 @@ start_header: dwToGo = lpMidiStrm->dwStartTicks + lpMidiStrm->position_usec / 1000; - TRACE("%u/%u/%u\n", dwToGo, GetTickCount(), me->dwDeltaTime); - while (dwToGo - (dwCurrTC = GetTickCount()) <= MAXLONG) { + TRACE("%u/%u/%u\n", dwToGo, timeGetTime(), me->dwDeltaTime); + while (dwToGo - (dwCurrTC = timeGetTime()) <= MAXLONG) { if (MsgWaitForMultipleObjects(0, NULL, FALSE, dwToGo - dwCurrTC, QS_ALLINPUT) == WAIT_OBJECT_0) { /* got a message, handle it */ while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) { diff --git a/dlls/winmm/winmm.spec b/dlls/winmm/winmm.spec index 9c6ed2dfa8..d7c7379d77 100644 --- a/dlls/winmm/winmm.spec +++ b/dlls/winmm/winmm.spec @@ -147,7 +147,7 @@ @ stdcall timeEndPeriod(long) @ stdcall timeGetDevCaps(ptr long) @ stdcall timeGetSystemTime(ptr long) -@ stdcall timeGetTime() kernel32.GetTickCount +@ stdcall timeGetTime() @ stdcall timeKillEvent(long) @ stdcall timeSetEvent(long long ptr long long) @ stdcall waveInAddBuffer(long ptr long) -- 2.28.0
Hi, I think this patch could be split up into a patch that first replaces the GetTickCount calls and adds a timeGetTime implementation that (still) calls GetTickCount and a second patch that changes timeGetTime to use QueryPerformanceCounter. That's just an IMHO though, I am fine with including the patch as-is too as it is pretty straightforward. Cheers, Stefan
Am 18.08.2020 um 16:27 schrieb Arkadiusz Hiler <ahiler(a)codeweavers.com>:
Wine's winmm is built on the assumption that we default to 1ms resolution without ever calling timeBeginPeriod(1).
For simplicity timeGetTime() was backed by GetTickCount() which recently has changed its implementation and, in worst case scenario, returns a new value each ~16ms. This breaks the mentioned assumption.
This patch introduces a proper timeGetTime() that uses QueryPerformanceCounter(). timeGetTime() is now also used instead of GetTickCount() internally by winmm.
The "observations" comment was updated with the latest findings.
Fixes: cd215bb49bc2 ("kernel32: Use the user shared data to implement GetTickCount().") Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49564 Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com> --- dlls/winmm/time.c | 47 +++++++++++++++++++++++++++++++++---------- dlls/winmm/winmm.c | 12 +++++------ dlls/winmm/winmm.spec | 2 +- 3 files changed, 43 insertions(+), 18 deletions(-)
diff --git a/dlls/winmm/time.c b/dlls/winmm/time.c index 22c89852a2..9bb948efbd 100644 --- a/dlls/winmm/time.c +++ b/dlls/winmm/time.c @@ -74,17 +74,30 @@ static inline void link_timer( WINE_TIMERENTRY *timer )
/* * Some observations on the behavior of winmm on Windows. - * First, the call to timeBeginPeriod(xx) can never be used - * to raise the timer resolution, only lower it. + * + * First, the call to timeBeginPeriod(xx) can never be used to + * lower the timer resolution (i.e. increase the update + * interval), only to increase the timer resolution (i.e. lower + * the update interval). * * Second, a brief survey of a variety of Win 2k and Win X * machines showed that a 'standard' (aka default) timer * resolution was 1 ms (Win9x is documented as being 1). However, one * machine had a standard timer resolution of 10 ms. * - * Further, if we set our default resolution to 1, - * the implementation of timeGetTime becomes GetTickCount(), - * and we can optimize the code to reduce overhead. + * Further, timeBeginPeriod(xx) also affects the resolution of + * wait calls such as NtDelayExecution() and + * NtWaitForMultipleObjects() which by default round up their + * timeout to the nearest multiple of 15.625ms across all Windows + * versions. In Wine all of those currently work with sub-1ms + * accuracy. + * + * Effective time resolution is a global value that is the max + * of the resolutions (i.e. min of update intervals) requested by + * all the processes. A lot of programs seem to do + * timeBeginPeriod(1) forcing it onto everyone else. + * + * Defaulting to 1ms accuracy in winmm should be safe. * * Additionally, a survey of Event behaviors shows that * if we request a Periodic event every 50 ms, then Windows @@ -97,6 +110,7 @@ static inline void link_timer( WINE_TIMERENTRY *timer ) * no delays. * * Jeremy White, October 2004 + * Arkadiusz Hiler, August 2020 */ #define MMSYSTIME_MININTERVAL (1) #define MMSYSTIME_MAXINTERVAL (65535) @@ -131,7 +145,7 @@ static int TIME_MMSysTimeCallback(void) }
timer = LIST_ENTRY( ptr, WINE_TIMERENTRY, entry ); - delta_time = timer->dwTriggerTime - GetTickCount(); + delta_time = timer->dwTriggerTime - timeGetTime(); if (delta_time > 0) break;
list_remove( &timer->entry ); @@ -242,16 +256,27 @@ void TIME_MMTimeStop(void) */ MMRESULT WINAPI timeGetSystemTime(LPMMTIME lpTime, UINT wSize) { - if (wSize >= sizeof(*lpTime)) { - lpTime->wType = TIME_MS; - lpTime->u.ms = GetTickCount(); - + lpTime->wType = TIME_MS; + lpTime->u.ms = timeGetTime(); }
return 0; }
+/************************************************************************** + * timeGetTime [WINMM.@] + */ +DWORD WINAPI timeGetTime(void) +{ + LARGE_INTEGER now, freq; + + QueryPerformanceCounter(&now); + QueryPerformanceFrequency(&freq); + + return (now.QuadPart * 1000) / freq.QuadPart; +} + /************************************************************************** * timeSetEvent [WINMM.@] */ @@ -272,7 +297,7 @@ MMRESULT WINAPI timeSetEvent(UINT wDelay, UINT wResol, LPTIMECALLBACK lpFunc, return 0;
lpNewTimer->wDelay = wDelay; - lpNewTimer->dwTriggerTime = GetTickCount() + wDelay; + lpNewTimer->dwTriggerTime = timeGetTime() + wDelay;
/* FIXME - wResol is not respected, although it is not clear that we could change our precision meaningfully */ diff --git a/dlls/winmm/winmm.c b/dlls/winmm/winmm.c index 9075a78f34..eea29ea61e 100644 --- a/dlls/winmm/winmm.c +++ b/dlls/winmm/winmm.c @@ -1030,7 +1030,7 @@ static DWORD midistream_get_playing_position(WINE_MIDIStream* lpMidiStrm) case MSM_STATUS_PAUSED: return lpMidiStrm->dwElapsedMS; case MSM_STATUS_PLAYING: - return GetTickCount() - lpMidiStrm->dwStartTicks; + return timeGetTime() - lpMidiStrm->dwStartTicks; default: FIXME("Unknown playing status %hu\n", lpMidiStrm->status); return 0; @@ -1080,7 +1080,7 @@ static BOOL MMSYSTEM_MidiStream_MessageHandler(WINE_MIDIStream* lpMidiStrm, LPWI /* FIXME: send out cc64 0 (turn off sustain pedal) on every channel */ if (lpMidiStrm->status != MSM_STATUS_PLAYING) { EnterCriticalSection(&lpMidiStrm->lock); - lpMidiStrm->dwStartTicks = GetTickCount() - lpMidiStrm->dwElapsedMS; + lpMidiStrm->dwStartTicks = timeGetTime() - lpMidiStrm->dwElapsedMS; lpMidiStrm->status = MSM_STATUS_PLAYING; LeaveCriticalSection(&lpMidiStrm->lock); } @@ -1090,7 +1090,7 @@ static BOOL MMSYSTEM_MidiStream_MessageHandler(WINE_MIDIStream* lpMidiStrm, LPWI /* FIXME: send out cc64 0 (turn off sustain pedal) on every channel */ if (lpMidiStrm->status != MSM_STATUS_PAUSED) { EnterCriticalSection(&lpMidiStrm->lock); - lpMidiStrm->dwElapsedMS = GetTickCount() - lpMidiStrm->dwStartTicks; + lpMidiStrm->dwElapsedMS = timeGetTime() - lpMidiStrm->dwStartTicks; lpMidiStrm->status = MSM_STATUS_PAUSED; LeaveCriticalSection(&lpMidiStrm->lock); } @@ -1126,7 +1126,7 @@ static BOOL MMSYSTEM_MidiStream_MessageHandler(WINE_MIDIStream* lpMidiStrm, LPWI case WINE_MSM_HEADER: /* sets initial tick count for first MIDIHDR */ if (!lpMidiStrm->dwStartTicks) - lpMidiStrm->dwStartTicks = GetTickCount(); + lpMidiStrm->dwStartTicks = timeGetTime(); lpMidiHdr = (LPMIDIHDR)msg->lParam; lpData = (LPBYTE)lpMidiHdr->lpData; TRACE("Adding %s lpMidiHdr=%p [lpData=0x%p dwBytesRecorded=%u/%u dwFlags=0x%08x size=%lu]\n", @@ -1235,8 +1235,8 @@ start_header:
dwToGo = lpMidiStrm->dwStartTicks + lpMidiStrm->position_usec / 1000;
- TRACE("%u/%u/%u\n", dwToGo, GetTickCount(), me->dwDeltaTime); - while (dwToGo - (dwCurrTC = GetTickCount()) <= MAXLONG) { + TRACE("%u/%u/%u\n", dwToGo, timeGetTime(), me->dwDeltaTime); + while (dwToGo - (dwCurrTC = timeGetTime()) <= MAXLONG) { if (MsgWaitForMultipleObjects(0, NULL, FALSE, dwToGo - dwCurrTC, QS_ALLINPUT) == WAIT_OBJECT_0) { /* got a message, handle it */ while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) { diff --git a/dlls/winmm/winmm.spec b/dlls/winmm/winmm.spec index 9c6ed2dfa8..d7c7379d77 100644 --- a/dlls/winmm/winmm.spec +++ b/dlls/winmm/winmm.spec @@ -147,7 +147,7 @@ @ stdcall timeEndPeriod(long) @ stdcall timeGetDevCaps(ptr long) @ stdcall timeGetSystemTime(ptr long) -@ stdcall timeGetTime() kernel32.GetTickCount +@ stdcall timeGetTime() @ stdcall timeKillEvent(long) @ stdcall timeSetEvent(long long ptr long long) @ stdcall waveInAddBuffer(long ptr long) -- 2.28.0
Timer resolution is a global property on Windows. If one process requests high resolution (e.g. 1ms update interval) via timeBeginPeriod() all the other processes get that effective resolution. Wine defaults to 1ms, as if there is a process running that has called timeBeginPeriod(1), which is a quite common situation on Windows. The tests ensures that timeGetTime(), and some of the "wait" calls that are affected by resolution work with our default to make regressions easier to spot. Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com> --- dlls/winmm/tests/timer.c | 88 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/dlls/winmm/tests/timer.c b/dlls/winmm/tests/timer.c index be63f6b032..490bb2dbd7 100644 --- a/dlls/winmm/tests/timer.c +++ b/dlls/winmm/tests/timer.c @@ -193,6 +193,92 @@ static void test_priority(void) timeKillEvent(id); } +#define IS_1MS(call) \ +{ \ + LARGE_INTEGER _counter[101]; \ + LARGE_INTEGER _frequency; \ + int _hits = 0; \ + \ + QueryPerformanceFrequency(&_frequency); \ + QueryPerformanceCounter(&_counter[0]); \ + for (int _i = 1; _i < ARRAYSIZE(_counter); _i++) \ + { \ + (call); \ + QueryPerformanceCounter(&_counter[_i]); \ + } \ + \ + for (int _i = 1; _i < ARRAYSIZE(_counter); _i++) \ + { \ + ULONGLONG _delta = _counter[_i].QuadPart - _counter[_i-1].QuadPart; \ + _delta = _delta * 10000000 / _frequency.QuadPart; \ + if (5000 < _delta && 20000 > _delta) \ + _hits++; \ + } \ + \ + /* windows has noisy waits, 20% is good enough */ \ + ok(_hits >= ((2*ARRAYSIZE(_counter)-1)/10), \ + #call " hit the expected 0.5ms - 2ms interval only %u/%u times\n", \ + _hits, ARRAYSIZE(_counter)-1); \ +} + +static BOOL spin_the_thread; + +static DWORD WINAPI spinner_thread(void *arg) +{ + while (spin_the_thread) + Sleep(1); + + return 0; +} + +static void test_timer_resolution(void) +{ + /* ensure 1ms resolution on Windows, Wine has that by default */ + ok(timeBeginPeriod(1) == TIMERR_NOERROR, "failed to timeBeginPeriod(1)\n"); + + { + DWORD time, prev; + int count = 0, correct = 0; + + prev = timeGetTime(); + while (count < 100) { + time = timeGetTime(); + if (time != prev) { + if (time - prev == 1) correct++; + prev = time; + count++; + } + } + + ok(correct > (8*count)/10, "timeGetTime() incremented more than 1 too many (%d/%d) times\n", count - correct, count); + } + + { + DWORD thread_id; + HANDLE thread; + HANDLE event = CreateEventA(NULL, TRUE, FALSE, NULL); + + spin_the_thread = TRUE; + thread = CreateThread(NULL, 0, spinner_thread, NULL, 0, &thread_id); + + ok(thread != NULL, "failed to start a thread to wait on\n"); + ok(event != NULL, "failed to create an event\n"); + + IS_1MS(Sleep(1)); + IS_1MS(WaitForSingleObject(thread, 1)); + IS_1MS(WaitForMultipleObjects(1, &thread, TRUE, 1)); + IS_1MS({ + SignalObjectAndWait(event, thread, TRUE, 1); + ResetEvent(event); + }); + + spin_the_thread = FALSE; + ok(WaitForSingleObject(thread, 50) == WAIT_OBJECT_0, "failed to stop the spinner thread\n"); + } + + ok(timeEndPeriod(1) == TIMERR_NOERROR, "failed to timeEndPeriod(1)\n"); +} + START_TEST(timer) { test_timeGetDevCaps(); @@ -216,4 +302,6 @@ START_TEST(timer) } test_priority(); + + test_timer_resolution(); } -- 2.28.0
participants (2)
-
Arkadiusz Hiler -
Stefan Dösinger