[PATCH 0/1] MR7932: mf: Restart transforms and sinks on seek.

[Brendan McGrath (＠redmcg)]

This MR fixes seek in VRChat by copying the sequence of flushes/stop/starts that Windows does.

The order on Windows is:
1. Stop sources;
2. Flush MFTs;
3. Start sources;
4. Request output down the chain of sink inputs;
6. Flush sinks; and
7. Start the clock

This takes place whether we pause before we seek or seek without pause.

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

[PATCH 1/1] mf: Restart transforms and sinks on seek.

[Brendan McGrath]

From: Brendan McGrath <bmcgrath(a)codeweavers.com>

The order should be:
1. Stop sources;
2. Flush MFTs;
3. Start sources;
4. Request output down the chain of sink inputs;
6. Flush sinks; and
7. Start the clock

This takes place whether we pause before we seek or seek without pause.
---
 dlls/mf/session.c  | 190 +++++++++++++++++++++++++++------------------
 dlls/mf/tests/mf.c |   1 -
 2 files changed, 116 insertions(+), 75 deletions(-)

diff --git a/dlls/mf/session.c b/dlls/mf/session.c
index 483ea6f904f..044ae6b6003 100644
--- a/dlls/mf/session.c
+++ b/dlls/mf/session.c
@@ -222,6 +222,7 @@ enum presentation_flags
     SESSION_FLAG_END_OF_PRESENTATION = 0x10,
     SESSION_FLAG_PENDING_RATE_CHANGE = 0x20,
     SESSION_FLAG_PENDING_COMMAND = 0x40,
+    SESSION_FLAG_RESTARTING = 0x80,
 };
 
 struct media_session
@@ -1006,6 +1007,38 @@ static HRESULT session_subscribe_sources(struct media_session *session)
     return hr;
 }
 
