[PATCH v2 0/1] MR7010: mf: Do not clean up a session op if it was submitted to a work queue.
Fixes a regression introduced by 500470029b67a2a07b68e4ce8a134c6819b9b838
-- v2: mf: Do not clean up a session op if it was submitted to a work queue.
From: Conor McCarthy cmccarthy@codeweavers.com
Fixes: 500470029b67a2a07b68e4ce8a134c6819b9b838 --- dlls/mf/session.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/dlls/mf/session.c b/dlls/mf/session.c index cb45fceb714..483ea6f904f 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -56,6 +56,7 @@ struct session_op IUnknown IUnknown_iface; LONG refcount; enum session_command command; + BOOL submitted; union { struct @@ -473,7 +474,10 @@ static HRESULT session_submit_command(struct media_session *session, struct sess if (SUCCEEDED(hr = session_is_shut_down(session))) { if (list_empty(&session->commands) && !(session->presentation.flags & SESSION_FLAG_PENDING_COMMAND)) + { hr = MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &session->commands_callback, &op->IUnknown_iface); + op->submitted = SUCCEEDED(hr); + } if (op->command == SESSION_CMD_SHUTDOWN) list_add_head(&session->commands, &op->entry); else @@ -859,6 +863,11 @@ static void session_clear_command_list(struct media_session *session)
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) + continue; list_remove(&op->entry); IUnknown_Release(&op->IUnknown_iface); } @@ -955,6 +964,7 @@ static void session_command_complete(struct media_session *session) { struct session_op *op; struct list *e; + HRESULT hr;
session->presentation.flags &= ~SESSION_FLAG_PENDING_COMMAND;
@@ -962,7 +972,8 @@ static void session_command_complete(struct media_session *session) if ((e = list_head(&session->commands))) { op = LIST_ENTRY(e, struct session_op, entry); - MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &session->commands_callback, &op->IUnknown_iface); + hr = MFPutWorkItem(MFASYNC_CALLBACK_QUEUE_STANDARD, &session->commands_callback, &op->IUnknown_iface); + op->submitted = SUCCEEDED(hr); } }
@@ -1021,6 +1032,10 @@ static void session_purge_pending_commands(struct media_session *session) if (op->command == SESSION_CMD_CLEAR_TOPOLOGIES || op->command == SESSION_CMD_CLOSE || op->command == SESSION_CMD_SHUTDOWN) continue; + /* Once a command is submitted, the callback becomes responsible + * for removal from the list and release of the ref. */ + if (op->submitted) + continue; list_remove(&op->entry); IUnknown_Release(&op->IUnknown_iface); }
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=150706
Your paranoid android.
=== debian11 (build log) ===
0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins. 0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins. 0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins. 0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins. 0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins. 0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins. 0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins. 0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins. 0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins. 0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins. 0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins. 0138:err:winediag:video_processor_create GStreamer doesn't support video conversion, please install appropriate plugins.
=== debian11b (64 bit WoW report) ===
Report validation errors: d3d11:d3d11 has no test summary line (early exit of the main process?) d3d11:d3d11 has unaccounted for failure messages d3d11:d3d11 has unaccounted for todo messages d3d11:d3d11 has unaccounted for skip messages
Nikolay Sivov (@nsivov) commented about dlls/mf/session.c:
if (op->command == SESSION_CMD_CLEAR_TOPOLOGIES || op->command == SESSION_CMD_CLOSE || op->command == SESSION_CMD_SHUTDOWN) continue;
/* Once a command is submitted, the callback becomes responsible* for removal from the list and release of the ref. */if (op->submitted)continue;
Would it make sense to remove it from commands list right before it's submitted and use a second list for submitted commands? Maybe this will break somewhere, I don't remember all the details of how this works. If all we need is to basically ignore its presence in the list by checking this flag, maybe we could simply remove it from the list in the first place.
If we actually need this flag, I think it should be set before calling MFPutWorkItem(), and then it's more natural to check for it first in purge_pending_commands() before filtering by command types.
If we actually need this flag, I think it should be set before calling MFPutWorkItem(), and then it's more natural to check for it first in purge_pending_commands() before filtering by command types.
We must check for `SESSION_CMD_SET_TOPOLOGY` in `session_purge_pending_commands()` even after the op is submitted, because it ends the purge if it has not been executed. The `submitted` check could be moved above the check for other commands, but the flag is only for cleanup and is not meant to affect purging, so I think it should stay where it is for clarity. The mutex is locked in all cases where it is set or checked.
Would it make sense to remove it from commands list right before it's submitted and use a second list for submitted commands?
The op stays in the list until execution because its presence flags that any new command must not be submitted immediately in `session_submit_command()`. Only one command at a time may be pending in the work queue, so we would need only a single pending op variable instead of a second list. But then in `session_purge_pending_commands()` we would need to check for `SESSION_CMD_SET_TOPOLOGY` in the pending op in addition to the list.
I think it best to limit changes to command submission due to the risk of regression. This commit effectively only changes cleanup responsibility in `session_purge_pending_commands()`.
This merge request was approved by Nikolay Sivov.
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Marvin -
Nikolay Sivov (＠nsivov)