[PATCH v3 0/1] MR8722: mf: Add a SUBMITTED command state to avoid multiple submission of the same op.
If a session is shutdown when its command_state is already COMPLETE, session_handle_source_shutdown calls session_set_stopped, which calls session_command_complete which will submit the op at the head of commands again, despite it having already been submitted. Given that only the op at the head of commands can be in the submitted state, tracking it as a command state makes more sense to me. And if the command state is SUBMITTED we know not to submit the op again. * * * Superceded !8270 -- v3: mf: Add a SUBMITTED command state to avoid multiple submission of the same op. https://gitlab.winehq.org/wine/wine/-/merge_requests/8722
From: Yuxuan Shui <yshui(a)codeweavers.com> If a session is shutdown when its command_state is already COMPLETE, session_handle_source_shutdown calls session_set_stopped, which calls session_command_complete which will submit the op at the head of commands again, despite it having already been submitted. Given that only the op at the head of commands can be in the submitted state, tracking it as a command state makes more sense to me. And if the command state is SUBMITTED we know not to submit the op again. --- dlls/mf/session.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 87af5b95eab..9d9ee15ac8d 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -57,7 +57,6 @@ struct session_op IUnknown IUnknown_iface; LONG refcount; enum session_command command; - BOOL submitted; union { struct @@ -102,6 +101,9 @@ enum session_state enum command_state { COMMAND_STATE_COMPLETE = 0, + /* Command submitted but hasn't started execution. + * invariant: the submitted command is always the head of session->commands. */ + COMMAND_STATE_SUBMITTED, /* STOPPED | PAUSED | STARTED -> STARTED transition */ COMMAND_STATE_RESTARTING_SOURCES, /* -> COMMAND_STATE_STARTING_SOURCES */ COMMAND_STATE_STARTING_SOURCES, /* -> COMMAND_STATE_PREROLLING_SINKS | COMMAND_STATE_STARTING_SINKS */ @@ -485,7 +487,9 @@ static HRESULT session_submit_command(struct media_session *session, struct sess if (list_empty(&session->commands) && session->command_state == COMMAND_STATE_COMPLETE) { hr = MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &session->commands_callback, &op->IUnknown_iface); - op->submitted = SUCCEEDED(hr); + /* Since session->commands is empty, whether `op` is added to head or tail, it will + * always be at the head of this list, thus the invariant holds. */ + if (SUCCEEDED(hr)) session->command_state = COMMAND_STATE_SUBMITTED; } if (op->command == SESSION_CMD_SHUTDOWN) list_add_head(&session->commands, &op->entry); @@ -869,14 +873,11 @@ static void session_shutdown_current_topology(struct media_session *session) static void session_clear_command_list(struct media_session *session) { struct session_op *op, *op2; - + /* Checking this flag is unnecessary if this function is only called + * from the callback or upon release, but do it for consistency and + * in case a call from elsewhere is added. */ LIST_FOR_EACH_ENTRY_SAFE(op, op2, &session->commands, struct session_op, entry) { - /* Checking this flag is unnecessary if this function is only called - * from the callback or upon release, but do it for consistency and - * in case a call from elsewhere is added. */ - if (op->submitted) - continue; list_remove(&op->entry); IUnknown_Release(&op->IUnknown_iface); } @@ -982,7 +983,7 @@ static void session_command_complete(struct media_session *session) { op = LIST_ENTRY(e, struct session_op, entry); hr = MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &session->commands_callback, &op->IUnknown_iface); - op->submitted = SUCCEEDED(hr); + if (SUCCEEDED(hr)) session->command_state = COMMAND_STATE_SUBMITTED; } } @@ -2779,12 +2780,7 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, EnterCriticalSection(&session->cs); - if (session->command_state != COMMAND_STATE_COMPLETE) - { - WARN("session %p command is in progress, waiting for it to complete.\n", session); - LeaveCriticalSection(&session->cs); - return S_OK; - } + assert( session->command_state == COMMAND_STATE_SUBMITTED ); list_remove(&op->entry); switch (op->command) @@ -2954,6 +2950,8 @@ static void session_handle_source_shutdown(struct media_session *session) if (session->state == SESSION_STATE_STARTED || session->state == SESSION_STATE_PAUSED) session_set_stopped(session, MESessionStopped, MF_E_SHUTDOWN); break; + case COMMAND_STATE_SUBMITTED: + break; } session->presentation.flags |= SESSION_FLAG_SOURCE_SHUTDOWN; @@ -3306,6 +3304,7 @@ static void session_set_source_object_state(struct media_session *session, IUnkn session_set_caps(session, session->caps & ~MFSESSIONCAP_PAUSE); session_finalize_sinks(session); break; + case COMMAND_STATE_SUBMITTED: case COMMAND_STATE_COMPLETE: case COMMAND_STATE_PREROLLING_SINKS: case COMMAND_STATE_STARTING_SINKS: @@ -3400,6 +3399,7 @@ static void session_set_sink_stream_state(struct media_session *session, IMFStre if (FAILED(hr)) session_set_closed(session, hr); break; + case COMMAND_STATE_SUBMITTED: case COMMAND_STATE_COMPLETE: case COMMAND_STATE_RESTARTING_SOURCES: case COMMAND_STATE_STARTING_SOURCES: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8722
mf:transform now crashes, looking into it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8722#note_112516
On Fri Aug 8 17:33:23 2025 +0000, Yuxuan Shui wrote:
mf:transform now crashes, looking into it. nope, broken on master as well. someone else broke it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8722#note_112518
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8722
@nsivov ping. this has been tested with several games on Proton -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8722#note_117312
@yshui what should happen if e.g. multiple Start() commands are submitted with different positions? Right now I think we are going to queue them all and execute one by one, but maybe it should drop pending ones and use the most recent? It's just something that occurred to me, don't know if it's closely related to this fix. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8722#note_117499
On Fri Oct 3 08:31:08 2025 +0000, Yuxuan Shui wrote:
@nsivov ping. this has been tested with several games on Proton Ok, I'm going to trust @rbernon's judgement. Could you rebase to re-run the CI please.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8722#note_117500
This merge request was approved by Nikolay Sivov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8722
participants (4)
-
Nikolay Sivov (＠nsivov) -
Rémi Bernon
-
Yuxuan Shui
-
Yuxuan Shui (＠yshui)