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

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