+static void session_restart_transforms(struct media_session *session)
+{
+    struct topo_node *node;
+    UINT i;
+
+    LIST_FOR_EACH_ENTRY(node, &session->presentation.nodes, struct topo_node, entry)
+    {
+        if (node->type == MF_TOPOLOGY_TRANSFORM_NODE)
+        {
+            IMFTransform_ProcessMessage(node->object.transform, MFT_MESSAGE_COMMAND_FLUSH, 0);
+            for (i = 0; i < node->u.transform.output_count; i++)
+                node->u.transform.outputs[i].requests = 0; /* these requests might have been flushed */
+        }
+    }
+}
+
+static void session_request_sample(struct media_session *session, IMFStreamSink *sink_stream);
+
+static void session_restart_sinks(struct media_session *session)
+{
+    struct topo_node *node;
+
+    LIST_FOR_EACH_ENTRY(node, &session->presentation.nodes, struct topo_node, entry)
+    {
+        if (node->type == MF_TOPOLOGY_OUTPUT_NODE)
+        {
+            session_request_sample(session, node->object.sink_stream);
+            IMFStreamSink_Flush(node->object.sink_stream);
+        }
+    }
+}
+
 static void session_flush_nodes(struct media_session *session)
 {
     struct topo_node *node;
@@ -1069,89 +1102,85 @@ static void session_start(struct media_session *session, const GUID *time_format
     HRESULT hr;
     UINT i;
 
-    switch (session->state)
+    if (session->presentation.topo_status == MF_TOPOSTATUS_INVALID)
     {
-        case SESSION_STATE_STOPPED:
-
-            /* Start request with no current topology. */
-            if (session->presentation.topo_status == MF_TOPOSTATUS_INVALID)
-            {
-                session_command_complete_with_event(session, MESessionStarted, MF_E_INVALIDREQUEST, NULL);
-                break;
-            }
-
-            /* fallthrough */
-        case SESSION_STATE_PAUSED:
+        session_command_complete_with_event(session, MESessionStarted, MF_E_INVALIDREQUEST, NULL);
+    }
+    else if (((session->state == SESSION_STATE_PAUSED || session->state == SESSION_STATE_STARTED)
+                && IsEqualGUID(time_format, &GUID_NULL) && start_position->vt == VT_EMPTY)
+                || session->state == SESSION_STATE_STOPPED)
+    {
+        session->presentation.time_format = *time_format;
+        session->presentation.start_position.vt = VT_EMPTY;
+        PropVariantCopy(&session->presentation.start_position, start_position);
 
-            session->presentation.time_format = *time_format;
-            session->presentation.start_position.vt = VT_EMPTY;
-            PropVariantCopy(&session->presentation.start_position, start_position);
+        if (FAILED(hr = session_subscribe_sources(session)))
+        {
+            session_handle_start_error(session, hr);
+            return;
+        }
 
-            if (FAILED(hr = session_subscribe_sources(session)))
+        LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry)
+        {
+            if (FAILED(hr = IMFMediaSource_Start(source->source, source->pd, &GUID_NULL, start_position)))
             {
+                WARN("Failed to start media source %p, hr %#lx.\n", source->source, hr);
                 session_handle_start_error(session, hr);
                 return;
             }
+        }
 
-            LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry)
+        LIST_FOR_EACH_ENTRY(topo_node, &session->presentation.nodes, struct topo_node, entry)
+        {
+            if (topo_node->type == MF_TOPOLOGY_TRANSFORM_NODE)
             {
-                if (FAILED(hr = IMFMediaSource_Start(source->source, source->pd, &GUID_NULL, start_position)))
+                for (i = 0; i < topo_node->u.transform.input_count; i++)
                 {
-                    WARN("Failed to start media source %p, hr %#lx.\n", source->source, hr);
-                    session_handle_start_error(session, hr);
-                    return;
+                    struct transform_stream *stream = &topo_node->u.transform.inputs[i];
+                    stream->draining = FALSE;
                 }
             }
+        }
 
-            LIST_FOR_EACH_ENTRY(topo_node, &session->presentation.nodes, struct topo_node, entry)
-            {
-                if (topo_node->type == MF_TOPOLOGY_TRANSFORM_NODE)
-                {
-                    for (i = 0; i < topo_node->u.transform.input_count; i++)
-                    {
-                        struct transform_stream *stream = &topo_node->u.transform.inputs[i];
-                        stream->draining = FALSE;
-                    }
-                }
-            }
+        session->state = SESSION_STATE_STARTING_SOURCES;
+    }
+    else if (session->state == SESSION_STATE_PAUSED || session->state == SESSION_STATE_STARTED)
+    {
 
-            session->state = SESSION_STATE_STARTING_SOURCES;
-            break;
-        case SESSION_STATE_STARTED:
-            /* Check for invalid positions */
-            LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry)
+        /* Check for invalid positions */
+        LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry)
+        {
+            hr = IMFPresentationDescriptor_GetUINT64(source->pd, &MF_PD_DURATION, (UINT64 *)&duration);
+            if (SUCCEEDED(hr) && IsEqualGUID(time_format, &GUID_NULL)
+                    && start_position->vt == VT_I8 && start_position->hVal.QuadPart > duration)
             {
-                hr = IMFPresentationDescriptor_GetUINT64(source->pd, &MF_PD_DURATION, (UINT64 *)&duration);
-                if (SUCCEEDED(hr) && IsEqualGUID(time_format, &GUID_NULL)
-                        && start_position->vt == VT_I8 && start_position->hVal.QuadPart > duration)
-                {
-                    WARN("Start position %s out of range, hr %#lx.\n", wine_dbgstr_longlong(start_position->hVal.QuadPart), hr);
-                    session_command_complete_with_event(session, MESessionStarted, MF_E_INVALID_POSITION, NULL);
-                    return;
-                }
+                WARN("Start position %s out of range, hr %#lx.\n", wine_dbgstr_longlong(start_position->hVal.QuadPart), hr);
+                session_command_complete_with_event(session, MESessionStarted, MF_E_INVALID_POSITION, NULL);
+                return;
             }
+        }
 
-            /* Stop sources */
-            LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry)
+        /* Stop sources */
+        LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry)
+        {
+            if (FAILED(hr = IMFMediaSource_Stop(source->source)))
             {
-                if (FAILED(hr = IMFMediaSource_Stop(source->source)))
-                {
-                    WARN("Failed to stop media source %p, hr %#lx.\n", source->source, hr);
-                    session_command_complete_with_event(session, MESessionStarted, hr, NULL);
-                    return;
-                }
+                WARN("Failed to stop media source %p, hr %#lx.\n", source->source, hr);
+                session_command_complete_with_event(session, MESessionStarted, hr, NULL);
+                return;
             }
+        }
 
-            session->presentation.time_format = *time_format;
-            session->presentation.start_position.vt = VT_EMPTY;
-            PropVariantCopy(&session->presentation.start_position, start_position);
+        session->presentation.time_format = *time_format;
+        session->presentation.start_position.vt = VT_EMPTY;
+        PropVariantCopy(&session->presentation.start_position, start_position);
 
-            /* SESSION_STATE_STARTED -> SESSION_STATE_RESTARTING_SOURCES -> SESSION_STATE_STARTED */
-            session->state = SESSION_STATE_RESTARTING_SOURCES;
-            break;
-        default:
-            session_command_complete_with_event(session, MESessionStarted, MF_E_INVALIDREQUEST, NULL);
-            break;
+        /* SESSION_STATE_STARTED -> SESSION_STATE_RESTARTING_SOURCES -> SESSION_STATE_STARTED */
+        session->state = SESSION_STATE_RESTARTING_SOURCES;
+    }
+    else
+    {
+        session_command_complete_with_event(session, MESessionStarted, MF_E_INVALIDREQUEST, NULL);
     }
 }
 
