[PATCH v2 0/2] MR1710: mf: Discard end of presentation on IMFMediaSession_Stop.
MR !607 was trying to fix an issue with Life Is Strange Remastered, but although it fixed some race conditions with presentation end, the issue it was trying to fix is still there. The game calls IMFMediaSession_Stop while the presentation is ending, expects that command to quickly execute, interrupting the presentation end and emitting a MESessionStopped event instead of the MESessionEnded. Delaying the Stop command and emitting the MESessionEnded event breaks the game assumptions and it crashes. -- v2: mf: Discard end of presentation on IMFMediaSession_Stop. https://gitlab.winehq.org/wine/wine/-/merge_requests/1710
From: Rémi Bernon <rbernon(a)codeweavers.com> It was introduced in !607 to avoid submitting commands while presentation is ending, although we have SESSION_FLAG_PENDING_COMMAND for that as well. In next commit we will want to preempt the end of presentation with the stop command. --- dlls/mf/session.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index b2371763150..2e1c5c6882c 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -215,7 +215,6 @@ 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 @@ -460,7 +459,7 @@ static HRESULT session_submit_command(struct media_session *session, struct sess EnterCriticalSection(&session->cs); if (SUCCEEDED(hr = session_is_shut_down(session))) { - if (list_empty(&session->commands) && !(session->presentation.flags & SESSION_FLAG_PENDING_COMMAND)) + if (list_empty(&session->commands)) hr = MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &session->commands_callback, &op->IUnknown_iface); if (op->command == SESSION_CMD_SHUTDOWN) list_add_head(&session->commands, &op->entry); @@ -849,8 +848,6 @@ static void session_command_complete(struct media_session *session) struct session_op *op; struct list *e; - session->presentation.flags &= ~SESSION_FLAG_PENDING_COMMAND; - /* Submit next command. */ if ((e = list_head(&session->commands))) { @@ -2524,16 +2521,14 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, EnterCriticalSection(&session->cs); - if (session->presentation.flags & SESSION_FLAG_PENDING_COMMAND) + if ((session->presentation.flags & SESSION_FLAG_END_OF_PRESENTATION)) { - WARN("session %p command is in progress, waiting for it to complete.\n", session); + WARN("session %p command is ending, waiting for it to complete.\n", session); LeaveCriticalSection(&session->cs); return S_OK; } list_remove(&op->entry); - session->presentation.flags |= SESSION_FLAG_PENDING_COMMAND; - switch (op->command) { case SESSION_CMD_CLEAR_TOPOLOGIES: @@ -3521,7 +3516,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_FLAG_PENDING_COMMAND; + session->presentation.flags |= SESSION_FLAG_END_OF_PRESENTATION; IMFMediaEventQueue_QueueEventParamVar(session->event_queue, MEEndOfPresentation, &GUID_NULL, S_OK, NULL); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1710
From: Rémi Bernon <rbernon(a)codeweavers.com> MR !607 was trying to fix an issue with Life Is Strange Remastered, but although it fixed some race conditions with presentation end, the issue it was trying to fix is still there. The game calls IMFMediaSession_Stop while the presentation is ending, expects that command to quickly execute, interrupting the presentation end and emitting a MESessionStopped event instead of the MESessionEnded. Delaying the Stop command and emitting the MESessionEnded event breaks the game assumptions and it crashes. --- dlls/mf/session.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 2e1c5c6882c..5a0a4a07693 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -2521,7 +2521,7 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, EnterCriticalSection(&session->cs); - if ((session->presentation.flags & SESSION_FLAG_END_OF_PRESENTATION)) + if ((session->presentation.flags & SESSION_FLAG_END_OF_PRESENTATION) && op->command != SESSION_CMD_STOP) { WARN("session %p command is ending, waiting for it to complete.\n", session); LeaveCriticalSection(&session->cs); @@ -2545,6 +2545,9 @@ static HRESULT WINAPI session_commands_callback_Invoke(IMFAsyncCallback *iface, session_pause(session); break; case SESSION_CMD_STOP: + if (session->presentation.flags & SESSION_FLAG_END_OF_PRESENTATION) + session_set_topo_status(session, S_OK, MF_TOPOSTATUS_ENDED); + session_clear_end_of_presentation(session); session_stop(session); break; case SESSION_CMD_CLOSE: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1710
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=127262 Your paranoid android. === debian11 (32 bit report) === winhttp: winhttp.c:3543: Test failed: got 10054 winhttp.c:3549: Test failed: got 12150 winhttp.c:3550: Test failed: got 3735928559 winhttp.c:3553: Test failed: got 8 winhttp.c:3572: Test failed: got 6 winhttp.c:3577: Test failed: got 6 winhttp.c:3582: Test failed: got 6 winhttp.c:3583: Test failed: got 6 winhttp.c:3588: Test failed: got 6 winhttp.c:3593: Test failed: got 6 winhttp.c:3598: Test failed: got 6 winhttp.c:3603: Test failed: got 6 winhttp.c:3609: Test failed: got 6 winhttp.c:3610: Test failed: got winhttp.c:3611: Test failed: got 0 winhttp.c:3612: Test failed: got 3735928559 winhttp.c:3621: Test failed: got 6 winhttp.c:3637: Test failed: got 6 winhttp.c:3638: Test failed: got winhttp.c:3639: Test failed: got 0 winhttp.c:3640: Test failed: got 3735928559 winhttp.c:3646: Test failed: got 6 winhttp.c:3647: Test failed: got winhttp.c:3648: Test failed: got 0 winhttp.c:3649: Test failed: got 3735928559 winhttp.c:3659: Test failed: got 6 winhttp.c:3669: Test failed: got 6 winhttp.c:3688: Test failed: got 6 winhttp.c:3689: Test failed: got 3735928559 winhttp.c:3698: Test failed: got 6 winhttp.c:3699: Test failed: got 57005 winhttp.c:3700: Test failed: got 3735928559
Does this change too much? Instead of letting stop command pass through pending check, all commands know won't wait for completion of currently executing command. For example doing start -> pause won't have END_OF_PRESENTATION set, and I don't think it should try to pause before pipeline completed transition. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1710#note_18962
On Wed Dec 7 09:50:52 2022 +0000, Nikolay Sivov wrote:
Does this change too much? Instead of letting stop command pass through pending check, all commands know won't wait for completion of currently executing command. For example doing start -> pause won't have END_OF_PRESENTATION set, and I don't think it should try to pause before pipeline completed transition. I think the normal commands are still executed one after another? The `session_submit_command` still checks whether the command list is empty before queueing the command to the thread pool. If it's not empty it means another command is either pending for execution, or waiting on `END_OF_PRESENTATION` to complete.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1710#note_18973
On Wed Dec 7 09:50:52 2022 +0000, Rémi Bernon wrote:
I think the normal commands are still executed one after another? The `session_submit_command` still checks whether the command list is empty before queueing the command to the thread pool. If it's not empty it means another command is either pending for execution, or waiting on `END_OF_PRESENTATION` to complete. Oh right, we're removing commands when they start executing... Sorry, I'll change that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1710#note_18974
On Wed Dec 7 09:51:40 2022 +0000, Rémi Bernon wrote:
Oh right, we're removing commands when they start executing... Sorry, I'll change that. I would keep the flags, but add some special handling for stop command only. Maybe that would be a smaller change.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1710#note_18975
On Wed Dec 7 09:55:45 2022 +0000, Nikolay Sivov wrote:
I would keep the flags, but add some special handling for stop command only. Maybe that would be a smaller change. Yes, thanks for spotting the problem I just can't wrap my head around this, async is hard.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1710#note_18976
participants (3)
-
Marvin -
Nikolay Sivov (＠nsivov) -
Rémi Bernon