[PATCH 0/2] MR10715: mf: Call GetCorrelatedTime without critical section.
This MR addresses two issues: 1. the MR presentation clock's periodic callback continuing to be called after it was shutdown; and 2. a deadlock that could occur when calling GetCorrelatedTime in parallel with a session shutdown The deadlock occurs when SAR is the time source. A call to GetCorrelatedTime will result in an attempt to acquire SAR's critical section. This means this thread will hold the presentation clock critical section and then attempt to obtain SARs critical section. However, if SAR is in the process of shutdown in a parallel thread, then that thread will already hold the SAR critical section and call IMFPresentationClock::RemoveClockStateSink. This will attempt to acquire the presentation clock critical section. In other words, we have two threads trying to acquire the same two critical sections in the opposite order. The result is both threads waiting on the other and thus a deadlock. This commit resolves the deadlock by ensuring the presentation clock does not hold its own critical section when calling GetCorrelatedTime. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10715
From: Brendan McGrath <bmcgrath@codeweavers.com> --- dlls/mf/clock.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/mf/clock.c b/dlls/mf/clock.c index 219ef4384e2..f1aead0fcb5 100644 --- a/dlls/mf/clock.c +++ b/dlls/mf/clock.c @@ -974,6 +974,11 @@ static HRESULT WINAPI present_clock_shutdown_Shutdown(IMFShutdown *iface) EnterCriticalSection(&clock->cs); clock->is_shut_down = TRUE; + if (clock->key) + { + MFRemovePeriodicCallback(clock->key); + clock->key = 0; + } LeaveCriticalSection(&clock->cs); return S_OK; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10715
From: Brendan McGrath <bmcgrath@codeweavers.com> When SAR is the time source, the call to GetCorrelatedTime will result in an attempt to acquire SAR's critical section. This means this thread will hold the presentation clock critical section and then attempt to obtain SARs critical section. However, if SAR is in the process of shutdown in a parallel thread, then that thread will already hold the SAR critical section and call IMFPresentationClock::RemoveClockStateSink. This will attempt to acquire the presentation clock critical section. In other words, we have two threads trying to acquire the same two critical sections in the opposite order. The result is both threads waiting on the other and thus a deadlock. This commit resolves the deadlock by ensuring the presentation clock does not hold its own critical section when calling GetCorrelatedTime. --- dlls/mf/clock.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/dlls/mf/clock.c b/dlls/mf/clock.c index f1aead0fcb5..7cc0a328891 100644 --- a/dlls/mf/clock.c +++ b/dlls/mf/clock.c @@ -565,21 +565,28 @@ static struct clock_timer *presentation_clock_next_timer(struct presentation_clo static void CALLBACK presentation_clock_timer_callback(IUnknown *context) { struct presentation_clock *clock = impl_from_IMFPresentationClock((IMFPresentationClock*)context); + IMFPresentationTimeSource *time_source; struct clock_timer *timer; LONGLONG time, systime; - EnterCriticalSection(&clock->cs); - if (clock->time_source && - SUCCEEDED(IMFPresentationTimeSource_GetCorrelatedTime(clock->time_source, 0, &time, &systime))) + if ((time_source = clock->time_source)) + IMFPresentationTimeSource_AddRef(time_source); + + if (time_source && + SUCCEEDED(IMFPresentationTimeSource_GetCorrelatedTime(time_source, 0, &time, &systime))) { + EnterCriticalSection(&clock->cs); while ( (timer = presentation_clock_next_timer(clock, time)) ) { list_remove(&timer->entry); MFInvokeCallback(timer->result); IUnknown_Release(&timer->IUnknown_iface); } + LeaveCriticalSection(&clock->cs); } - LeaveCriticalSection(&clock->cs); + + if (time_source) + IMFPresentationTimeSource_Release(time_source); } static HRESULT clock_change_state(struct presentation_clock *clock, enum clock_command command, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10715
Could we instead try to make GetCorrelatedTime() in SAR lock-free? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10715#note_138240
On Wed Apr 29 22:21:17 2026 +0000, Nikolay Sivov wrote:
Could we instead try to make GetCorrelatedTime() in SAR lock-free? I think I did that in my original fix, but switched because I considered that (in theory) an application could provide its own `PresentationTimeSource` with equivalent locking to SAR's. I never tested if that would work on Windows though. Maybe it's such an unlikely scenario that we needn't consider it though?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10715#note_138284
On Wed Apr 29 22:21:17 2026 +0000, Brendan McGrath wrote:
I think I did that in my original fix, but switched because I considered that (in theory) an application could provide its own `PresentationTimeSource` with equivalent locking to SAR's. I never tested if that would work on Windows though. Maybe it's such an unlikely scenario that we needn't consider it though? If it's easy to try as a standalone test, it sounds worth it. We shouldn't rely on whatever happens in SAR.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10715#note_138285
On Wed Apr 29 22:25:21 2026 +0000, Nikolay Sivov wrote:
If it's easy to try as a standalone test, it sounds worth it. We shouldn't rely on whatever happens in SAR. I made a test app with my own `IMFMediaSink` and `IMFPresentationTimeSource`, and had it obtain a critical section in the `PresentationTimeSource::GetCorrelatedTime` and `IMFMediaSink::Shutdown` functions.
This resulted in a deadlock when ran it on Wine, but it had no issues when ran on Windows. If I apply this MR to Wine, the deadlock is fixed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10715#note_138303
participants (3)
-
Brendan McGrath -
Brendan McGrath (＠redmcg)
-
Nikolay Sivov (＠nsivov)