[PATCH v6 0/4] MR607: mf: Fix session command handling and SESSION_CMD_END.

[Rémi Bernon (＠rbernon)]

--
  v6: mf: Notify quality manager of topology change in session_set_topology.
      mf: Use the SESSION_FLAG_PENDING_COMMAND to delay further commands.
      mf: Delay media session command processing when presentation is ending.
      mf: Use a dedicated interface for sample allocator ready callbacks.

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

[PATCH v6 1/4] mf: Use a dedicated interface for sample allocator ready callbacks.

[Rémi Bernon]

From: R��mi Bernon

---
 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..2553369fb54 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 sa_ready_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_sa_ready_callback_IMFAsyncCallback(IMFAsyncCallback *iface)
+{
+    return CONTAINING_RECORD(iface, struct media_session, sa_ready_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->sa_ready_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_sa_ready_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_sa_ready_callback_AddRef(IMFAsyncCallback *iface)
+{
+    struct media_session *session = impl_from_sa_ready_callback_IMFAsyncCallback(iface);
+    return IMFMediaSession_AddRef(&session->IMFMediaSession_iface);
+}
+
+static ULONG WINAPI session_sa_ready_callback_Release(IMFAsyncCallback *iface)
+{
+    struct media_session *session = impl_from_sa_ready_callback_IMFAsyncCallback(iface);
+    return IMFMediaSession_Release(&session->IMFMediaSession_iface);
+}
+
+static HRESULT WINAPI session_sa_ready_callback_GetParameters(IMFAsyncCallback *iface, DWORD *flags, DWORD *queue)
+{
+    return E_NOTIMPL;
+}
+
+static HRESULT WINAPI session_sa_ready_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_sa_ready_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_sa_ready_callback_vtbl =
+{
+    session_sa_ready_callback_QueryInterface,
+    session_sa_ready_callback_AddRef,
+    session_sa_ready_callback_Release,
+    session_sa_ready_callback_GetParameters,
+    session_sa_ready_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->sa_ready_callback.lpVtbl = &session_sa_ready_callback_vtbl;
     object->events_callback.lpVtbl = &session_events_callback_vtbl;
     object->sink_finalizer_callback.lpVtbl = &session_sink_finalizer_callback_vtbl;
     object->refcount = 1;

[PATCH v6 2/4] mf: Delay media session command processing when presentation is ending.

[Rémi Bernon]

From: R��mi Bernon

---
 dlls/mf/session.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/dlls/mf/session.c b/dlls/mf/session.c
index 2553369fb54..eedeaa540b3 100644
--- a/dlls/mf/session.c
+++ b/dlls/mf/session.c
@@ -216,6 +216,7 @@ enum presentation_flags
     SESSION_FLAG_NEEDS_PREROLL = 0x8,
     SESSION_FLAG_END_OF_PRESENTATION = 0x10,
     SESSION_FLAG_PENDING_RATE_CHANGE = 0x20,
+    SESSION_FLAG_PENDING_COMMAND = 0x40,
 };

struct media_session
@@ -802,6 +803,8 @@ static void session_command_complete(struct media_session *session)
     struct session_op *op;
     struct list *e;

+    session->presentation.flags &= ~SESSION_FLAG_PENDING_COMMAND;
+
     /* Pop current command, submit next. */
     if ((e = list_head(&session->commands)))
     {
@@ -2374,6 +2377,13 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface,

EnterCriticalSection(&session->cs);

+    if (session->presentation.flags & SESSION_FLAG_PENDING_COMMAND)
+    {
+        WARN("session %p command is in progress, waiting for it to complete.\n", session);
+        LeaveCriticalSection(&session->cs);
+        return S_OK;
+    }
+
     switch (op->command)
     {
         case SESSION_CMD_CLEAR_TOPOLOGIES:
@@ -3335,7 +3345,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->presentation.flags |= SESSION_FLAG_END_OF_PRESENTATION | SESSION_FLAG_PENDING_COMMAND;
             session_push_back_command(session, SESSION_CMD_END);
             IMFMediaEventQueue_QueueEventParamVar(session->event_queue, MEEndOfPresentation, &GUID_NULL, S_OK, NULL);
         }

[PATCH v6 3/4] mf: Use the SESSION_FLAG_PENDING_COMMAND to delay further commands.

[Rémi Bernon]

From: R��mi Bernon

Instead of keeping the command ahead of the command list, making the
SESSION_CMD_END internal command unnecessary.
---
 dlls/mf/session.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/dlls/mf/session.c b/dlls/mf/session.c
index eedeaa540b3..4d052efc85e 100644
--- a/dlls/mf/session.c
+++ b/dlls/mf/session.c
@@ -48,7 +48,6 @@ enum session_command
     SESSION_CMD_STOP,
     SESSION_CMD_SET_RATE,
     /* Internally used commands. */
-    SESSION_CMD_END,
     SESSION_CMD_QM_NOTIFY_TOPOLOGY,
 };

@@ -456,22 +455,16 @@ 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;

+    TRACE("session %p, op %p, command %u.\n", session, op, op->command);
+
     EnterCriticalSection(&session->cs);
     if (SUCCEEDED(hr = session_is_shut_down(session)))
     {
-        if (list_empty(&session->commands))
+        if (list_empty(&session->commands) && !(session->presentation.flags & SESSION_FLAG_PENDING_COMMAND))
             hr = MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &session->commands_callback, &op->IUnknown_iface);
         list_add_tail(&session->commands, &op->entry);
         IUnknown_AddRef(&op->IUnknown_iface);
@@ -805,14 +798,7 @@ static void session_command_complete(struct media_session *session)

session->presentation.flags &= ~SESSION_FLAG_PENDING_COMMAND;

-    /* Pop current command, submit next. */
-    if ((e = list_head(&session->commands)))
-    {
-        op = LIST_ENTRY(e, struct session_op, entry);
-        list_remove(&op->entry);
-        IUnknown_Release(&op->IUnknown_iface);
-    }
-
+    /* Submit next command. */
     if ((e = list_head(&session->commands)))
     {
         op = LIST_ENTRY(e, struct session_op, entry);
@@ -2375,6 +2361,8 @@ 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);

+    TRACE("session %p, op %p, command %u.\n", session, op, op->command);
+
     EnterCriticalSection(&session->cs);

if (session->presentation.flags & SESSION_FLAG_PENDING_COMMAND)
@@ -2384,6 +2372,9 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface,
         return S_OK;
     }

+    list_remove(&op->entry);
+    session->presentation.flags |= SESSION_FLAG_PENDING_COMMAND;
+
     switch (op->command)
     {
         case SESSION_CMD_CLEAR_TOPOLOGIES:
@@ -2418,6 +2409,8 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface,

LeaveCriticalSection(&session->cs);

+    IUnknown_Release(&op->IUnknown_iface);
+
     return S_OK;
 }

@@ -3346,7 +3339,6 @@ 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_FLAG_PENDING_COMMAND;
-            session_push_back_command(session, SESSION_CMD_END);
             IMFMediaEventQueue_QueueEventParamVar(session->event_queue, MEEndOfPresentation, &GUID_NULL, S_OK, NULL);
         }
     }

