[PATCH v2] winmm: Default to 1ms resolution like we used to.

[Arkadiusz Hiler]

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 
---

Changes in v2:

Don't assume QueryPerformanceCounter() frequency - internal implementation
may change (suggested by KittyCat)

Also I am more leaning towards not adding extra TRACEs here - high accuracy
timers tend to be called A LOT.

Minor wording changes.

dlls/winmm/time.c     | 44 +++++++++++++++++++++++++++++++++----------
 dlls/winmm/winmm.c    | 12 ++++++------
 dlls/winmm/winmm.spec |  2 +-
 3 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/dlls/winmm/time.c b/dlls/winmm/time.c
index 22c89852a2..46f7484315 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,26 @@ 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;
+    if (!QueryPerformanceCounter(&now) || !QueryPerformanceFrequency(&freq))
+        WARN("QueryPerformanceCounter and QueryPerformanceFrequency should not fail!\n");
+    return (now.QuadPart * 1000) / freq.QuadPart;
+}
+
 /**************************************************************************
  * 				timeSetEvent		[WINMM.@]
  */
@@ -272,7 +296,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)

[Rémi Bernon]

On 2020-08-16 23:25, Arkadiusz Hiler wrote:
> 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
>
> Changes in v2:
>
> Don't assume QueryPerformanceCounter() frequency - internal implementation
> may change (suggested by KittyCat)
>
> Also I am more leaning towards not adding extra TRACEs here - high accuracy
> timers tend to be called A LOT.
>
> Minor wording changes.
>
> dlls/winmm/time.c     | 44 +++++++++++++++++++++++++++++++++----------
>   dlls/winmm/winmm.c    | 12 ++++++------
>   dlls/winmm/winmm.spec |  2 +-
>   3 files changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/dlls/winmm/time.c b/dlls/winmm/time.c
> index 22c89852a2..46f7484315 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,26 @@ 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;
> - if (!QueryPerformanceCounter(&now) || !QueryPerformanceFrequency(&freq))
> -
>    WARN("QueryPerformanceCounter and QueryPerformanceFrequency should not fail!\n");
> - return (now.QuadPart * 1000) / freq.QuadPart;
>
> +}
> -

If it's really not supposed to happen I think it should go to ERR 
instead, WARN is not enabled by default. Also maybe not use the values 
if that failed?
> /**************************************************************************
> -
> 		timeSetEvent		[WINMM.@]
>
> */
> @@ -272,7 +296,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)

[Arkadiusz Hiler]

On Mon, Aug 17, 2020 at 10:16:42AM +0200, Rémi Bernon wrote:
> On 2020-08-16 23:25, Arkadiusz Hiler wrote:
> > 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
> > @@ -242,16 +256,26 @@ void	TIME_MMTimeStop(void)
> > 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
> >
> > Changes in v2:
> >
> > Don't assume QueryPerformanceCounter() frequency - internal implementation
> > may change (suggested by KittyCat)
> >
> > Also I am more leaning towards not adding extra TRACEs here - high accuracy
> > timers tend to be called A LOT.
> >
> > Minor wording changes.
> >
> > dlls/winmm/time.c     | 44 +++++++++++++++++++++++++++++++++----------
> >   dlls/winmm/winmm.c    | 12 ++++++------
> >   dlls/winmm/winmm.spec |  2 +-
> >   3 files changed, 41 insertions(+), 17 deletions(-)
> >
> > diff --git a/dlls/winmm/time.c b/dlls/winmm/time.c
> > index 22c89852a2..46f7484315 100644
> > --- a/dlls/winmm/time.c
> > +++ b/dlls/winmm/time.c

<SNIP>
> > @@ -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 );
> > */
> > 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;
> > - if (!QueryPerformanceCounter(&now) || !QueryPerformanceFrequency(&freq))
> > -
> >    WARN("QueryPerformanceCounter and QueryPerformanceFrequency should not fail!\n");
> > - return (now.QuadPart * 1000) / freq.QuadPart;
> >
> > +}
> > -
>
> If it's really not supposed to happen I think it should go to ERR instead,
> WARN is not enabled by default.

I was just getting inspiration from the existing code[0], but indeed ERR
sound better here.

[0]: https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/krnl386.exe16/ioports...
> Also maybe not use the values if that failed?

It is *really* not supposed to happen. A lot of non-test code is calling
[Nt]QueryPerformanceCounter() without ever checking the returned values.

Sure, we can return 0 here, but if QueryPerformanceCounter() and
*Frequency() wil start failing we are screwed in 100s of ways anyway.

I would say that failing QPC() is a prime candidate for a hard abort,
but on the other hand a lot of conformance tests are using QPC and
ok()-ing on the returned value, so we would notice a regression. I am
also unsure if we have any unwritten rules on the topic.

Personally I would just:

if (!QPC()) {
    ERR("QPC should not fail!\n");
    assert(FALSE);
}

Any suggestions?

[Paul Gofman]

On 8/17/20 12:04, Arkadiusz Hiler wrote:
> On Mon, Aug 17, 2020 at 10:16:42AM +0200, Rémi Bernon wrote:
> > On 2020-08-16 23:25, Arkadiusz Hiler wrote:
> > > 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
> > > @@ -242,16 +256,26 @@ void	TIME_MMTimeStop(void)
> > > 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
> > >
> > > Changes in v2:
> > >
> > > Don't assume QueryPerformanceCounter() frequency - internal implementation
> > > may change (suggested by KittyCat)
> > >
> > > Also I am more leaning towards not adding extra TRACEs here - high accuracy
> > > timers tend to be called A LOT.
> > >
> > > Minor wording changes.
> > >
> > > dlls/winmm/time.c     | 44 +++++++++++++++++++++++++++++++++----------
> > >    dlls/winmm/winmm.c    | 12 ++++++------
> > >    dlls/winmm/winmm.spec |  2 +-
> > >    3 files changed, 41 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/dlls/winmm/time.c b/dlls/winmm/time.c
> > > index 22c89852a2..46f7484315 100644
> > > --- a/dlls/winmm/time.c
> > > +++ b/dlls/winmm/time.c
>
> <SNIP>
> >> @@ -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 );
> >>     */
> >>    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;
> >> +    if (!QueryPerformanceCounter(&now) || !QueryPerformanceFrequency(&freq))
> >> +        WARN("QueryPerformanceCounter and QueryPerformanceFrequency should not fail!\n");
> >> +    return (now.QuadPart * 1000) / freq.QuadPart;
> >> +}
> >> +
> > If it's really not supposed to happen I think it should go to ERR instead,
> > WARN is not enabled by default.
> I was just getting inspiration from the existing code[0], but indeed ERR
> sound better here.
> > Also maybe not use the values if that failed?
>
> It is *really* not supposed to happen. A lot of non-test code is calling
> [Nt]QueryPerformanceCounter() without ever checking the returned values.
>
> Sure, we can return 0 here, but if QueryPerformanceCounter() and
> *Frequency() wil start failing we are screwed in 100s of ways anyway.
>
> I would say that failing QPC() is a prime candidate for a hard abort,
> but on the other hand a lot of conformance tests are using QPC and
> ok()-ing on the returned value, so we would notice a regression. I am
> also unsure if we have any unwritten rules on the topic.
>
> Personally I would just:
>
> if (!QPC()) {
>      ERR("QPC should not fail!\n");
>      assert(FALSE);
> }
>
> Any suggestions?

I would guess not checking the return values at all (like it is done 
elsewhere) is a viable option in this particular case, these functions 
are really not supposed to fail. In some other cases when something is 
really not supposed to fail but we want the verification check anyway we 
also used to use assert().