[PATCH v6 0/8] MR7466: mf: Fix some issues with seeking.
This fixes failure to play the prologue video in Planet of the Apes: Last Frontier, which unpauses by calling Start() with a location. The exact state leading to this issue does not occur in the Start() tests, and it's not clear how to reproduce it reliably. -- v6: mf: Reset transform nodes when seeking. mf: Initialise the grabber sample count when setting state after a seek. mf: Set pending requests for transform outputs to zero on flush. mf: Reset transform node outputs when seeking. mf: Flush nodes before restarting a paused source at a specific position. mf: Drop transform node input events when unpausing at a specific position. mf: Reset transforms before restarting a paused source instead of after. mf: Move transform node reset to a helper function. https://gitlab.winehq.org/wine/wine/-/merge_requests/7466
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/mf/session.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 483ea6f904f..e595cadd185 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -1061,13 +1061,29 @@ static void session_handle_start_error(struct media_session *session, HRESULT hr session_command_complete_with_event(session, MESessionStarted, hr, NULL); } +static void session_reset_transforms(struct media_session *session) +{ + struct topo_node *topo_node; + UINT i; + + LIST_FOR_EACH_ENTRY(topo_node, &session->presentation.nodes, struct topo_node, entry) + { + if (topo_node->type != MF_TOPOLOGY_TRANSFORM_NODE) + continue; + + for (i = 0; i < topo_node->u.transform.input_count; i++) + { + struct transform_stream *stream = &topo_node->u.transform.inputs[i]; + stream->draining = FALSE; + } + } +} + static void session_start(struct media_session *session, const GUID *time_format, const PROPVARIANT *start_position) { struct media_source *source; - struct topo_node *topo_node; MFTIME duration; HRESULT hr; - UINT i; switch (session->state) { @@ -1103,17 +1119,7 @@ static void session_start(struct media_session *session, const GUID *time_format } } - 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_reset_transforms(session); session->state = SESSION_STATE_STARTING_SOURCES; break; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7466
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/mf/session.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index e595cadd185..d9c44cef934 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -1109,6 +1109,8 @@ static void session_start(struct media_session *session, const GUID *time_format return; } + session_reset_transforms(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))) @@ -1119,8 +1121,6 @@ static void session_start(struct media_session *session, const GUID *time_format } } - session_reset_transforms(session); - session->state = SESSION_STATE_STARTING_SOURCES; break; case SESSION_STATE_STARTED: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7466
From: Conor McCarthy <cmccarthy(a)codeweavers.com> The start position is empty when starting at the current position from the paused state, but if it is not empty, this is a seek operation. --- dlls/mf/session.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index d9c44cef934..66551e146a7 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -1061,7 +1061,7 @@ static void session_handle_start_error(struct media_session *session, HRESULT hr session_command_complete_with_event(session, MESessionStarted, hr, NULL); } -static void session_reset_transforms(struct media_session *session) +static void session_reset_transforms(struct media_session *session, BOOL drop) { struct topo_node *topo_node; UINT i; @@ -1075,6 +1075,8 @@ static void session_reset_transforms(struct media_session *session) { struct transform_stream *stream = &topo_node->u.transform.inputs[i]; stream->draining = FALSE; + if (drop) + transform_stream_drop_events(stream); } } } @@ -1082,6 +1084,7 @@ static void session_reset_transforms(struct media_session *session) static void session_start(struct media_session *session, const GUID *time_format, const PROPVARIANT *start_position) { struct media_source *source; + BOOL unpause_seek; MFTIME duration; HRESULT hr; @@ -1109,7 +1112,8 @@ static void session_start(struct media_session *session, const GUID *time_format return; } - session_reset_transforms(session); + unpause_seek = start_position->vt == VT_I8; + session_reset_transforms(session, unpause_seek); LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7466
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Behaviour in Windows is to send MFT_MESSAGE_COMMAND_FLUSH before calling Start() on the source. --- dlls/mf/session.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 66551e146a7..0103d7a65df 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -1113,6 +1113,8 @@ static void session_start(struct media_session *session, const GUID *time_format } unpause_seek = start_position->vt == VT_I8; + if (unpause_seek) + session_flush_nodes(session); session_reset_transforms(session, unpause_seek); LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7466
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/mf/session.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 0103d7a65df..423b7964d85 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -1078,6 +1078,15 @@ static void session_reset_transforms(struct media_session *session, BOOL drop) if (drop) transform_stream_drop_events(stream); } + + if (!drop) + continue; + + for (i = 0; i < topo_node->u.transform.output_count; ++i) + { + struct transform_stream *stream = &topo_node->u.transform.outputs[i]; + transform_stream_drop_events(stream); + } } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7466
From: Brendan McGrath <bmcgrath(a)codeweavers.com> --- dlls/mf/session.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 423b7964d85..2f53ed1838c 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -1009,13 +1009,18 @@ static HRESULT session_subscribe_sources(struct media_session *session) static void session_flush_nodes(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_OUTPUT_NODE) IMFStreamSink_Flush(node->object.sink_stream); else 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 */ + } } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7466
From: Conor McCarthy <cmccarthy(a)codeweavers.com> If sample_count is zero in this case, we end up with no samples being requested. --- dlls/mf/samplegrabber.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dlls/mf/samplegrabber.c b/dlls/mf/samplegrabber.c index de599139736..4d54afc1d31 100644 --- a/dlls/mf/samplegrabber.c +++ b/dlls/mf/samplegrabber.c @@ -1195,6 +1195,10 @@ static HRESULT sample_grabber_set_state(struct sample_grabber *grabber, enum sin if (state == SINK_STATE_RUNNING && grabber->state != SINK_STATE_RUNNING) { + /* Unpause at a position is a seek operation which drops everything pending. */ + if (grabber->state == SINK_STATE_PAUSED && offset != PRESENTATION_CURRENT_POSITION) + grabber->sample_count = MAX_SAMPLE_QUEUE_LENGTH; + /* Every transition to running state sends a bunch requests to build up initial queue. */ for (i = 0; i < grabber->sample_count; ++i) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7466
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/mf/session.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 2f53ed1838c..3e12f0642a4 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -1168,6 +1168,8 @@ static void session_start(struct media_session *session, const GUID *time_format } } + session_reset_transforms(session, TRUE); + session->presentation.time_format = *time_format; session->presentation.start_position.vt = VT_EMPTY; PropVariantCopy(&session->presentation.start_position, start_position); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7466
I have another change in that branch that resets `stream->requests` to zero in `session_flush_nodes`
That makes sense. Updated this to include your commit instead of setting `stream->requests` in `session_reset_transforms()`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7466#note_100874
Nikolay Sivov (@nsivov) commented about dlls/mf/samplegrabber.c:
if (state == SINK_STATE_RUNNING && grabber->state != SINK_STATE_RUNNING) { + /* Unpause at a position is a seek operation which drops everything pending. */ + if (grabber->state == SINK_STATE_PAUSED && offset != PRESENTATION_CURRENT_POSITION) + grabber->sample_count = MAX_SAMPLE_QUEUE_LENGTH; + /* Every transition to running state sends a bunch requests to build up initial queue. */ for (i = 0; i < grabber->sample_count; ++i) {
This looks a bit messed up I think - first we reset this to max length when stopping, then when start-from-pause case only to execute this loop and reset it back to zero. But I don't see a cleaner way right away. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7466#note_100910
Nikolay Sivov (@nsivov) commented about dlls/mf/session.c:
} }
- session_reset_transforms(session); - session->state = SESSION_STATE_STARTING_SOURCES; break;
Why would moving this make any difference? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7466#note_100911
What bothers me about seeking changes is that it looks like patching pieces as they fail, to make transitions work. And there is no way to actually tell what is correct. The way I did it initially for other states was to create a full stub branch of source->transform->sink, let it run on empty samples, and then issue more commands to see what needs to happen. I don't see how this could be reviewed without doing something similar, especially if it's difficult to reproduce with a proper test. How did you test individual changes, apart from testing the game with the whole set? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7466#note_100913
On Tue Apr 15 04:37:39 2025 +0000, Conor McCarthy wrote:
I have another change in that branch that resets `stream->requests` to zero in `session_flush_nodes` That makes sense. Updated this to include your commit instead of setting `stream->requests` in `session_reset_transforms()`. FYI - I have raised MR !7932, which overlaps a little bit with this one. My MR however doesn't touch the Sample Grabber for example, or include a call to `transform_stream_drop_events`, so it doesn't completely overlap.
I wrote that MR to fix a number of seeking issues within VRChat. But I found when I merged my MR in to Proton (keeping your Sample Grabber changes) it also seems to fix the intro video in Planet of the Apes: Last Frontier. But I guess we'll need to co-ordinate these two MRs. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7466#note_104164
participants (5)
-
Brendan McGrath -
Brendan McGrath (＠redmcg)
-
Conor McCarthy
-
Conor McCarthy (＠cmccarthy)
-
Nikolay Sivov (＠nsivov)