[PATCH 0/6] MR8415: mf: Get rid of the IMFMediaShutdownNotify interface.
As discussed in !8402. @cmccarthy what do you think of this? I think queueing an event right before shutting down the queue is enough to get the media session notified? It won't get the actual event but it will be able to see the MF_E_SHUTDOWN error after calling `EndGetEvent`? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8415
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mfplat/tests/mfplat.c | 41 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 1cdbb6f48ff..0a2471d3849 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -4654,12 +4654,26 @@ static void test_event_queue(void) hr = IMFMediaEventQueue_EndGetEvent(queue, result, &event); ok(hr == E_FAIL, "Unexpected hr %#lx.\n", hr); + /* Shutdown behavior. */ + hr = MFCreateMediaEvent(MEError, &GUID_NULL, E_FAIL, NULL, &event); + ok(hr == S_OK, "Failed to create event object, hr %#lx.\n", hr); + hr = IMFMediaEventQueue_QueueEvent(queue, event); + ok(hr == S_OK, "Failed to queue event, hr %#lx.\n", hr); + IMFMediaEvent_Release(event); + hr = IMFMediaEventQueue_Shutdown(queue); ok(hr == S_OK, "Failed to shut down, hr %#lx.\n", hr); + event = (void *)0xdeadbeef; + hr = IMFMediaEventQueue_GetEvent(queue, 0, &event); + ok(hr == MF_E_SHUTDOWN, "Unexpected hr %#lx.\n", hr); + ok(event == (void *)0xdeadbeef, "Unexpected event %p.\n", event); + event = (void *)0xdeadbeef; hr = IMFMediaEventQueue_GetEvent(queue, MF_EVENT_FLAG_NO_WAIT, &event); ok(hr == MF_E_SHUTDOWN, "Unexpected hr %#lx.\n", hr); + ok(event == (void *)0xdeadbeef, "Unexpected event %p.\n", event); + hr = MFCreateMediaEvent(MEError, &GUID_NULL, E_FAIL, NULL, &event); ok(hr == S_OK, "Failed to create event object, hr %#lx.\n", hr); @@ -4707,6 +4721,33 @@ static void test_event_queue(void) IMFAsyncCallback_Release(&callback->IMFAsyncCallback_iface); IMFAsyncCallback_Release(&callback2->IMFAsyncCallback_iface); + + callback = create_test_callback(&test_async_callback_result_vtbl); + + hr = MFCreateEventQueue(&queue); + ok(hr == S_OK, "Failed to create event queue, hr %#lx.\n", hr); + hr = IMFMediaEventQueue_BeginGetEvent(queue, &callback->IMFAsyncCallback_iface, NULL); + ok(hr == S_OK, "Failed to Begin*, hr %#lx.\n", hr); + hr = IMFMediaEventQueue_QueueEventParamVar(queue, MEError, &GUID_NULL, E_FAIL, NULL); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFMediaEventQueue_Shutdown(queue); + ok(hr == S_OK, "Failed to shut down, hr %#lx.\n", hr); + + ret = WaitForSingleObject(callback->event, 500); + ok(ret == WAIT_OBJECT_0, "Unexpected return value %#lx.\n", ret); + result = callback->result; + callback->result = NULL; + + event = (void *)0xdeadbeef; + hr = IMFMediaEventQueue_EndGetEvent(queue, result, &event); + ok(hr == MF_E_SHUTDOWN, "Unexpected hr %#lx.\n", hr); + ok(event == (void *)0xdeadbeef, "Unexpected event %p.\n", event); + IMFAsyncResult_Release(result); + + IMFMediaEventQueue_Release(queue); + IMFAsyncCallback_Release(&callback->IMFAsyncCallback_iface); + + hr = MFShutdown(); ok(hr == S_OK, "Failed to shut down, hr %#lx.\n", hr); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8415
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mfsrcsnk/media_source.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/mfsrcsnk/media_source.c b/dlls/mfsrcsnk/media_source.c index d7f20e511c8..068f64b9f03 100644 --- a/dlls/mfsrcsnk/media_source.c +++ b/dlls/mfsrcsnk/media_source.c @@ -1337,12 +1337,14 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) } source->state = SOURCE_SHUTDOWN; + IMFMediaEventQueue_QueueEventParamVar(source->queue, MEError, &GUID_NULL, MF_E_SHUTDOWN, NULL); IMFMediaEventQueue_Shutdown(source->queue); IMFByteStream_Close(source->stream); while (source->stream_count--) { struct media_stream *stream = source->streams[source->stream_count]; + IMFMediaEventQueue_QueueEventParamVar(stream->queue, MEError, &GUID_NULL, MF_E_SHUTDOWN, NULL); IMFMediaEventQueue_Shutdown(stream->queue); IMFMediaStream_Release(&stream->IMFMediaStream_iface); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8415
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/media_source.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index ab842b3e438..f6f7d6fd413 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -1649,6 +1649,7 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) WaitForSingleObject(source->read_thread, INFINITE); CloseHandle(source->read_thread); + IMFMediaEventQueue_QueueEventParamVar(source->event_queue, MEError, &GUID_NULL, MF_E_SHUTDOWN, NULL); IMFMediaEventQueue_Shutdown(source->event_queue); IMFByteStream_Close(source->byte_stream); @@ -1656,6 +1657,7 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) { struct media_stream *stream = source->streams[source->stream_count]; IMFStreamDescriptor_Release(source->descriptors[source->stream_count]); + IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEError, &GUID_NULL, MF_E_SHUTDOWN, NULL); IMFMediaEventQueue_Shutdown(stream->event_queue); IMFMediaStream_Release(&stream->IMFMediaStream_iface); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8415
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/media_source.c | 92 ------------------------------- 1 file changed, 92 deletions(-) diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index f6f7d6fd413..f85a6ff1599 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -174,15 +174,12 @@ struct media_source IMFGetService IMFGetService_iface; IMFRateSupport IMFRateSupport_iface; IMFRateControl IMFRateControl_iface; - IMFMediaShutdownNotify IMFMediaShutdownNotify_iface; IMFAsyncCallback async_commands_callback; LONG ref; DWORD async_commands_queue; IMFMediaEventQueue *event_queue; IMFByteStream *byte_stream; - IMFAsyncResult *shutdown_result; - CRITICAL_SECTION cs; UINT64 file_size; @@ -232,11 +229,6 @@ static inline struct media_source *impl_from_IMFRateControl(IMFRateControl *ifac return CONTAINING_RECORD(iface, struct media_source, IMFRateControl_iface); } -static inline struct media_source *impl_from_IMFMediaShutdownNotify(IMFMediaShutdownNotify *iface) -{ - return CONTAINING_RECORD(iface, struct media_source, IMFMediaShutdownNotify_iface); -} - static inline struct media_source *impl_from_async_commands_callback_IMFAsyncCallback(IMFAsyncCallback *iface) { return CONTAINING_RECORD(iface, struct media_source, async_commands_callback); @@ -1323,76 +1315,6 @@ static const IMFRateControlVtbl media_source_rate_control_vtbl = media_source_rate_control_GetRate, }; -static void media_source_release_shutdown_callback(struct media_source *source) -{ - if (source->shutdown_result) - IMFAsyncResult_Release(source->shutdown_result); - source->shutdown_result = NULL; -} - -static HRESULT WINAPI media_source_shutdown_notify_QueryInterface(IMFMediaShutdownNotify *iface, REFIID riid, void **obj) -{ - if (IsEqualIID(riid, &IID_IMFMediaShutdownNotify) || - IsEqualIID(riid, &IID_IUnknown)) - { - *obj = iface; - IUnknown_AddRef(iface); - return S_OK; - } - - WARN("Unsupported %s.\n", debugstr_guid(riid)); - *obj = NULL; - return E_NOINTERFACE; -} - -static ULONG WINAPI media_source_shutdown_notify_AddRef(IMFMediaShutdownNotify *iface) -{ - struct media_source *source = impl_from_IMFMediaShutdownNotify(iface); - return IMFMediaSource_AddRef(&source->IMFMediaSource_iface); -} - -static ULONG WINAPI media_source_shutdown_notify_Release(IMFMediaShutdownNotify *iface) -{ - struct media_source *source = impl_from_IMFMediaShutdownNotify(iface); - return IMFMediaSource_Release(&source->IMFMediaSource_iface); -} - -static HRESULT WINAPI media_source_shutdown_notify_set_notification_callback(IMFMediaShutdownNotify *iface, - IMFAsyncCallback *callback, IUnknown *state) -{ - struct media_source *source = impl_from_IMFMediaShutdownNotify(iface); - IMFAsyncResult *result = NULL; - HRESULT hr = S_OK; - - EnterCriticalSection(&source->cs); - - if (source->state == SOURCE_SHUTDOWN) - hr = MF_E_SHUTDOWN; - else - { - if (callback && FAILED(hr = MFCreateAsyncResult(NULL, callback, state, &result))) - { - LeaveCriticalSection(&source->cs); - return hr; - } - - media_source_release_shutdown_callback(source); - source->shutdown_result = result; - } - - LeaveCriticalSection(&source->cs); - - return hr; -} - -static const IMFMediaShutdownNotifyVtbl media_source_shutdown_notify_vtbl = -{ - media_source_shutdown_notify_QueryInterface, - media_source_shutdown_notify_AddRef, - media_source_shutdown_notify_Release, - media_source_shutdown_notify_set_notification_callback, -}; - static HRESULT WINAPI media_source_QueryInterface(IMFMediaSource *iface, REFIID riid, void **out) { struct media_source *source = impl_from_IMFMediaSource(iface); @@ -1409,10 +1331,6 @@ static HRESULT WINAPI media_source_QueryInterface(IMFMediaSource *iface, REFIID { *out = &source->IMFGetService_iface; } - else if (IsEqualIID(riid, &IID_IMFMediaShutdownNotify)) - { - *out = &source->IMFMediaShutdownNotify_iface; - } else { FIXME("%s, %p.\n", debugstr_guid(riid), out); @@ -1443,7 +1361,6 @@ static ULONG WINAPI media_source_Release(IMFMediaSource *iface) if (!ref) { - media_source_release_shutdown_callback(source); IMFMediaSource_Shutdown(iface); IMFMediaEventQueue_Release(source->event_queue); IMFByteStream_Release(source->byte_stream); @@ -1629,7 +1546,6 @@ static HRESULT WINAPI media_source_Pause(IMFMediaSource *iface) static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) { struct media_source *source = impl_from_IMFMediaSource(iface); - HRESULT hr; TRACE("%p.\n", iface); @@ -1664,13 +1580,6 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) free(source->descriptors); free(source->streams); - if (source->shutdown_result) - { - if (FAILED(hr = MFPutWorkItemEx(MFASYNC_CALLBACK_QUEUE_STANDARD, source->shutdown_result))) - WARN("Failed to put shutdown notification, hr %#lx.\n", hr); - media_source_release_shutdown_callback(source); - } - LeaveCriticalSection(&source->cs); MFUnlockWorkQueue(source->async_commands_queue); @@ -1729,7 +1638,6 @@ static HRESULT media_source_create(struct object_context *context, IMFMediaSourc object->IMFGetService_iface.lpVtbl = &media_source_get_service_vtbl; object->IMFRateSupport_iface.lpVtbl = &media_source_rate_support_vtbl; object->IMFRateControl_iface.lpVtbl = &media_source_rate_control_vtbl; - object->IMFMediaShutdownNotify_iface.lpVtbl = &media_source_shutdown_notify_vtbl; object->async_commands_callback.lpVtbl = &source_async_commands_callback_vtbl; object->ref = 1; object->byte_stream = context->stream; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8415
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mf/session.c | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 47760cc44a7..f85390382ea 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -129,7 +129,6 @@ struct media_source IMFMediaSource *source; IUnknown *object; }; - IMFMediaShutdownNotify *shutdown_notify; IMFPresentationDescriptor *pd; enum object_state state; unsigned int flags; @@ -1604,11 +1603,6 @@ static struct media_source *session_get_media_source(struct media_session *sessi static void session_release_media_source(struct media_source *source) { - if (source->shutdown_notify) - { - IMFMediaShutdownNotify_set_notification_callback(source->shutdown_notify, NULL, NULL); - IMFMediaShutdownNotify_Release(source->shutdown_notify); - } if (source->source) IMFMediaSource_Release(source->source); if (source->pd) @@ -1630,21 +1624,6 @@ static HRESULT session_add_media_source(struct media_session *session, IMFTopolo media_source->source = source; IMFMediaSource_AddRef(media_source->source); - if (SUCCEEDED(hr = IMFMediaSource_QueryInterface(media_source->source, - &IID_IMFMediaShutdownNotify, (void **)&media_source->shutdown_notify))) - { - hr = IMFMediaShutdownNotify_set_notification_callback(media_source->shutdown_notify, - &session->shutdown_callback, (IUnknown *)media_source->source); - if (hr == MF_E_SHUTDOWN) - { - session_release_media_source(media_source); - return hr; - } - } - - if (FAILED(hr)) - TRACE("Session is not robust to unexpected shutdown, hr %#lx.\n", hr); - hr = IMFTopologyNode_GetUnknown(node, &MF_TOPONODE_PRESENTATION_DESCRIPTOR, &IID_IMFPresentationDescriptor, (void **)&media_source->pd); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8415
From: Rémi Bernon <rbernon(a)codeweavers.com> --- include/wine/mfinternal.idl | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/include/wine/mfinternal.idl b/include/wine/mfinternal.idl index 0b77eaefab1..f1c1f8c9090 100644 --- a/include/wine/mfinternal.idl +++ b/include/wine/mfinternal.idl @@ -45,19 +45,6 @@ interface IMFSinkClassFactory : IUnknown ); } -[ - uuid(0aab315c-33a4-43db-af6b-c9c86921034e), - object, - local -] -interface IMFMediaShutdownNotify : IUnknown -{ - HRESULT set_notification_callback( - [in] IMFAsyncCallback *callback, - [in] IUnknown *state - ); -} - cpp_quote("DEFINE_GUID(CLSID_MF3GPSinkClassFactory, 0xe54cdfaf, 0x2381, 0x4cad, 0xab, 0x99, 0xf3, 0x85, 0x17, 0x12, 0x7d, 0x5c);") cpp_quote("DEFINE_GUID(CLSID_MFAC3SinkClassFactory, 0x255a6fda, 0x6f93, 0x4e8a, 0x96, 0x11, 0xde, 0xd1, 0x16, 0x9e, 0xef, 0xb4);") cpp_quote("DEFINE_GUID(CLSID_MFADTSSinkClassFactory, 0xd7ca55ab, 0x5022, 0x4db3, 0xa5, 0x99, 0xab, 0xaf, 0xa3, 0x58, 0xe6, 0xf3);") -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8415
Making sure I understand, the idea is that queuing any event should invoke user callback? Doesn't that create another potential race when callback might receive this MEError event successfully before queue was shut down? I know that's unlikely but the point is that QueueEvent + Shutdown is not "atomic". I'm not opposed to such notification interface in general, but we need to figure out this undocumented API first, if it exists at all. What is the core issue with session not noticing shut down sources? Is that when user calls source's Shutdown() during playback? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8415#note_107774
Making sure I understand, the idea is that queuing any event should invoke user callback? Doesn't that create another potential race when callback might receive this MEError event successfully before queue was shut down? I know that's unlikely but the point is that QueueEvent + Shutdown is not "atomic".
Yes, indeed, and I missed that possibility, but I think it's then just a matter of handling such event like a source shutdown.
What is the core issue with session not noticing shut down sources? Is that when user calls source's Shutdown() during playback?
Yes. And sometimes the game calling Shutdown very close to when the playback ends, so possibly after the playback has ended and no more events are generated (so I don't think we can rely on something like "events are generated often enough during playback for the session to eventually notice the shutdown"). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8415#note_107779
participants (2)
-
Nikolay Sivov (＠nsivov) -
Rémi Bernon