-- v4: mf: Actually implement SESSION_CMD_END internal command. mf: Use session_submit_command to put SESSION_CMD_END ahead of the queue. mf: Keep pending session command out of the queued commands list. mf: Use a dedicated session callback interface for sample requests.
From: R��mi Bernon rbernon@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..7d46b2c1bdf 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 requests_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_requests_callback_IMFAsyncCallback(IMFAsyncCallback *iface) +{ + return CONTAINING_RECORD(iface, struct media_session, requests_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->requests_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_requests_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_requests_callback_AddRef(IMFAsyncCallback *iface) +{ + struct media_session *session = impl_from_requests_callback_IMFAsyncCallback(iface); + return IMFMediaSession_AddRef(&session->IMFMediaSession_iface); +} + +static ULONG WINAPI session_requests_callback_Release(IMFAsyncCallback *iface) +{ + struct media_session *session = impl_from_requests_callback_IMFAsyncCallback(iface); + return IMFMediaSession_Release(&session->IMFMediaSession_iface); +} + +static HRESULT WINAPI session_requests_callback_GetParameters(IMFAsyncCallback *iface, DWORD *flags, DWORD *queue) +{ + return E_NOTIMPL; +} + +static HRESULT WINAPI session_requests_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_requests_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_requests_callback_vtbl = +{ + session_requests_callback_QueryInterface, + session_requests_callback_AddRef, + session_requests_callback_Release, + session_requests_callback_GetParameters, + session_requests_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->requests_callback.lpVtbl = &session_requests_callback_vtbl; object->events_callback.lpVtbl = &session_events_callback_vtbl; object->sink_finalizer_callback.lpVtbl = &session_sink_finalizer_callback_vtbl; object->refcount = 1;
From: R��mi Bernon rbernon@codeweavers.com
--- dlls/mf/session.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 7d46b2c1bdf..abe50aed1d3 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -254,7 +254,10 @@ struct media_session float rate; } presentation; struct list topologies; + + struct session_op *pending_command; struct list commands; + enum session_state state; DWORD caps; CRITICAL_SECTION cs; @@ -470,7 +473,7 @@ static HRESULT session_submit_command(struct media_session *session, struct sess EnterCriticalSection(&session->cs); if (SUCCEEDED(hr = session_is_shut_down(session))) { - if (list_empty(&session->commands)) + if (!session->pending_command && list_empty(&session->commands)) hr = MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &session->commands_callback, &op->IUnknown_iface); list_add_tail(&session->commands, &op->entry); IUnknown_AddRef(&op->IUnknown_iface); @@ -736,6 +739,12 @@ static void session_clear_command_list(struct media_session *session) { struct session_op *op, *op2;
+ if ((op = session->pending_command)) + { + IUnknown_Release(&op->IUnknown_iface); + session->pending_command = NULL; + } + LIST_FOR_EACH_ENTRY_SAFE(op, op2, &session->commands, struct session_op, entry) { list_remove(&op->entry); @@ -802,12 +811,10 @@ static void session_command_complete(struct media_session *session) struct session_op *op; struct list *e;
- /* Pop current command, submit next. */ - if ((e = list_head(&session->commands))) + if ((op = session->pending_command)) { - op = LIST_ENTRY(e, struct session_op, entry); - list_remove(&op->entry); IUnknown_Release(&op->IUnknown_iface); + session->pending_command = NULL; }
if ((e = list_head(&session->commands))) @@ -2374,6 +2381,9 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface,
EnterCriticalSection(&session->cs);
+ list_remove(&op->entry); + session->pending_command = op; + switch (op->command) { case SESSION_CMD_CLEAR_TOPOLOGIES:
From: R��mi Bernon rbernon@codeweavers.com
--- dlls/mf/session.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/dlls/mf/session.c b/dlls/mf/session.c index abe50aed1d3..821e380be4e 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -458,14 +458,6 @@ 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; @@ -475,7 +467,10 @@ static HRESULT session_submit_command(struct media_session *session, struct sess { if (!session->pending_command && list_empty(&session->commands)) hr = MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &session->commands_callback, &op->IUnknown_iface); - list_add_tail(&session->commands, &op->entry); + if (op->command == SESSION_CMD_END) + list_add_head(&session->commands, &op->entry); + else + list_add_tail(&session->commands, &op->entry); IUnknown_AddRef(&op->IUnknown_iface); } LeaveCriticalSection(&session->cs); @@ -3346,7 +3341,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_push_back_command(session, SESSION_CMD_END); + session_submit_simple_command(session, SESSION_CMD_END); IMFMediaEventQueue_QueueEventParamVar(session->event_queue, MEEndOfPresentation, &GUID_NULL, S_OK, NULL); } }
From: R��mi Bernon rbernon@codeweavers.com
--- dlls/mf/session.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 821e380be4e..d2cd9562f10 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -2394,6 +2394,10 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, case SESSION_CMD_PAUSE: session_pause(session); break; + case SESSION_CMD_END: + session_set_topo_status(session, S_OK, MF_TOPOSTATUS_ENDED); + session_set_caps(session, session->caps & ~MFSESSIONCAP_PAUSE); + /* fallthrough */ case SESSION_CMD_STOP: session_stop(session); break; @@ -2407,8 +2411,6 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, case SESSION_CMD_SET_RATE: session_set_rate(session, op->set_rate.thin, op->set_rate.rate); break; - default: - ; }
LeaveCriticalSection(&session->cs);
Sorry I mixed up some patch with Proton, which doesn't have 048e21d22e33fb8187532851dad6ce566592156b yet.
On Tue Aug 16 08:06:27 2022 +0000, R��mi Bernon wrote:
Sorry I mixed up some patch with Proton, which doesn't have 048e21d22e33fb8187532851dad6ce566592156b yet.
FWIW last revision of the MR should be fine regarding this comment.
Separating current command looks okay to me, but stopping the session on CMD_END is causing problems by sending MESessionStopped(MF_E_INVALIDREQUEST). You can see this with attached mfplay program, that's using EVR internally. My original idea for CMD_END was to use it as a marker in commands queue that would block pending commands, allowing for end-of-presentation transitions to complete properly. For me with this MR it's still at SESSION_STATE_STOPPING_SINKS when session_stop() is called. Note that you need !608 to use this program.
P.S. please rename requests_callback to sa_ready_callback or something similar, it's about checking for pending samples, and not about making new requests.
[MFPlayer.exe](/uploads/bd4e0e1f11666eaf435a6ad54045655e/MFPlayer.exe)
On Tue Aug 30 11:35:12 2022 +0000, Nikolay Sivov wrote:
Separating current command looks okay to me, but stopping the session on CMD_END is causing problems by sending MESessionStopped(MF_E_INVALIDREQUEST). You can see this with attached mfplay program, that's using EVR internally. My original idea for CMD_END was to use it as a marker in commands queue that would block pending commands, allowing for end-of-presentation transitions to complete properly. For me with this MR it's still at SESSION_STATE_STOPPING_SINKS when session_stop() is called. Note that you need !608 to use this program. P.S. please rename requests_callback to sa_ready_callback or something similar, it's about checking for pending samples, and not about making new requests. [MFPlayer.exe](/uploads/bd4e0e1f11666eaf435a6ad54045655e/MFPlayer.exe)
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.
-
Nikolay Sivov (＠nsivov) -
Rémi Bernon
-
Rémi Bernon (＠rbernon)