[PATCH v3 0/1] MR8722: mf: Add a SUBMITTED command state to avoid multiple submission of the same op.

[Yuxuan Shui (＠yshui)]

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

[PATCH v3 1/1] mf: Add a SUBMITTED command state to avoid multiple submission of the same op.

[Yuxuan Shui]

From: Yuxuan Shui

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:

[Yuxuan Shui (＠yshui)]

mf:transform now crashes, looking into it.

[Yuxuan Shui (＠yshui)]

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.

[PATCH v3 0/1] MR8722: mf: Add a SUBMITTED command state to avoid multiple submission of the same op. - approved

[Rémi Bernon (＠rbernon)]

This merge request was approved by Rémi Bernon.

[Yuxuan Shui (＠yshui)]

@nsivov ping. this has been tested with several games on Proton

[Nikolay Sivov (＠nsivov)]

@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.

[Nikolay Sivov (＠nsivov)]

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.

[PATCH v3 0/1] MR8722: mf: Add a SUBMITTED command state to avoid multiple submission of the same op. - approved

[Nikolay Sivov (＠nsivov)]

This merge request was approved by Nikolay Sivov.