[PATCH v6 0/4] MR607: mf: Fix session command handling and SESSION_CMD_END.
-- v6: mf: Notify quality manager of topology change in session_set_topology. mf: Use the SESSION_FLAG_PENDING_COMMAND to delay further commands. mf: Delay media session command processing when presentation is ending. mf: Use a dedicated interface for sample allocator ready callbacks. https://gitlab.winehq.org/wine/wine/-/merge_requests/607
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/mf/session.c | 102 +++++++++++++++++++++++++++++++++------------- 1 file changed, 73 insertions(+), 29 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 87fe77a730f..2553369fb54 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -50,7 +50,6 @@ enum session_command /* Internally used commands. */ SESSION_CMD_END, SESSION_CMD_QM_NOTIFY_TOPOLOGY, - SESSION_CMD_SA_READY, }; struct session_op @@ -79,10 +78,6 @@ struct session_op { IMFTopology *topology; } notify_topology; - struct - { - TOPOID node_id; - } sa_ready; }; struct list entry; }; @@ -231,6 +226,7 @@ struct media_session IMFRateControl IMFRateControl_iface; IMFTopologyNodeAttributeEditor IMFTopologyNodeAttributeEditor_iface; IMFAsyncCallback commands_callback; + IMFAsyncCallback sa_ready_callback; IMFAsyncCallback events_callback; IMFAsyncCallback sink_finalizer_callback; LONG refcount; @@ -274,6 +270,11 @@ static struct media_session *impl_from_commands_callback_IMFAsyncCallback(IMFAsy return CONTAINING_RECORD(iface, struct media_session, commands_callback); } +static struct media_session *impl_from_sa_ready_callback_IMFAsyncCallback(IMFAsyncCallback *iface) +{ + return CONTAINING_RECORD(iface, struct media_session, sa_ready_callback); +} + static struct media_session *impl_from_events_callback_IMFAsyncCallback(IMFAsyncCallback *iface) { return CONTAINING_RECORD(iface, struct media_session, events_callback); @@ -1537,15 +1538,7 @@ static HRESULT session_request_sample_from_node(struct media_session *session, I static HRESULT WINAPI node_sample_allocator_cb_NotifyRelease(IMFVideoSampleAllocatorNotify *iface) { struct topo_node *topo_node = impl_node_from_IMFVideoSampleAllocatorNotify(iface); - struct session_op *op; - - if (SUCCEEDED(create_session_op(SESSION_CMD_SA_READY, &op))) - { - op->sa_ready.node_id = topo_node->node_id; - MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &topo_node->session->commands_callback, &op->IUnknown_iface); - IUnknown_Release(&op->IUnknown_iface); - } - + MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &topo_node->session->sa_ready_callback, (IUnknown *)iface); return S_OK; } @@ -2378,9 +2371,6 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, { struct session_op *op = impl_op_from_IUnknown(IMFAsyncResult_GetStateNoAddRef(result)); struct media_session *session = impl_from_commands_callback_IMFAsyncCallback(iface); - struct topo_node *topo_node; - IMFTopologyNode *upstream_node; - DWORD upstream_output; EnterCriticalSection(&session->cs); @@ -2409,18 +2399,6 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, IMFQualityManager_NotifyTopology(session->quality_manager, op->notify_topology.topology); session_command_complete(session); break; - case SESSION_CMD_SA_READY: - topo_node = session_get_node_by_id(session, op->sa_ready.node_id); - - if (topo_node->u.sink.requests) - { - if (SUCCEEDED(IMFTopologyNode_GetInput(topo_node->node, 0, &upstream_node, &upstream_output))) - { - session_deliver_pending_samples(session, upstream_node); - IMFTopologyNode_Release(upstream_node); - } - } - break; case SESSION_CMD_SET_RATE: session_set_rate(session, op->set_rate.thin, op->set_rate.rate); break; @@ -2442,6 +2420,71 @@ static const IMFAsyncCallbackVtbl session_commands_callback_vtbl = session_commands_callback_Invoke, }; +static HRESULT WINAPI session_sa_ready_callback_QueryInterface(IMFAsyncCallback *iface, REFIID riid, void **obj) +{ + if (IsEqualIID(riid, &IID_IMFAsyncCallback) + || IsEqualIID(riid, &IID_IUnknown)) + { + *obj = iface; + IMFAsyncCallback_AddRef(iface); + return S_OK; + } + + WARN("Unsupported %s.\n", debugstr_guid(riid)); + *obj = NULL; + return E_NOINTERFACE; +} + +static ULONG WINAPI session_sa_ready_callback_AddRef(IMFAsyncCallback *iface) +{ + struct media_session *session = impl_from_sa_ready_callback_IMFAsyncCallback(iface); + return IMFMediaSession_AddRef(&session->IMFMediaSession_iface); +} + +static ULONG WINAPI session_sa_ready_callback_Release(IMFAsyncCallback *iface) +{ + struct media_session *session = impl_from_sa_ready_callback_IMFAsyncCallback(iface); + return IMFMediaSession_Release(&session->IMFMediaSession_iface); +} + +static HRESULT WINAPI session_sa_ready_callback_GetParameters(IMFAsyncCallback *iface, DWORD *flags, DWORD *queue) +{ + return E_NOTIMPL; +} + +static HRESULT WINAPI session_sa_ready_callback_Invoke(IMFAsyncCallback *iface, IMFAsyncResult *result) +{ + IMFVideoSampleAllocatorNotify *notify = (IMFVideoSampleAllocatorNotify *)IMFAsyncResult_GetStateNoAddRef(result); + struct topo_node *topo_node = impl_node_from_IMFVideoSampleAllocatorNotify(notify); + struct media_session *session = impl_from_sa_ready_callback_IMFAsyncCallback(iface); + IMFTopologyNode *upstream_node; + DWORD upstream_output; + + EnterCriticalSection(&session->cs); + + if (topo_node->u.sink.requests) + { + if (SUCCEEDED(IMFTopologyNode_GetInput(topo_node->node, 0, &upstream_node, &upstream_output))) + { + session_deliver_pending_samples(session, upstream_node); + IMFTopologyNode_Release(upstream_node); + } + } + + LeaveCriticalSection(&session->cs); + + return S_OK; +} + +static const IMFAsyncCallbackVtbl session_sa_ready_callback_vtbl = +{ + session_sa_ready_callback_QueryInterface, + session_sa_ready_callback_AddRef, + session_sa_ready_callback_Release, + session_sa_ready_callback_GetParameters, + session_sa_ready_callback_Invoke, +}; + static HRESULT WINAPI session_events_callback_QueryInterface(IMFAsyncCallback *iface, REFIID riid, void **obj) { if (IsEqualIID(riid, &IID_IMFAsyncCallback) || @@ -3926,6 +3969,7 @@ HRESULT WINAPI MFCreateMediaSession(IMFAttributes *config, IMFMediaSession **ses object->IMFRateControl_iface.lpVtbl = &session_rate_control_vtbl; object->IMFTopologyNodeAttributeEditor_iface.lpVtbl = &node_attribute_editor_vtbl; object->commands_callback.lpVtbl = &session_commands_callback_vtbl; + object->sa_ready_callback.lpVtbl = &session_sa_ready_callback_vtbl; object->events_callback.lpVtbl = &session_events_callback_vtbl; object->sink_finalizer_callback.lpVtbl = &session_sink_finalizer_callback_vtbl; object->refcount = 1; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/607
From: R��mi Bernon <rbernon(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 2553369fb54..eedeaa540b3 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -216,6 +216,7 @@ enum presentation_flags SESSION_FLAG_NEEDS_PREROLL = 0x8, SESSION_FLAG_END_OF_PRESENTATION = 0x10, SESSION_FLAG_PENDING_RATE_CHANGE = 0x20, + SESSION_FLAG_PENDING_COMMAND = 0x40, }; struct media_session @@ -802,6 +803,8 @@ static void session_command_complete(struct media_session *session) struct session_op *op; struct list *e; + session->presentation.flags &= ~SESSION_FLAG_PENDING_COMMAND; + /* Pop current command, submit next. */ if ((e = list_head(&session->commands))) { @@ -2374,6 +2377,13 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, EnterCriticalSection(&session->cs); + if (session->presentation.flags & SESSION_FLAG_PENDING_COMMAND) + { + WARN("session %p command is in progress, waiting for it to complete.\n", session); + LeaveCriticalSection(&session->cs); + return S_OK; + } + switch (op->command) { case SESSION_CMD_CLEAR_TOPOLOGIES: @@ -3335,7 +3345,7 @@ static void session_raise_end_of_presentation(struct media_session *session) { if (session_nodes_is_mask_set(session, MF_TOPOLOGY_MAX, SOURCE_FLAG_END_OF_PRESENTATION)) { - session->presentation.flags |= SESSION_FLAG_END_OF_PRESENTATION; + session->presentation.flags |= SESSION_FLAG_END_OF_PRESENTATION | SESSION_FLAG_PENDING_COMMAND; session_push_back_command(session, SESSION_CMD_END); IMFMediaEventQueue_QueueEventParamVar(session->event_queue, MEEndOfPresentation, &GUID_NULL, S_OK, NULL); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/607
From: R��mi Bernon <rbernon(a)codeweavers.com> Instead of keeping the command ahead of the command list, making the SESSION_CMD_END internal command unnecessary. --- dlls/mf/session.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index eedeaa540b3..4d052efc85e 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -48,7 +48,6 @@ enum session_command SESSION_CMD_STOP, SESSION_CMD_SET_RATE, /* Internally used commands. */ - SESSION_CMD_END, SESSION_CMD_QM_NOTIFY_TOPOLOGY, }; @@ -456,22 +455,16 @@ static HRESULT session_is_shut_down(struct media_session *session) return session->state == SESSION_STATE_SHUT_DOWN ? MF_E_SHUTDOWN : S_OK; } -static void session_push_back_command(struct media_session *session, enum session_command command) -{ - struct session_op *op; - - if (SUCCEEDED(create_session_op(command, &op))) - list_add_head(&session->commands, &op->entry); -} - static HRESULT session_submit_command(struct media_session *session, struct session_op *op) { HRESULT hr; + TRACE("session %p, op %p, command %u.\n", session, op, op->command); + EnterCriticalSection(&session->cs); if (SUCCEEDED(hr = session_is_shut_down(session))) { - if (list_empty(&session->commands)) + if (list_empty(&session->commands) && !(session->presentation.flags & SESSION_FLAG_PENDING_COMMAND)) hr = MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &session->commands_callback, &op->IUnknown_iface); list_add_tail(&session->commands, &op->entry); IUnknown_AddRef(&op->IUnknown_iface); @@ -805,14 +798,7 @@ static void session_command_complete(struct media_session *session) session->presentation.flags &= ~SESSION_FLAG_PENDING_COMMAND; - /* Pop current command, submit next. */ - if ((e = list_head(&session->commands))) - { - op = LIST_ENTRY(e, struct session_op, entry); - list_remove(&op->entry); - IUnknown_Release(&op->IUnknown_iface); - } - + /* Submit next command. */ if ((e = list_head(&session->commands))) { op = LIST_ENTRY(e, struct session_op, entry); @@ -2375,6 +2361,8 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, struct session_op *op = impl_op_from_IUnknown(IMFAsyncResult_GetStateNoAddRef(result)); struct media_session *session = impl_from_commands_callback_IMFAsyncCallback(iface); + TRACE("session %p, op %p, command %u.\n", session, op, op->command); + EnterCriticalSection(&session->cs); if (session->presentation.flags & SESSION_FLAG_PENDING_COMMAND) @@ -2384,6 +2372,9 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, return S_OK; } + list_remove(&op->entry); + session->presentation.flags |= SESSION_FLAG_PENDING_COMMAND; + switch (op->command) { case SESSION_CMD_CLEAR_TOPOLOGIES: @@ -2418,6 +2409,8 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, LeaveCriticalSection(&session->cs); + IUnknown_Release(&op->IUnknown_iface); + return S_OK; } @@ -3346,7 +3339,6 @@ static void session_raise_end_of_presentation(struct media_session *session) if (session_nodes_is_mask_set(session, MF_TOPOLOGY_MAX, SOURCE_FLAG_END_OF_PRESENTATION)) { session->presentation.flags |= SESSION_FLAG_END_OF_PRESENTATION | SESSION_FLAG_PENDING_COMMAND; - session_push_back_command(session, SESSION_CMD_END); IMFMediaEventQueue_QueueEventParamVar(session->event_queue, MEEndOfPresentation, &GUID_NULL, S_OK, NULL); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/607
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/mf/session.c | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 4d052efc85e..8be3ffcd7f3 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -47,8 +47,6 @@ enum session_command SESSION_CMD_PAUSE, SESSION_CMD_STOP, SESSION_CMD_SET_RATE, - /* Internally used commands. */ - SESSION_CMD_QM_NOTIFY_TOPOLOGY, }; struct session_op @@ -414,10 +412,6 @@ static ULONG WINAPI session_op_Release(IUnknown *iface) case SESSION_CMD_START: PropVariantClear(&op->start.start_position); break; - case SESSION_CMD_QM_NOTIFY_TOPOLOGY: - if (op->notify_topology.topology) - IMFTopology_Release(op->notify_topology.topology); - break; default: ; } @@ -1680,21 +1674,9 @@ static HRESULT session_set_current_topology(struct media_session *session, IMFTo DWORD caps, object_flags; struct media_sink *sink; struct topo_node *node; - struct session_op *op; IMFMediaEvent *event; HRESULT hr; - if (session->quality_manager) - { - if (SUCCEEDED(create_session_op(SESSION_CMD_QM_NOTIFY_TOPOLOGY, &op))) - { - op->notify_topology.topology = topology; - IMFTopology_AddRef(op->notify_topology.topology); - session_submit_command(session, op); - IUnknown_Release(&op->IUnknown_iface); - } - } - if (FAILED(hr = IMFTopology_CloneFrom(session->presentation.current_topology, topology))) { WARN("Failed to clone topology, hr %#lx.\n", hr); @@ -1826,6 +1808,8 @@ static void session_set_topology(struct media_session *session, DWORD flags, IMF { hr = session_set_current_topology(session, topology); session_set_topo_status(session, hr, MF_TOPOSTATUS_READY); + if (session->quality_manager) + IMFQualityManager_NotifyTopology(session->quality_manager, topology); } } @@ -2396,10 +2380,6 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, case SESSION_CMD_CLOSE: session_close(session); break; - case SESSION_CMD_QM_NOTIFY_TOPOLOGY: - IMFQualityManager_NotifyTopology(session->quality_manager, op->notify_topology.topology); - session_command_complete(session); - break; case SESSION_CMD_SET_RATE: session_set_rate(session, op->set_rate.thin, op->set_rate.rate); break; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/607
On Tue Aug 30 11:35:12 2022 +0000, R��mi Bernon wrote:
I've been looking into that, and I now think I better understand the logic. There's two different issues at play, one is the SA_READY_COMMAND, which is currently unconditionally queueing callbacks commands into the task queue, causing race conditions when other commands such as START or END are pending. I think this issue is solved by making the sample allocator ready notifications a separate callback. Then, there's the handling of the END command. I believe that regardless of how we keep track of the pending command, it'll still be racy the way it's currently done. For instance, consider the following sequence: Initially the command queue is empty, the session is playing. 1. Thread 1 calls IMFMediaSession_Close. 2. Thread 2 receives MEEndOfPresentation. 3. Thread 1 enters session CS. * Append a CLOSE command. * Queue the callback task to Thread 3. 4. Thread 1 leaves session CS. 5. Thread 3 calls session_commands_callback_Invoke. 6. Thread 2 enters session CS. * Prepend a END command. 7. Thread 2 leaves session CS. 8. Thread 3 enters session CS. * Removes the END command, does nothing. 9. Thread 3 leaves session CS. Thread 3 is now idle as no more callback task has been queued. Later, when either MEStreamSinkMarker event or any of the MEStreamSink* state events are received, the session will complete the pending command which is now the CLOSE command, but not actually execute it. I think the same can happen with commands other than Close, and I don't think having a separate pending command would actually change anything here. I think this should be fixed now.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/607#note_7545
Thanks, this works now with my test player. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/607#note_7554
This merge request was approved by Nikolay Sivov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/607
participants (2)
-
Nikolay Sivov (＠nsivov) -
Rémi Bernon