[PATCH 0/1] MR9937: winegstreamer: Protect wg_transform drain and flush by mutex.

[Charlotte Pabst (＠CharlottePabst)]

When flushing and draining at the same time from different threads without synchronization, one of them may fail and in the worst case, both fail, and the pipeline gets permanently stuck in EOS state.

I'm dealing with a game that drains the Video Decoder MFT immediately after sending a stop command to the media session. In some cases, after the session is started again, the decoder does not output any samples anymore.

The simple instance of this bug (with only one of them failing) stems from either drain-related events being pushed while the src pad is flushing, or flush-related events being pushed while the src pad is EOS. This does not cause any problems in the application.

In the more complex case, the flush-stop event is processed by some pads (src in particular) but not yet the entire pipeline when a context switch happens, meaning that when the draining thread pushes its EOS event, it fails at a later pad (complete_drain then also fails), but is still stored as sticky in the src pad. After flush-stop finishes propagating, the stored EOS event then also propagates, leaving all pads in EOS state. This is the main problem I'm concerned about.

I'm attaching `GST_DEBUG="4,WINE:9,GST_PADS:6,GST_EVENT:6"` logs for the EOS bug, as well as a test program. Note that the EOS case is comparatively rare.

I'm unsure if this is the best fix for this issue, feel free to suggest something different.

[flush-drain.c](/uploads/4e48cdea79a32867974c49b0aa2637c6/flush-drain.c)
[flush-drain-bug.log](/uploads/9e873f0e807ecc665565a152bbade9d7/flush-drain-bug.log)

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

[PATCH 1/1] winegstreamer: Protect wg_transform drain and flush by mutex.

[Charlotte Pabst]

From: Charlotte Pabst <[e-mail]>

When flushing and draining at the same time from different threads without
synchronization, one of them may fail and in the worst case, both fail,
and the pipeline gets permanently stuck in EOS state.
---
 dlls/winegstreamer/wg_transform.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c
index f8bf4474756..32fc6d72d16 100644
--- a/dlls/winegstreamer/wg_transform.c
+++ b/dlls/winegstreamer/wg_transform.c
@@ -95,6 +95,7 @@ struct wg_transform
     GstCaps *output_caps;
     GstCaps *input_caps;
 
+    pthread_mutex_t mutex;
     bool draining;
     INT64 ts_offset;
 };
@@ -485,6 +486,7 @@ NTSTATUS wg_transform_destroy(void *args)
     gst_caps_unref(transform->output_caps);
     gst_caps_unref(transform->input_caps);
     gst_atomic_queue_unref(transform->output_queue);
+    pthread_mutex_destroy(&transform->mutex);
     free(transform);
 
     return STATUS_SUCCESS;
@@ -756,6 +758,8 @@ NTSTATUS wg_transform_create(void *args)
             || !push_event(transform->my_src, event))
         goto out;
 
+    pthread_mutex_init(&transform->mutex, NULL);
+
     GST_INFO("Created winegstreamer transform %p.", transform);
     params->transform = (wg_transform_t)(ULONG_PTR)transform;
     return STATUS_SUCCESS;
@@ -1170,6 +1174,13 @@ error:
     return STATUS_UNSUCCESSFUL;
 }
 
+static NTSTATUS drain_transform(struct wg_transform *transform)
+{
+    GST_LOG("transform %p, draining %d buffers", transform, gst_atomic_queue_length(transform->input_queue));
+    transform->draining = true;
+    return complete_drain(transform);
+}
+
 static bool get_transform_output(struct wg_transform *transform, struct wg_sample *sample)
 {
     GstFlowReturn ret;
@@ -1312,12 +1323,13 @@ NTSTATUS wg_transform_get_status(void *args)
 NTSTATUS wg_transform_drain(void *args)
 {
     struct wg_transform *transform = get_transform(*(wg_transform_t *)args);
+    NTSTATUS status;
 
-    GST_LOG("transform %p, draining %d buffers", transform, gst_atomic_queue_length(transform->input_queue));
-
-    transform->draining = true;
+    pthread_mutex_lock(&transform->mutex);
+    status = drain_transform(transform);
+    pthread_mutex_unlock(&transform->mutex);
 
-    return complete_drain(transform);
+    return status;
 }
 
 NTSTATUS wg_transform_flush(void *args)
@@ -1328,6 +1340,7 @@ NTSTATUS wg_transform_flush(void *args)
     GstEvent *event;
     NTSTATUS status;
 
+    pthread_mutex_lock(&transform->mutex);
     GST_LOG("transform %p", transform);
 
     /* this ensures no messages are travelling through the pipeline whilst we flush */
@@ -1343,16 +1356,19 @@ NTSTATUS wg_transform_flush(void *args)
     event = gst_event_new_flush_stop(true);
     gst_pad_push_event(transform->my_src, event);
 
-    if ((status = wg_transform_drain(args)))
-        return status;
+    if ((status = drain_transform(transform)))
+        goto out;
 
     while ((sample = gst_atomic_queue_pop(transform->output_queue)))
         gst_sample_unref(sample);
     if ((sample = transform->output_sample))
         gst_sample_unref(sample);
     transform->output_sample = NULL;
+    status = STATUS_SUCCESS;
 
-    return STATUS_SUCCESS;
+out:
+    pthread_mutex_unlock(&transform->mutex);
+    return status;
 }
 
 NTSTATUS wg_transform_notify_qos(void *args)
-- 
GitLab

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

[Rémi Bernon (＠rbernon)]

This is perhaps that the IMFTransform are supposed to be thread safe (according to https://learn.microsoft.com/en-us/windows/win32/medfound/media-foundation-an...). We avoided that issue out of convenience and before we have many transforms and adding CS to each of them is annoying, and unnecessary until now as they seemed to be generally used from a single thread, but perhaps this could be factored with an aggregating wrapper.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/9937#note_127412