[PATCH 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

[PATCH 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 | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/dlls/mf/session.c b/dlls/mf/session.c
index 87af5b95eab..ef956298a0a 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,18 @@ 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. */
+    BOOL submitted = session->command_state == COMMAND_STATE_SUBMITTED;

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)
+        if (submitted)
+        {
+            submitted = FALSE;
             continue;
+        }
         list_remove(&op->entry);
         IUnknown_Release(&op->IUnknown_iface);
     }
@@ -975,6 +983,8 @@ static void session_command_complete(struct media_session *session)
     struct list *e;
     HRESULT hr;

+    if (session->command_state == COMMAND_STATE_SUBMITTED) return;
+
     session->command_state = COMMAND_STATE_COMPLETE;

/* Submit next command. */
@@ -982,7 +992,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 +2789,21 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface,

EnterCriticalSection(&session->cs);

-    if (session->command_state != COMMAND_STATE_COMPLETE)
+    if (session->command_state != COMMAND_STATE_COMPLETE &&
+        session->command_state != COMMAND_STATE_SUBMITTED)
     {
         WARN("session %p command is in progress, waiting for it to complete.\n", session);
         LeaveCriticalSection(&session->cs);
         return S_OK;
     }
+
+    if (&op->entry == list_head(&session->commands) && session->command_state == COMMAND_STATE_SUBMITTED)
+        session->command_state = COMMAND_STATE_COMPLETE;
+    else if (session->command_state == COMMAND_STATE_SUBMITTED)
+        WARN("invoked command is not the one submitted.\n");
+    else
+        WARN("no command was submitted.\n");
+
     list_remove(&op->entry);

switch (op->command)
@@ -2950,6 +2969,7 @@ static void session_handle_source_shutdown(struct media_session *session)
             session->state = SESSION_STATE_CLOSED;
             session_command_complete_with_event(session, MESessionClosed, MF_E_SHUTDOWN, NULL);
             break;
+        case COMMAND_STATE_SUBMITTED:
         case COMMAND_STATE_COMPLETE:
             if (session->state == SESSION_STATE_STARTED || session->state == SESSION_STATE_PAUSED)
                 session_set_stopped(session, MESessionStopped, MF_E_SHUTDOWN);
@@ -3306,6 +3326,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 +3421,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:

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/mf/session.c:
>          session->state = SESSION_STATE_CLOSED;
>          session_command_complete_with_event(session, MESessionClosed, MF_E_SHUTDOWN, NULL);
>          break;
> -
>    case COMMAND_STATE_SUBMITTED:
>    case COMMAND_STATE_COMPLETE:
>        if (session->state == SESSION_STATE_STARTED || session->state == SESSION_STATE_PAUSED)
>            session_set_stopped(session, MESessionStopped, MF_E_SHUTDOWN);
>        break;

```suggestion:-4+0
        case COMMAND_STATE_COMPLETE:
            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;
```

This is where the issue may come from (ie: `session_set_stopped` calling `session_command_complete` which needs to avoid submitting the command again if it was already), and I think we can simply avoid doing anything if a command has been submitted already: either it's a command that will try to use the same source and it will handle the source shutdown eventually, or it's a command that will clear the topology and already do the proper state cleanup and update.

I believe this creates a stronger invariant for session_command_complete: it can safely complete the pending command if any, without having to special case `COMMAND_STATE_SUBMITTED` itself.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/mf/session.c:
> - if (session->command_state != COMMAND_STATE_COMPLETE)
> - if (session->command_state != COMMAND_STATE_COMPLETE &&
> -
>    session->command_state != COMMAND_STATE_SUBMITTED)
> {
>        WARN("session %p command is in progress, waiting for it to complete.\n", session);
>        LeaveCriticalSection(&session->cs);
>        return S_OK;
>    }
> -
> - if (&op->entry == list_head(&session->commands) && session->command_state == COMMAND_STATE_SUBMITTED)
> -
>    session->command_state = COMMAND_STATE_COMPLETE;
> - else if (session->command_state == COMMAND_STATE_SUBMITTED)
> -
>    WARN("invoked command is not the one submitted.\n");
> - else
> -
>    WARN("no command was submitted.\n");
> -

When the command callback is invoked, its command state has to be `COMMAND_STATE_SUBMITTED`. I think that check is also not necessary anymore, unless there's something wrong with our state machines.

```suggestion:-14+0
    assert( session->command_state == COMMAND_STATE_SUBMITTED );
```

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/mf/session.c:
>  struct list *e;
>  HRESULT hr;
>
> - if (session->command_state == COMMAND_STATE_SUBMITTED) return;
> -

This then shouldn't be necessary, the function should only ever be called with the proper command state.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/mf/session.c:
>  /* 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)
> - BOOL submitted = session->command_state == COMMAND_STATE_SUBMITTED;
> -
> - LIST_FOR_EACH_ENTRY_SAFE(op, op2, &session->commands, struct session_op, entry)
> - {
> -
>    if (submitted)
> -
>    {
> -
>        submitted = FALSE;
>        continue;
> -
>    }
>    list_remove(&op->entry);
>    IUnknown_Release(&op->IUnknown_iface);
> }

```suggestion:-11+0
    LIST_FOR_EACH_ENTRY_SAFE(op, op2, &session->commands, struct session_op, entry)
    {
        list_remove(&op->entry);
        IUnknown_Release(&op->IUnknown_iface);
    }
```

This isn't necessary, `session_clear_command_list` is only called on last release or on `SESSION_CMD_SHUTDOWN`. In the former case it's not possible to have any submitted command, and in the latter the submitted command should've removed from the list already from `session_commands_callback_Invoke`.

[Rémi Bernon (＠rbernon)]

One thing maybe worth adding is `session_raise_end_of_presentation`, which maybe should include `COMMAND_STATE_SUBMITTED` too, but I'm not 100% sure it should (and would be difficult to test).

[Rémi Bernon (＠rbernon)]

On Wed Aug  6 13:54:06 2025 +0000, Rémi Bernon wrote:
> One thing maybe worth adding is `session_raise_end_of_presentation`,
> which maybe should include `COMMAND_STATE_SUBMITTED` too, but I'm not
> 100% sure it should (and would be difficult to test).

Eh no, it shouldn't as it needs to transition to COMMAND_STATE_ENDING_STREAMS.