[PATCH v6 4/4] mf: Notify quality manager of topology change in session_set_topology.

[Rémi Bernon]

From: R��mi Bernon

---
 dlls/mf/session.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/dlls/mf/session.c b/dlls/mf/session.c
index 4d052efc85e..8be3ffcd7f3 100644
--- a/dlls/mf/session.c
+++ b/dlls/mf/session.c
@@ -47,8 +47,6 @@ enum session_command
     SESSION_CMD_PAUSE,
     SESSION_CMD_STOP,
     SESSION_CMD_SET_RATE,
-    /* Internally used commands. */
-    SESSION_CMD_QM_NOTIFY_TOPOLOGY,
 };

struct session_op
@@ -414,10 +412,6 @@ static ULONG WINAPI session_op_Release(IUnknown *iface)
             case SESSION_CMD_START:
                 PropVariantClear(&op->start.start_position);
                 break;
-            case SESSION_CMD_QM_NOTIFY_TOPOLOGY:
-                if (op->notify_topology.topology)
-                    IMFTopology_Release(op->notify_topology.topology);
-                break;
             default:
                 ;
         }
@@ -1680,21 +1674,9 @@ static HRESULT session_set_current_topology(struct media_session *session, IMFTo
     DWORD caps, object_flags;
     struct media_sink *sink;
     struct topo_node *node;
-    struct session_op *op;
     IMFMediaEvent *event;
     HRESULT hr;

-    if (session->quality_manager)
-    {
-        if (SUCCEEDED(create_session_op(SESSION_CMD_QM_NOTIFY_TOPOLOGY, &op)))
-        {
-            op->notify_topology.topology = topology;
-            IMFTopology_AddRef(op->notify_topology.topology);
-            session_submit_command(session, op);
-            IUnknown_Release(&op->IUnknown_iface);
-        }
-    }
-
     if (FAILED(hr = IMFTopology_CloneFrom(session->presentation.current_topology, topology)))
     {
         WARN("Failed to clone topology, hr %#lx.\n", hr);
@@ -1826,6 +1808,8 @@ static void session_set_topology(struct media_session *session, DWORD flags, IMF
         {
             hr = session_set_current_topology(session, topology);
             session_set_topo_status(session, hr, MF_TOPOSTATUS_READY);
+            if (session->quality_manager)
+                IMFQualityManager_NotifyTopology(session->quality_manager, topology);
         }
     }

@@ -2396,10 +2380,6 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface,
         case SESSION_CMD_CLOSE:
             session_close(session);
             break;
-        case SESSION_CMD_QM_NOTIFY_TOPOLOGY:
-            IMFQualityManager_NotifyTopology(session->quality_manager, op->notify_topology.topology);
-            session_command_complete(session);
-            break;
         case SESSION_CMD_SET_RATE:
             session_set_rate(session, op->set_rate.thin, op->set_rate.rate);
             break;

[Rémi Bernon (＠rbernon)]

On Tue Aug 30 11:35:12 2022 +0000, R��mi Bernon wrote:
> 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.
> - Thread 1 calls IMFMediaSession_Close.
> - Thread 2 receives MEEndOfPresentation.
> - Thread 1 enters session CS.
> - Append a CLOSE command.
> - Queue the callback task to Thread 3.
> - Thread 1 leaves session CS.
> - Thread 3 calls session_commands_callback_Invoke.
> - Thread 2 enters session CS.
> - Prepend a END command.
> - Thread 2 leaves session CS.
> - Thread 3 enters session CS.
> - Removes the END command, does nothing.
> - 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.

I think this should be fixed now.

[Nikolay Sivov (＠nsivov)]

Thanks, this works now with my test player.

[PATCH v6 0/4] MR607: mf: Fix session command handling and SESSION_CMD_END. - approved

[Nikolay Sivov (＠nsivov)]

This merge request was approved by Nikolay Sivov.