[PATCH v2 0/2] 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

--
  v2: Apply 3 suggestion(s) to 1 file(s)

https://gitlab.winehq.org/wine/wine/-/merge_requests/8722

[PATCH v2 1/2] 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:

[PATCH v2 2/2] Apply 3 suggestion(s) to 1 file(s)

[Yuxuan Shui]

From: Yuxuan Shui

Co-authored-by: Rémi Bernon 
---
 dlls/mf/session.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/dlls/mf/session.c b/dlls/mf/session.c
index ef956298a0a..975431672b4 100644
--- a/dlls/mf/session.c
+++ b/dlls/mf/session.c
@@ -876,15 +876,8 @@ static void session_clear_command_list(struct media_session *session)
     /* 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)
     {
-        if (submitted)
-        {
-            submitted = FALSE;
-            continue;
-        }
         list_remove(&op->entry);
         IUnknown_Release(&op->IUnknown_iface);
     }
@@ -2789,21 +2782,7 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface,

EnterCriticalSection(&session->cs);

-    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");
-
+    assert( session->command_state == COMMAND_STATE_SUBMITTED );
     list_remove(&op->entry);

switch (op->command)
@@ -2969,11 +2948,12 @@ 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);
             break;
+        case COMMAND_STATE_SUBMITTED:
+            break;
     }

session->presentation.flags |= SESSION_FLAG_SOURCE_SHUTDOWN;