[PATCH 1/4] quartz: Factor out more of AdviseTime() and AdvisePeriodic().
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/quartz/systemclock.c | 74 ++++++++++++++------------------------- 1 file changed, 27 insertions(+), 47 deletions(-) diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c index 24d8651aaba..cbe5f395258 100644 --- a/dlls/quartz/systemclock.c +++ b/dlls/quartz/systemclock.c @@ -169,8 +169,29 @@ static DWORD WINAPI SystemClockAdviseThread(void *param) } } -static void notify_thread(struct system_clock *clock) +static HRESULT add_sink(struct system_clock *clock, DWORD_PTR handle, + REFERENCE_TIME due_time, REFERENCE_TIME period, DWORD_PTR *cookie) { + struct advise_sink *sink; + + if (!handle) + return E_INVALIDARG; + + if (!cookie) + return E_POINTER; + + if (!(sink = heap_alloc_zero(sizeof(*sink)))) + return E_OUTOFMEMORY; + + sink->handle = (HANDLE)handle; + sink->due_time = due_time; + sink->period = period; + sink->cookie = InterlockedIncrement(&cookie_counter); + + EnterCriticalSection(&clock->cs); + list_add_tail(&clock->sinks, &sink->entry); + LeaveCriticalSection(&clock->cs); + if (!InterlockedCompareExchange(&clock->thread_created, TRUE, FALSE)) { clock->notify_event = CreateEventW(NULL, FALSE, FALSE, NULL); @@ -178,6 +199,9 @@ static void notify_thread(struct system_clock *clock) clock->thread = CreateThread(NULL, 0, SystemClockAdviseThread, clock, 0, NULL); } SetEvent(clock->notify_event); + + *cookie = sink->cookie; + return S_OK; } static HRESULT WINAPI SystemClockImpl_QueryInterface(IReferenceClock *iface, REFIID iid, void **out) @@ -225,72 +249,28 @@ static HRESULT WINAPI SystemClockImpl_AdviseTime(IReferenceClock *iface, REFERENCE_TIME base, REFERENCE_TIME offset, HEVENT event, DWORD_PTR *cookie) { struct system_clock *clock = impl_from_IReferenceClock(iface); - struct advise_sink *sink; TRACE("clock %p, base %s, offset %s, event %#lx, cookie %p.\n", clock, debugstr_time(base), debugstr_time(offset), event, cookie); - if (!event) - return E_INVALIDARG; - if (base + offset <= 0) return E_INVALIDARG; - if (!cookie) - return E_POINTER; - - if (!(sink = heap_alloc_zero(sizeof(*sink)))) - return E_OUTOFMEMORY; - - sink->handle = (HANDLE)event; - sink->due_time = base + offset; - sink->period = 0; - sink->cookie = InterlockedIncrement(&cookie_counter); - - EnterCriticalSection(&clock->cs); - list_add_tail(&clock->sinks, &sink->entry); - LeaveCriticalSection(&clock->cs); - - notify_thread(clock); - - *cookie = sink->cookie; - return S_OK; + return add_sink(clock, event, base + offset, 0, cookie); } static HRESULT WINAPI SystemClockImpl_AdvisePeriodic(IReferenceClock* iface, REFERENCE_TIME start, REFERENCE_TIME period, HSEMAPHORE semaphore, DWORD_PTR *cookie) { struct system_clock *clock = impl_from_IReferenceClock(iface); - struct advise_sink *sink; TRACE("clock %p, start %s, period %s, semaphore %#lx, cookie %p.\n", clock, debugstr_time(start), debugstr_time(period), semaphore, cookie); - if (!semaphore) - return E_INVALIDARG; - if (start <= 0 || period <= 0) return E_INVALIDARG; - if (!cookie) - return E_POINTER; - - if (!(sink = heap_alloc_zero(sizeof(*sink)))) - return E_OUTOFMEMORY; - - sink->handle = (HANDLE)semaphore; - sink->due_time = start; - sink->period = period; - sink->cookie = InterlockedIncrement(&cookie_counter); - - EnterCriticalSection(&clock->cs); - list_add_tail(&clock->sinks, &sink->entry); - LeaveCriticalSection(&clock->cs); - - notify_thread(clock); - - *cookie = sink->cookie; - return S_OK; + return add_sink(clock, semaphore, start, period, cookie); } static HRESULT WINAPI SystemClockImpl_Unadvise(IReferenceClock *iface, DWORD_PTR cookie) -- 2.32.0
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/quartz/systemclock.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c index cbe5f395258..597373a4112 100644 --- a/dlls/quartz/systemclock.c +++ b/dlls/quartz/systemclock.c @@ -42,10 +42,11 @@ struct system_clock IUnknown *outer_unk; LONG refcount; - BOOL thread_created; - HANDLE thread, notify_event, stop_event; + BOOL thread_created, thread_stopped; + HANDLE thread; REFERENCE_TIME last_time; CRITICAL_SECTION cs; + CONDITION_VARIABLE cv; struct list sinks; }; @@ -96,11 +97,12 @@ static ULONG WINAPI system_clock_inner_Release(IUnknown *iface) { if (clock->thread) { - SetEvent(clock->stop_event); + EnterCriticalSection(&clock->cs); + clock->thread_stopped = TRUE; + LeaveCriticalSection(&clock->cs); + WakeConditionVariable(&clock->cv); WaitForSingleObject(clock->thread, INFINITE); CloseHandle(clock->thread); - CloseHandle(clock->notify_event); - CloseHandle(clock->stop_event); } clock->cs.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&clock->cs); @@ -128,7 +130,6 @@ static DWORD WINAPI SystemClockAdviseThread(void *param) struct system_clock *clock = param; struct advise_sink *sink, *cursor; REFERENCE_TIME current_time; - HANDLE handles[2] = {clock->stop_event, clock->notify_event}; TRACE("Starting advise thread for clock %p.\n", clock); @@ -162,10 +163,13 @@ static DWORD WINAPI SystemClockAdviseThread(void *param) timeout = min(timeout, (sink->due_time - current_time) / 10000); } - LeaveCriticalSection(&clock->cs); - - if (WaitForMultipleObjects(2, handles, FALSE, timeout) == 0) + SleepConditionVariableCS(&clock->cv, &clock->cs, timeout); + if (clock->thread_stopped) + { + LeaveCriticalSection(&clock->cs); return 0; + } + LeaveCriticalSection(&clock->cs); } } @@ -194,11 +198,9 @@ static HRESULT add_sink(struct system_clock *clock, DWORD_PTR handle, if (!InterlockedCompareExchange(&clock->thread_created, TRUE, FALSE)) { - clock->notify_event = CreateEventW(NULL, FALSE, FALSE, NULL); - clock->stop_event = CreateEventW(NULL, TRUE, FALSE, NULL); clock->thread = CreateThread(NULL, 0, SystemClockAdviseThread, clock, 0, NULL); } - SetEvent(clock->notify_event); + WakeConditionVariable(&clock->cv); *cookie = sink->cookie; return S_OK; -- 2.32.0
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/quartz/tests/systemclock.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/dlls/quartz/tests/systemclock.c b/dlls/quartz/tests/systemclock.c index f13e89abedb..fa9464429f0 100644 --- a/dlls/quartz/tests/systemclock.c +++ b/dlls/quartz/tests/systemclock.c @@ -24,12 +24,6 @@ static ULONGLONG (WINAPI *pGetTickCount64)(void); -static BOOL compare_time(REFERENCE_TIME x, REFERENCE_TIME y, unsigned int max_diff) -{ - REFERENCE_TIME diff = x > y ? x - y : y - x; - return diff <= max_diff; -} - static IReferenceClock *create_system_clock(void) { IReferenceClock *clock = NULL; @@ -172,7 +166,7 @@ static void test_aggregation(void) static void test_get_time(void) { IReferenceClock *clock = create_system_clock(); - REFERENCE_TIME time1, time2; + REFERENCE_TIME time1, ticks, time2; HRESULT hr; ULONG ref; @@ -180,18 +174,21 @@ static void test_get_time(void) ok(hr == E_POINTER, "Got hr %#x.\n", hr); hr = IReferenceClock_GetTime(clock, &time1); - if (pGetTickCount64) - time2 = pGetTickCount64() * 10000; - else - time2 = (REFERENCE_TIME)GetTickCount() * 10000; ok(hr == S_OK, "Got hr %#x.\n", hr); ok(time1 % 10000 == 0, "Expected no less than 1ms coarseness, but got time %s.\n", wine_dbgstr_longlong(time1)); - ok(compare_time(time1, time2, 20 * 10000), "Expected about %s, got %s.\n", - wine_dbgstr_longlong(time2), wine_dbgstr_longlong(time1)); + + if (pGetTickCount64) + ticks = pGetTickCount64() * 10000; + else + ticks = (REFERENCE_TIME)GetTickCount() * 10000; hr = IReferenceClock_GetTime(clock, &time2); ok(hr == (time2 == time1 ? S_FALSE : S_OK), "Got hr %#x.\n", hr); + ok(time2 % 10000 == 0, "Expected no less than 1ms coarseness, but got time %s.\n", + wine_dbgstr_longlong(time1)); + + ok(time2 >= ticks && ticks >= time1, "Got timestamps %I64d, %I64d, %I64d.\n", time1, ticks, time2); Sleep(100); hr = IReferenceClock_GetTime(clock, &time2); -- 2.32.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=94259 Your paranoid android. === w8 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 786340000, 786400000, 786340000. === w8adm (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 579450000, 579530000, 579450000. === w864 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 34279625900000, 377877009530000, 34279625900000. === w1064v1507 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 507560000, 507650000, 507560000. === w1064v1809 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 660270000, 660310000, 660270000. === w1064 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 601610000, 601560000, 601610000. === w1064_tsign (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 882550000, 882500000, 882550000. === w10pro64 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 706740000, 706710000, 706740000. === w2008s64 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 796420000, 796400000, 796420000. === w864 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 34279621680000, 377877005310000, 34279621680000. === w1064v1507 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 499270000, 499370000, 499270000. === w1064v1809 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 660550000, 660620000, 660550000. === w1064 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 589700000, 589680000, 589700000. === w1064_2qxl (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 696720000, 696710000, 696720000. === w1064_tsign (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 885080000, 885000000, 885080000. === w10pro64 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 705210000, 705310000, 705210000. === w10pro64_ar (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 2709150000, 2709060000, 2709150000. === w10pro64_he (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 593630000, 593590000, 593630000. === w10pro64_ja (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 593560000, 593590000, 593560000. === w10pro64_zh_CN (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 571660000, 571560000, 571660000.
On Mon, 19 Jul 2021, Marvin wrote: [...]
=== w1064v1809 (32 bit report) ===
quartz: systemclock.c:191: Test failed: Got timestamps 660270000, 660310000, 660270000.
=== w1064 (32 bit report) ===
quartz: systemclock.c:191: Test failed: Got timestamps 601610000, 601560000, 601610000.
It looks like this does fail on about every Windows version, VM or not: https://test.winehq.org/data/patterns.html#quartz:systemclock I guess you're already looking into it? -- Francois Gouget <fgouget(a)codeweavers.com>
On 7/22/21 1:59 PM, Francois Gouget wrote:
On Mon, 19 Jul 2021, Marvin wrote: [...]
=== w1064v1809 (32 bit report) ===
quartz: systemclock.c:191: Test failed: Got timestamps 660270000, 660310000, 660270000.
=== w1064 (32 bit report) ===
quartz: systemclock.c:191: Test failed: Got timestamps 601610000, 601560000, 601610000.
It looks like this does fail on about every Windows version, VM or not:
https://test.winehq.org/data/patterns.html#quartz:systemclock
I guess you're already looking into it?
Yep, it's already on my list.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/quartz/systemclock.c | 8 ++++++++ dlls/quartz/tests/systemclock.c | 12 ++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c index 597373a4112..73a923a7b99 100644 --- a/dlls/quartz/systemclock.c +++ b/dlls/quartz/systemclock.c @@ -90,6 +90,7 @@ static ULONG WINAPI system_clock_inner_Release(IUnknown *iface) { struct system_clock *clock = impl_from_IUnknown(iface); ULONG refcount = InterlockedDecrement(&clock->refcount); + struct advise_sink *sink, *cursor; TRACE("%p decreasing refcount to %u.\n", clock, refcount); @@ -104,6 +105,13 @@ static ULONG WINAPI system_clock_inner_Release(IUnknown *iface) WaitForSingleObject(clock->thread, INFINITE); CloseHandle(clock->thread); } + + LIST_FOR_EACH_ENTRY_SAFE(sink, cursor, &clock->sinks, struct advise_sink, entry) + { + list_remove(&sink->entry); + heap_free(sink); + } + clock->cs.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&clock->cs); heap_free(clock); diff --git a/dlls/quartz/tests/systemclock.c b/dlls/quartz/tests/systemclock.c index fa9464429f0..be5472ed6e1 100644 --- a/dlls/quartz/tests/systemclock.c +++ b/dlls/quartz/tests/systemclock.c @@ -272,11 +272,19 @@ static void test_advise(void) ok(hr == S_OK, "Got hr %#x.\n", hr); ok(WaitForSingleObject(semaphore, 200) == WAIT_TIMEOUT, "Semaphore should not be signaled.\n"); - CloseHandle(event); - CloseHandle(semaphore); + ResetEvent(event); + hr = IReferenceClock_GetTime(clock, ¤t); + ok(SUCCEEDED(hr), "Got hr %#x.\n", hr); + hr = IReferenceClock_AdviseTime(clock, current, 500 * 10000, (HEVENT)event, &cookie); + ok(hr == S_OK, "Got hr %#x.\n", hr); ref = IReferenceClock_Release(clock); ok(!ref, "Got outstanding refcount %d.\n", ref); + + ok(WaitForSingleObject(event, 0) == WAIT_TIMEOUT, "Event should not be signaled.\n"); + + CloseHandle(event); + CloseHandle(semaphore); } START_TEST(systemclock) -- 2.32.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=94260 Your paranoid android. === w2008s64 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 798440000, 798430000, 798440000. === w7u_adm (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 653790000, 653800000, 653790000. === w8 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 780350000, 780310000, 780350000. === w8adm (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 570120000, 570150000, 570120000. === w864 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 34279626520000, 377877010150000, 34279626520000. === w1064v1507 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 504310000, 504370000, 504310000. === w1064v1809 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 658580000, 658590000, 658580000. === w1064_tsign (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 883390000, 883280000, 883390000. === w10pro64 (32 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 706480000, 706560000, 706480000. === wvistau64 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 2354820000, 2354830000, 2354820000. === w2008s64 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 813740000, 813750000, 813740000. === w864 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 34279621070000, 377877004680000, 34279621070000. === w1064v1507 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 497640000, 497650000, 497640000. === w1064v1809 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 659310000, 659210000, 659310000. === w1064 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 588670000, 588590000, 588670000. === w1064_2qxl (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 692870000, 692960000, 692870000. === w1064_tsign (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 877680000, 877650000, 877680000. === w10pro64 (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 703150000, 703120000, 703150000. === w10pro64_ar (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 652760000, 652650000, 652760000. === w10pro64_he (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 597950000, 597960000, 597950000. === w10pro64_ja (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 592350000, 592340000, 592350000. === w10pro64_zh_CN (64 bit report) === quartz: systemclock.c:191: Test failed: Got timestamps 572390000, 572340000, 572390000.
participants (4)
-
Francois Gouget -
Marvin -
Zebediah Figura -
Zebediah Figura (she/her)