[PATCH v3 0/9] MR6800: winegstreamer: Lock the media source critical section while using the event queue.
These changes are the documented way to implement IMFMediaEventGenerator for a media source. On top of !6783 it seems to greatly reduce the chance of hanging while switching characters in Killsquad if tracing is not enabled. -- v3: winegstreamer: Enqueue an event if a media source start/pause/stop async command fails due to shutdown. mf: Handle media source Start() failure due to source shutdown. mf: Handle media source event subscription failure due to source shutdown. mf: Handle media source BeginGetEvent() failure due to shutdown. mf: Handle media source EndGetEvent() failure due to shutdown. mf/tests: Add tests for shutting down a media source used in a session. winegstreamer: Return the result code from media_source_Pause(). rtworkq: Do not cancel pending callbacks when closing a thread pool. mfplat/tests: Add a test for calling MFUnlockWorkQueue() with pending items. https://gitlab.winehq.org/wine/wine/-/merge_requests/6800
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/mfplat/tests/mfplat.c | 43 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 752be9ea260..554d457dd0d 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -585,6 +585,7 @@ struct test_callback LONG refcount; HANDLE event; DWORD param; + UINT sleep_ms; IMFMediaEvent *media_event; IMFAsyncResult *result; }; @@ -640,6 +641,10 @@ static HRESULT WINAPI test_async_callback_result_Invoke(IMFAsyncCallback *iface, callback->result = result; IMFAsyncResult_AddRef(callback->result); + + if (callback->sleep_ms) + Sleep(callback->sleep_ms); + SetEvent(callback->event); return S_OK; @@ -6703,6 +6708,43 @@ static void test_queue_com_state(const char *name) IMFAsyncCallback_Release(&callback->IMFAsyncCallback_iface); } +static void test_queue_unlock_with_pending(void) +{ + struct test_callback *callback0, *callback1; + DWORD queue; + HRESULT hr; + + callback0 = create_test_callback(&test_queue_com_state_callback_vtbl); + callback1 = create_test_callback(&test_queue_com_state_callback_vtbl); + callback0->sleep_ms = 500; + + hr = MFStartup(MF_VERSION, MFSTARTUP_FULL); + ok(SUCCEEDED(hr), "Failed to start up, hr %#lx.\n", hr); + + hr = MFAllocateWorkQueue(&queue); + ok(SUCCEEDED(hr), "Failed to allocate queue, hr %#lx.\n", hr); + + hr = MFPutWorkItem(queue, &callback0->IMFAsyncCallback_iface, NULL); + ok(SUCCEEDED(hr), "Failed to queue work item, hr %#lx.\n", hr); + hr = MFPutWorkItem(queue, &callback1->IMFAsyncCallback_iface, NULL); + ok(SUCCEEDED(hr), "Failed to queue work item, hr %#lx.\n", hr); + + /* Unlocking does not prevent pending items from executing. */ + MFUnlockWorkQueue(queue); + + hr = WaitForSingleObject(callback0->event, 1000); + ok(hr == WAIT_OBJECT_0 || hr == WAIT_TIMEOUT, "Failed to wait for event, hr %#lx.\n", hr); + hr = WaitForSingleObject(callback1->event, 1000); + todo_wine + ok(hr == WAIT_OBJECT_0, "Failed to wait for event, hr %#lx.\n", hr); + + hr = MFShutdown(); + ok(hr == S_OK, "Failed to shut down, hr %#lx.\n", hr); + + IMFAsyncCallback_Release(&callback0->IMFAsyncCallback_iface); + IMFAsyncCallback_Release(&callback1->IMFAsyncCallback_iface); +} + static void test_MFGetStrideForBitmapInfoHeader(void) { static const struct stride_test @@ -13216,6 +13258,7 @@ START_TEST(mfplat) CoInitialize(NULL); test_startup(); + test_queue_unlock_with_pending(); test_register(); test_media_type(); test_MFCreateMediaEvent(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6800
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Windows behaviour is to process items until cleared. --- dlls/mfplat/tests/mfplat.c | 3 +-- dlls/rtworkq/queue.c | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 554d457dd0d..5abd45918a4 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -6733,9 +6733,8 @@ static void test_queue_unlock_with_pending(void) MFUnlockWorkQueue(queue); hr = WaitForSingleObject(callback0->event, 1000); - ok(hr == WAIT_OBJECT_0 || hr == WAIT_TIMEOUT, "Failed to wait for event, hr %#lx.\n", hr); + ok(hr == WAIT_OBJECT_0, "Failed to wait for event, hr %#lx.\n", hr); hr = WaitForSingleObject(callback1->event, 1000); - todo_wine ok(hr == WAIT_OBJECT_0, "Failed to wait for event, hr %#lx.\n", hr); hr = MFShutdown(); diff --git a/dlls/rtworkq/queue.c b/dlls/rtworkq/queue.c index 9046a70b359..8440d16aa98 100644 --- a/dlls/rtworkq/queue.c +++ b/dlls/rtworkq/queue.c @@ -354,7 +354,7 @@ static BOOL pool_queue_shutdown(struct queue *queue) if (!queue->pool) return FALSE; - CloseThreadpoolCleanupGroupMembers(queue->envs[0].CleanupGroup, TRUE, NULL); + CloseThreadpoolCleanupGroupMembers(queue->envs[0].CleanupGroup, FALSE, NULL); CloseThreadpool(queue->pool); queue->pool = NULL; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6800
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/winegstreamer/media_source.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 030d0c1b9a2..8dc8145b60f 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -1537,7 +1537,7 @@ static HRESULT WINAPI media_source_Pause(IMFMediaSource *iface) LeaveCriticalSection(&source->cs); - return S_OK; + return hr; } static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6800
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/mf/tests/mf.c | 164 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 164 insertions(+) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 09830191363..b1546fbcf36 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -6434,6 +6434,169 @@ static void test_media_session_Start(void) ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); } +static void test_media_session_source_shutdown(void) +{ + media_type_desc video_rgb32_desc = + { + ATTR_GUID(MF_MT_MAJOR_TYPE, MFMediaType_Video), + ATTR_GUID(MF_MT_SUBTYPE, MFVideoFormat_RGB32), + }; + struct test_grabber_callback *grabber_callback; + IMFActivate *sink_activate; + IMFAsyncCallback *callback; + IMFMediaType *output_type; + IMFMediaSession *session; + IMFMediaSource *source; + IMFTopology *topology; + PROPVARIANT propvar; + UINT64 duration; + HRESULT hr; + enum + { + TEST_START, + TEST_RESTART, + TEST_PAUSE, + TEST_STOP, + } shutdown_point; + + hr = MFStartup(MF_VERSION, MFSTARTUP_FULL); + ok(hr == S_OK, "Failed to start up, hr %#lx.\n", hr); + + /* These tests don't cover asynchronous shutdown, which is difficult to consistently test. */ + + for (shutdown_point = TEST_START; shutdown_point <= TEST_STOP; ++shutdown_point) + { + winetest_push_context("Test %d", shutdown_point); + + if (!(source = create_media_source(L"test.mp4", L"video/mp4"))) + { + todo_wine /* Gitlab CI Debian runner */ + win_skip("MP4 media source is not supported, skipping tests.\n"); + MFShutdown(); + winetest_pop_context(); + return; + } + + grabber_callback = impl_from_IMFSampleGrabberSinkCallback(create_test_grabber_callback()); + hr = MFCreateMediaType(&output_type); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + init_media_type(output_type, video_rgb32_desc, -1); + hr = MFCreateSampleGrabberSinkActivate(output_type, &grabber_callback->IMFSampleGrabberSinkCallback_iface, &sink_activate); + ok(hr == S_OK, "Failed to create grabber sink, hr %#lx.\n", hr); + IMFMediaType_Release(output_type); + + hr = MFCreateMediaSession(NULL, &session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + topology = create_test_topology(source, sink_activate, &duration); + hr = IMFMediaSession_SetTopology(session, 0, topology); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + IMFTopology_Release(topology); + + callback = create_test_callback(TRUE); + propvar.vt = VT_EMPTY; + hr = IMFMediaSession_Start(session, &GUID_NULL, &propvar); + if (shutdown_point == TEST_START) + IMFMediaSource_Shutdown(source); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event(session, callback, MESessionStarted, 5000, &propvar); + todo_wine_if(shutdown_point == TEST_START) + ok(hr == (shutdown_point == TEST_START ? MF_E_INVALIDREQUEST : S_OK), "Unexpected hr %#lx.\n", hr); + + switch(shutdown_point) + { + case TEST_RESTART: + /* Seek to 1s */ + propvar.vt = VT_I8; + propvar.hVal.QuadPart = 10000000; + hr = IMFMediaSession_Start(session, &GUID_NULL, &propvar); + IMFMediaSource_Shutdown(source); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event(session, callback, MESessionStarted, 5000, &propvar); + todo_wine + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + break; + case TEST_PAUSE: + hr = IMFMediaSession_Pause(session); + IMFMediaSource_Shutdown(source); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event(session, callback, MESessionPaused, 1000, &propvar); + ok(hr == MF_E_SHUTDOWN, "Unexpected hr %#lx.\n", hr); + break; + default: + break; + } + + if (shutdown_point == TEST_STOP) + { + hr = IMFMediaSession_Stop(session); + IMFMediaSource_Shutdown(source); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event(session, callback, MESessionStopped, 1000, &propvar); + ok(hr == MF_E_SHUTDOWN, "Unexpected hr %#lx.\n", hr); + } + + IMFMediaSource_Release(source); + + IMFActivate_ShutdownObject(sink_activate); + IMFActivate_Release(sink_activate); + IMFSampleGrabberSinkCallback_Release(&grabber_callback->IMFSampleGrabberSinkCallback_iface); + + grabber_callback = impl_from_IMFSampleGrabberSinkCallback(create_test_grabber_callback()); + hr = MFCreateMediaType(&output_type); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + init_media_type(output_type, video_rgb32_desc, -1); + hr = MFCreateSampleGrabberSinkActivate(output_type, &grabber_callback->IMFSampleGrabberSinkCallback_iface, &sink_activate); + ok(hr == S_OK, "Failed to create grabber sink, hr %#lx.\n", hr); + IMFMediaType_Release(output_type); + + source = create_media_source(L"test.mp4", L"video/mp4"); + ok(!!source, "Failed to create source.\n"); + + topology = create_test_topology(source, sink_activate, &duration); + hr = IMFMediaSession_SetTopology(session, 0, topology); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + IMFTopology_Release(topology); + + propvar.vt = VT_EMPTY; + hr = IMFMediaSession_Start(session, &GUID_NULL, &propvar); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event_until_blocking(session, callback, MESessionStarted, 5000, &propvar); + flaky_wine + ok(hr == MF_E_SHUTDOWN || hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IMFMediaSession_Stop(session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event_until_blocking(session, callback, MESessionStopped, 1000, &propvar); + ok(hr == MF_E_SHUTDOWN || hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFMediaSession_Close(session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event_until_blocking(session, callback, MESessionClosed, 1000, &propvar); + if (shutdown_point >= TEST_PAUSE) + ok(hr == MF_E_SHUTDOWN || hr == S_OK || hr == WAIT_TIMEOUT, "Unexpected hr %#lx.\n", hr); + else + ok(hr == MF_E_SHUTDOWN || hr == S_OK, "Unexpected hr %#lx.\n", hr); + + /* Media session is shut down */ + hr = IMFMediaSource_Shutdown(source); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFMediaSession_Shutdown(session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + IMFMediaSource_Release(source); + IMFAsyncCallback_Release(callback); + /* sometimes briefly leaking */ + IMFMediaSession_Release(session); + IMFActivate_ShutdownObject(sink_activate); + IMFActivate_Release(sink_activate); + IMFSampleGrabberSinkCallback_Release(&grabber_callback->IMFSampleGrabberSinkCallback_iface); + + winetest_pop_context(); + } + + hr = MFShutdown(); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); +} + static void test_MFEnumDeviceSources(void) { static const WCHAR devinterface_audio_capture_wstr[] = L"{2eef81be-33fa-4800-9670-1cd474972c3f}"; @@ -6658,4 +6821,5 @@ START_TEST(mf) test_media_session_Start(); test_MFEnumDeviceSources(); test_media_session_Close(); + test_media_session_source_shutdown(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6800
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Windows robustly handles asynchronous shutdown of a media source used in a session. --- dlls/mf/session.c | 59 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 2 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index e24cc6376b4..67af5ade4ae 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -1001,6 +1001,54 @@ static void session_flush_nodes(struct media_session *session) } } +static void session_purge_pending_commands(struct media_session *session) +{ + struct session_op *op, *op2; + + /* Purge all commands which are no longer valid after a forced source shutdown. + * Calling Stop() in this case is not required in native Windows. */ + LIST_FOR_EACH_ENTRY_SAFE(op, op2, &session->commands, struct session_op, entry) + { + if (op->command == SESSION_CMD_CLEAR_TOPOLOGIES || op->command == SESSION_CMD_SET_TOPOLOGY + || op->command == SESSION_CMD_CLOSE || op->command == SESSION_CMD_SHUTDOWN) + continue; + list_remove(&op->entry); + IUnknown_Release(&op->IUnknown_iface); + } +} + +static void session_reset(struct media_session *session) +{ + /* Media sessions in native Windows are not consistently usable after a + * forced source shutdown, but we try to clean up as well as possible. */ + session->state = SESSION_STATE_STOPPED; + session_clear_presentation(session); + session_purge_pending_commands(session); + session_command_complete(session); +} + +static void session_handle_source_shutdown(struct media_session *session) +{ + EnterCriticalSection(&session->cs); + + /* When stopping the session, MESessionStopped is sent without waiting + * for MESourceStopped, so we need do nothing in that case. */ + switch (session->state) + { + case SESSION_STATE_STARTING_SOURCES: + case SESSION_STATE_RESTARTING_SOURCES: + IMFMediaEventQueue_QueueEventParamVar(session->event_queue, MESessionStarted, &GUID_NULL, + MF_E_INVALIDREQUEST, NULL); + break; + default: + break; + } + + session_reset(session); + + LeaveCriticalSection(&session->cs); +} + static void session_start(struct media_session *session, const GUID *time_format, const PROPVARIANT *start_position) { struct media_source *source; @@ -4050,8 +4098,15 @@ static HRESULT WINAPI session_events_callback_Invoke(IMFAsyncCallback *iface, IM if (FAILED(hr = IMFMediaEventGenerator_EndGetEvent(event_source, result, &event))) { - WARN("Failed to get event from %p, hr %#lx.\n", event_source, hr); - IMFMediaEventQueue_QueueEventParamVar(session->event_queue, MEError, &GUID_NULL, hr, NULL); + if (hr == MF_E_SHUTDOWN) + { + session_handle_source_shutdown(session); + } + else + { + WARN("Failed to get event from %p, hr %#lx.\n", event_source, hr); + IMFMediaEventQueue_QueueEventParamVar(session->event_queue, MEError, &GUID_NULL, hr, NULL); + } IMFMediaEventGenerator_Release(event_source); return hr; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6800
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/mf/session.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 67af5ade4ae..076188b3e23 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -4304,8 +4304,15 @@ failed: if (FAILED(hr = IMFMediaEventGenerator_BeginGetEvent(event_source, iface, (IUnknown *)event_source))) { - WARN("Failed to re-subscribe, hr %#lx.\n", hr); - IMFMediaEventQueue_QueueEvent(session->event_queue, event); + if (hr == MF_E_SHUTDOWN) + { + session_handle_source_shutdown(session); + } + else + { + WARN("Failed to re-subscribe, hr %#lx.\n", hr); + IMFMediaEventQueue_QueueEvent(session->event_queue, event); + } } if (event) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6800
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/mf/session.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 076188b3e23..3af59fc870f 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -1049,6 +1049,16 @@ static void session_handle_source_shutdown(struct media_session *session) LeaveCriticalSection(&session->cs); } +static void session_handle_start_error(struct media_session *session, HRESULT hr) +{ + if (hr == MF_E_SHUTDOWN) + { + session_reset(session); + hr = MF_E_INVALIDREQUEST; + } + session_command_complete_with_event(session, MESessionStarted, hr, NULL); +} + static void session_start(struct media_session *session, const GUID *time_format, const PROPVARIANT *start_position) { struct media_source *source; @@ -1077,7 +1087,7 @@ static void session_start(struct media_session *session, const GUID *time_format if (FAILED(hr = session_subscribe_sources(session))) { - session_command_complete_with_event(session, MESessionStarted, hr, NULL); + session_handle_start_error(session, hr); return; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6800
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/mf/session.c | 2 +- dlls/mf/tests/mf.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 3af59fc870f..7136f1796b3 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -1096,7 +1096,7 @@ static void session_start(struct media_session *session, const GUID *time_format if (FAILED(hr = IMFMediaSource_Start(source->source, source->pd, &GUID_NULL, start_position))) { WARN("Failed to start media source %p, hr %#lx.\n", source->source, hr); - session_command_complete_with_event(session, MESessionStarted, hr, NULL); + session_handle_start_error(session, hr); return; } } diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index b1546fbcf36..1654d36d93d 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -6499,7 +6499,6 @@ static void test_media_session_source_shutdown(void) IMFMediaSource_Shutdown(source); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); hr = wait_media_event(session, callback, MESessionStarted, 5000, &propvar); - todo_wine_if(shutdown_point == TEST_START) ok(hr == (shutdown_point == TEST_START ? MF_E_INVALIDREQUEST : S_OK), "Unexpected hr %#lx.\n", hr); switch(shutdown_point) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6800
From: Conor McCarthy <cmccarthy(a)codeweavers.com> A race condition exists if the source is shut down in another thread between completion of a method and execution of its async command. Reasons for emitting the event are detailed in media_source_start(). --- dlls/winegstreamer/media_source.c | 92 ++++++++++++++++++++++++++++--- 1 file changed, 84 insertions(+), 8 deletions(-) diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 8dc8145b60f..d10850c7aa1 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -586,6 +586,7 @@ static HRESULT media_source_start(struct media_source *source, IMFPresentationDe GUID *format, PROPVARIANT *position) { BOOL starting = source->state == SOURCE_STOPPED, seek_message = !starting && position->vt != VT_EMPTY; + MediaEventType event_type = seek_message ? MESourceSeeked : MESourceStarted; IMFStreamDescriptor **descriptors; DWORD i, count; HRESULT hr; @@ -594,7 +595,19 @@ static HRESULT media_source_start(struct media_source *source, IMFPresentationDe debugstr_guid(format), wine_dbgstr_variant((VARIANT *)position)); if (source->state == SOURCE_SHUTDOWN) + { + /* We land here if the source is shut down before the async start command + * executes. The media session implementation still needs some form of + * notification in this case. The below call results in callback invocation, + * but EndGetEvent() will fail. That can happen without doing this, so we + * are not creating a new behaviour here. It's not possible for the app + * to determine if the event was enqueued before or after source shutdown, + * which implies this cannot cause issues. */ + if (FAILED(hr = IMFMediaEventQueue_QueueEventParamVar(source->event_queue, + event_type, &GUID_NULL, MF_E_SHUTDOWN, position))) + WARN("Failed to queue event for shutdown, hr %#lx\n", hr); return MF_E_SHUTDOWN; + } /* seek to beginning on stop->play */ if (source->state == SOURCE_STOPPED && position->vt == VT_EMPTY) @@ -661,7 +674,7 @@ static HRESULT media_source_start(struct media_source *source, IMFPresentationDe flush_token_queue(source->streams[i], position->vt == VT_EMPTY); return IMFMediaEventQueue_QueueEventParamVar(source->event_queue, - seek_message ? MESourceSeeked : MESourceStarted, &GUID_NULL, S_OK, position); + event_type, &GUID_NULL, S_OK, position); } static HRESULT media_source_pause(struct media_source *source) @@ -672,7 +685,12 @@ static HRESULT media_source_pause(struct media_source *source) TRACE("source %p\n", source); if (source->state == SOURCE_SHUTDOWN) + { + if (FAILED(hr = IMFMediaEventQueue_QueueEventParamVar(source->event_queue, + MESourcePaused, &GUID_NULL, MF_E_SHUTDOWN, NULL))) + WARN("Failed to queue shutdown MESourcePaused event, hr %#lx\n", hr); return MF_E_SHUTDOWN; + } for (i = 0; i < source->stream_count; i++) { @@ -694,7 +712,12 @@ static HRESULT media_source_stop(struct media_source *source) TRACE("source %p\n", source); if (source->state == SOURCE_SHUTDOWN) + { + if (FAILED(hr = IMFMediaEventQueue_QueueEventParamVar(source->event_queue, + MESourceStopped, &GUID_NULL, MF_E_SHUTDOWN, NULL))) + WARN("Failed to queue shutdown MESourceStopped event, hr %#lx\n", hr); return MF_E_SHUTDOWN; + } for (i = 0; i < source->stream_count; i++) { @@ -1362,6 +1385,9 @@ static ULONG WINAPI media_source_Release(IMFMediaSource *iface) if (!ref) { IMFMediaSource_Shutdown(iface); + /* The event queue is kept alive after shutdown for reasons described + * in media_source_start(). This is not externally visible. */ + IMFMediaEventQueue_Shutdown(source->event_queue); IMFMediaEventQueue_Release(source->event_queue); IMFByteStream_Release(source->byte_stream); wg_parser_destroy(source->wg_parser); @@ -1376,38 +1402,89 @@ static ULONG WINAPI media_source_Release(IMFMediaSource *iface) static HRESULT WINAPI media_source_GetEvent(IMFMediaSource *iface, DWORD flags, IMFMediaEvent **event) { struct media_source *source = impl_from_IMFMediaSource(iface); + IMFMediaEventQueue *event_queue = NULL; + HRESULT hr = S_OK; TRACE("%p, %#lx, %p.\n", iface, flags, event); - return IMFMediaEventQueue_GetEvent(source->event_queue, flags, event); + EnterCriticalSection(&source->cs); + + /* GetEvent() is blocking and must not share a CS with QueueEvent(). */ + if (source->state == SOURCE_SHUTDOWN) + hr = MF_E_SHUTDOWN; + else + { + event_queue = source->event_queue; + IMFMediaEventQueue_AddRef(event_queue); + } + + LeaveCriticalSection(&source->cs); + + if (SUCCEEDED(hr)) + { + hr = IMFMediaEventQueue_GetEvent(event_queue, flags, event); + IMFMediaEventQueue_Release(event_queue); + } + + return hr; } static HRESULT WINAPI media_source_BeginGetEvent(IMFMediaSource *iface, IMFAsyncCallback *callback, IUnknown *state) { struct media_source *source = impl_from_IMFMediaSource(iface); + HRESULT hr; TRACE("%p, %p, %p.\n", iface, callback, state); - return IMFMediaEventQueue_BeginGetEvent(source->event_queue, callback, state); + EnterCriticalSection(&source->cs); + + if (source->state == SOURCE_SHUTDOWN) + hr = MF_E_SHUTDOWN; + else + hr = IMFMediaEventQueue_BeginGetEvent(source->event_queue, callback, state); + + LeaveCriticalSection(&source->cs); + + return hr; } static HRESULT WINAPI media_source_EndGetEvent(IMFMediaSource *iface, IMFAsyncResult *result, IMFMediaEvent **event) { struct media_source *source = impl_from_IMFMediaSource(iface); + HRESULT hr; TRACE("%p, %p, %p.\n", iface, result, event); - return IMFMediaEventQueue_EndGetEvent(source->event_queue, result, event); + EnterCriticalSection(&source->cs); + + if (source->state == SOURCE_SHUTDOWN) + hr = MF_E_SHUTDOWN; + else + hr = IMFMediaEventQueue_EndGetEvent(source->event_queue, result, event); + + LeaveCriticalSection(&source->cs); + + return hr; } static HRESULT WINAPI media_source_QueueEvent(IMFMediaSource *iface, MediaEventType event_type, REFGUID ext_type, - HRESULT hr, const PROPVARIANT *value) + HRESULT status, const PROPVARIANT *value) { struct media_source *source = impl_from_IMFMediaSource(iface); + HRESULT hr; - TRACE("%p, %lu, %s, %#lx, %p.\n", iface, event_type, debugstr_guid(ext_type), hr, value); + TRACE("%p, %lu, %s, %#lx, %p.\n", iface, event_type, debugstr_guid(ext_type), status, value); - return IMFMediaEventQueue_QueueEventParamVar(source->event_queue, event_type, ext_type, hr, value); + EnterCriticalSection(&source->cs); + + if (source->state == SOURCE_SHUTDOWN) + hr = MF_E_SHUTDOWN; + else + hr = IMFMediaEventQueue_QueueEventParamVar(source->event_queue, event_type, ext_type, status, value); + + LeaveCriticalSection(&source->cs); + + return hr; } static HRESULT WINAPI media_source_GetCharacteristics(IMFMediaSource *iface, DWORD *characteristics) @@ -1562,7 +1639,6 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) WaitForSingleObject(source->read_thread, INFINITE); CloseHandle(source->read_thread); - IMFMediaEventQueue_Shutdown(source->event_queue); IMFByteStream_Close(source->byte_stream); while (source->stream_count--) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6800
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149979 Your paranoid android. === w7u_2qxl (32 bit report) === mf: mf.c:5383: Test failed: got hr 0x80072f8f mf.c:5383: Test failed: got hr 0x80072f8f mf.c:5454: Test failed: got hr 0x80072f8f mf.c:5460: Test failed: got hr 0x80072f8f mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf.c:6339: Test failed: WaitForSingleObject returned 258 mf.c:6339: Test failed: Unexpected hr 0xd36d8. mf: Timeout === w7u_adm (32 bit report) === mfplat: mfplat.c:1329: Test failed: Unexpected refcount 3 === w8 (32 bit report) === mfplat: mfplat.c:1329: Test failed: Unexpected refcount 3 === w1064_adm (64 bit report) === mfplat: mfplat.c:1329: Test failed: Unexpected refcount 3 === debian11 (32 bit ar:MA report) === mf: mf.c:6534: Test failed: Test 3: Unexpected hr 0. === debian11 (32 bit de report) === mf: mf.c:6522: Test failed: Test 2: Unexpected hr 0xc00d36fc. === debian11 (32 bit fr report) === mf: mf.c:6771: Test failed: Unexpected hr 0x102. === debian11 (32 bit hi:IN report) === mf: mf.c:6522: Test failed: Test 2: Unexpected hr 0xc00d36fc. === debian11b (32 bit WoW report) === Report validation errors: mfplat:mfplat crashed (c0000028)
I pushed a new version after lots of probing of native behaviour. I'm still keeping the event queue alive after shutdown to raise a final event. Wrapped media source tests in Windows show that native does not use this method. I made an async shutdown test which consistently caused hanging in Wine without the last commit here. Native didn't hang unless the wrapper was used, but no `EndGetEvent()` failures occurred. The reason for hanging with a wrapped source wasn't visible, but maybe native uses a private interface for notification and the source object included in the notification doesn't match the wrapped object the session stores. I looked into the undocumented interface mentioned above. The native session calls a method which takes a result pointer and writes a COM object pointer to that location. The second call takes a COM object pointer, and the session passes the pointer returned by the first call. If that call failed, it apparently creates an object and passes that. I was unable to determine the object type, but it wasn't `IMFAsyncCallback`, `IMFMediaEvent`, `IMFMediaStream` or `IMFAsyncResult`. I haven't used a private interface here, but will use one if it's deemed necessary. `EndGetEvent()` can fail without the hack used here, and it's just a matter of timing if the callback is invoked or not, so it's not possible for the app to prove if the callback "should" or "should not" have been invoked. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6800#note_89001
I found an issue with the async shutdown test. With that fixed it's not hanging in Wine or native. In Wine at least that probably results from using the MF work queue to shut down the source. It's not random wrt other MF commands, so I'll try creating a thread for it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6800#note_89052
participants (3)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Marvin