@@ -3245,15 +3274,6 @@ static void session_set_source_object_state(struct media_session *session, IUnkn
 
             session_set_presentation_clock(session);
 
-            /* If sinks are already started, start session immediately. This can happen when doing a
-             * seek from SESSION_STATE_STARTED */
-            if (session_is_output_nodes_state(session, OBJ_STATE_STARTED)
-                    && SUCCEEDED(session_start_clock(session)))
-            {
-                session_set_started(session);
-                return;
-            }
-
             if ((session->presentation.flags & SESSION_FLAG_NEEDS_PREROLL) && session_is_output_nodes_state(session, OBJ_STATE_STOPPED))
             {
                 MFTIME preroll_time = 0;
@@ -3288,15 +3308,37 @@ static void session_set_source_object_state(struct media_session *session, IUnkn
                 }
                 session->state = SESSION_STATE_PREROLLING_SINKS;
             }
-            else if (SUCCEEDED(session_start_clock(session)))
-                session->state = SESSION_STATE_STARTING_SINKS;
+            else
+            {
+                if (session->presentation.flags & SESSION_FLAG_RESTARTING)
+                {
+                    session->presentation.flags &= ~SESSION_FLAG_RESTARTING;
+                    session_restart_sinks(session);
+                }
+
+                if (SUCCEEDED(hr = session_start_clock(session)))
+                {
+                    /* If sinks are already started, start session immediately. This can happen when doing a
+                     * seek from SESSION_STATE_STARTED (i.e. a seek without pause/stop) */
+                    if (session_is_output_nodes_state(session, OBJ_STATE_STARTED))
+                        session_set_started(session);
+                    else
+                        session->state = SESSION_STATE_STARTING_SINKS;
+                }
+                else
+                {
+                    WARN("Failed to start session clock %p, hr %#lx.\n", session, hr);
+                    session_command_complete_with_event(session, MESessionStarted, hr, NULL);
+                }
+            }
 
             break;
         case SESSION_STATE_RESTARTING_SOURCES:
             if (!session_is_source_nodes_state(session, OBJ_STATE_STOPPED))
                 break;
 
-            session_flush_nodes(session);
+            session->presentation.flags |= SESSION_FLAG_RESTARTING;
+            session_restart_transforms(session);
 
             /* Start sources */
             LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry)
diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c
index 37c1b0be456..958b3385d7b 100644
--- a/dlls/mf/tests/mf.c
+++ b/dlls/mf/tests/mf.c
@@ -6410,7 +6410,6 @@ static void test_media_session_Start(void)
         ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
         hr = wait_media_event(session, callback, MESessionStarted, 5000, &propvar);
         ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
-        todo_wine_if(initial_state == SOURCE_PAUSED)
         compare_object_states(&actual_object_state_record, &expected_object_state_records[initial_state]);
 
         hr = IMFMediaSession_Stop(session);
-- 
GitLab

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

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/mf/session.c:
> +
> +static void session_request_sample(struct media_session *session, IMFStreamSink *sink_stream);
> +
> +static void session_restart_sinks(struct media_session *session)
> +{
> +    struct topo_node *node;
> +
> +    LIST_FOR_EACH_ENTRY(node, &session->presentation.nodes, struct topo_node, entry)
> +    {
> +        if (node->type == MF_TOPOLOGY_OUTPUT_NODE)
> +        {
> +            session_request_sample(session, node->object.sink_stream);
> +            IMFStreamSink_Flush(node->object.sink_stream);
> +        }
> +    }
> +}
> This doesn't look right. Why would we pretend that sink requested anything? I don't see why this would be necessary or different comparing to a regular start sequence.

Also, helpers names are not great, restart is associated with a clock state. Those are more like flush+drop downstream requests.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7932#note_102425

[Brendan McGrath (＠redmcg)]

> This doesn't look right. Why would we pretend that sink requested anything? I don't see why this would be necessary or different comparing to a regular start sequence.

I agree, and my original fix made it the sinks responsibility to request a new sample. But in my testing on Windows, I couldn't see the sink ever make a request for a new sample after the flush. So my assumption was that the design on Windows was that it was the client's responsibility to request a new sample (i.e. if you call flush, you also provide the next sample). But maybe my test was wrong. I'll see if I can write one that I can include in this MR (and I'll then fix the implementation if necessary).
> helpers names are not great

Agree actually. I used restart as they were called from the `SESSION_STATE_RESTARTING_SOURCES` path. But I shouldn't be naming functions based on where they are called from. I'll rename them to `session_flush_sinks` and `session_flush_transforms`.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7932#note_102481