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

[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 <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?
>
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().