[PATCH v2] winmm: Default to 1ms resolution like we used to.
Arkadiusz Hiler
ahiler at codeweavers.com
Mon Aug 17 04:04:17 CDT 2020
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 <ahiler at codeweavers.com>
> > ---
> >
> > 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.c#l167
> 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?
--
Cheers,
Arek
More information about the wine-devel
mailing list