[PATCH 0/6] MR5917: Misc. winegstreamer fixes

[Torge Matthies (＠tmatthies)]

Mostly race condition fixes.

"winegstreamer: Do waits for samples on stream-specific work queues." works around a Gstreamer bug that I'll try to write a minimal reproducer for and submit to their bug tracker. For now, doing sample waits actually concurrently works around the problem.

"winegstreamer: Fixate caps in autoplug_continue_cb." so far I've only seen relevant when the source is a uridecodebin (in Proton), but I though it couldn't hurt to upstream it too.

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

[PATCH 2/6] winegstreamer: Ignore an assert in wg_parser.

[Torge Matthies]

From: Torge Matthies <openglfreak(a)googlemail.com>

This gets hit when a wg_parser receives GST_EVENT_FLUSH_START between the wg_parser_stream_get_buffer function return
and the wg_parser_stream_release_buffer call. In this case the NULL buffer can be ignored, it does no harm and there
is no memory leak from this.
---
 dlls/winegstreamer/wg_parser.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c
index 021aa66ae12..93046e14766 100644
--- a/dlls/winegstreamer/wg_parser.c
+++ b/dlls/winegstreamer/wg_parser.c
@@ -412,11 +412,12 @@ static NTSTATUS wg_parser_stream_release_buffer(void *args)
 
     pthread_mutex_lock(&parser->mutex);
 
-    assert(stream->buffer);
-
-    gst_buffer_unmap(stream->buffer, &stream->map_info);
-    gst_buffer_unref(stream->buffer);
-    stream->buffer = NULL;
+    if (stream->buffer)
+    {
+        gst_buffer_unmap(stream->buffer, &stream->map_info);
+        gst_buffer_unref(stream->buffer);
+        stream->buffer = NULL;
+    }
 
     pthread_mutex_unlock(&parser->mutex);
     pthread_cond_signal(&stream->event_empty_cond);
-- 
GitLab


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

[PATCH 1/6] winegstreamer: Fix race between wg_parser_stream_en/disable and GST_EVENT_FLUSH_START/STOP.

[Torge Matthies]

From: Torge Matthies <openglfreak(a)googlemail.com>

---
 dlls/winegstreamer/wg_parser.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c
index f9b76b12f8f..021aa66ae12 100644
--- a/dlls/winegstreamer/wg_parser.c
+++ b/dlls/winegstreamer/wg_parser.c
@@ -608,17 +608,14 @@ static gboolean sink_event_cb(GstPad *pad, GstObject *parent, GstEvent *event)
         case GST_EVENT_FLUSH_START:
             pthread_mutex_lock(&parser->mutex);
 
-            if (stream->enabled)
-            {
-                stream->flushing = true;
-                pthread_cond_signal(&stream->event_empty_cond);
+            stream->flushing = true;
+            pthread_cond_signal(&stream->event_empty_cond);
 
-                if (stream->buffer)
-                {
-                    gst_buffer_unmap(stream->buffer, &stream->map_info);
-                    gst_buffer_unref(stream->buffer);
-                    stream->buffer = NULL;
-                }
+            if (stream->buffer)
+            {
+                gst_buffer_unmap(stream->buffer, &stream->map_info);
+                gst_buffer_unref(stream->buffer);
+                stream->buffer = NULL;
             }
 
             pthread_mutex_unlock(&parser->mutex);
@@ -636,8 +633,7 @@ static gboolean sink_event_cb(GstPad *pad, GstObject *parent, GstEvent *event)
             pthread_mutex_lock(&parser->mutex);
 
             stream->eos = false;
-            if (stream->enabled)
-                stream->flushing = false;
+            stream->flushing = false;
 
             pthread_mutex_unlock(&parser->mutex);
             break;
-- 
GitLab


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

[PATCH 3/6] winegstreamer: Handle Gstreamer pipeline flushes gracefully in the media source.

[Torge Matthies]

From: Torge Matthies <openglfreak(a)googlemail.com>

If a GST_EVENT_FLUSH_START event is received between the calls from PE code to wg_parser_stream_get_buffer
and wg_parser_stream_copy_buffer, the latter function will return an error, resulting in the sample request being
dropped without having delivered a sample.

If wg_parser_stream_copy_buffer returns an error, retry the whole wait_on_sample procedure.

Flushes can be triggered by seek operations.
---
 dlls/winegstreamer/media_source.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c
index 474bc42ae15..ecfa94dcfa1 100644
--- a/dlls/winegstreamer/media_source.c
+++ b/dlls/winegstreamer/media_source.c
@@ -728,6 +728,7 @@ static HRESULT media_stream_send_sample(struct media_stream *stream, const struc
 
     if (!wg_parser_stream_copy_buffer(stream->wg_stream, data, 0, wg_buffer->size))
     {
+        hr = HRESULT_FROM_WIN32(ERROR_RETRY);
         wg_parser_stream_release_buffer(stream->wg_stream);
         IMFMediaBuffer_Unlock(buffer);
         goto out;
@@ -789,8 +790,12 @@ static HRESULT wait_on_sample(struct media_stream *stream, IUnknown *token)
 
     TRACE("%p, %p\n", stream, token);
 
-    if (wg_parser_stream_get_buffer(source->wg_parser, stream->wg_stream, &buffer))
-        return media_stream_send_sample(stream, &buffer, token);
+    while (wg_parser_stream_get_buffer(source->wg_parser, stream->wg_stream, &buffer))
+    {
+        HRESULT hr = media_stream_send_sample(stream, &buffer, token);
+        if (hr != HRESULT_FROM_WIN32(ERROR_RETRY))
+            return hr;
+    }
 
     return media_stream_send_eos(source, stream);
 }
-- 
GitLab


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

[PATCH 6/6] winegstreamer: Fixate caps in autoplug_continue_cb.

[Torge Matthies]

From: Torge Matthies <openglfreak(a)googlemail.com>

Not strictly necessary but avoids a potential assert in Gstreamer code.
---
 dlls/winegstreamer/wg_parser.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c
index 7253013b6a3..9aaba2ac908 100644
--- a/dlls/winegstreamer/wg_parser.c
+++ b/dlls/winegstreamer/wg_parser.c
@@ -526,7 +526,12 @@ static bool parser_no_more_pads(struct wg_parser *parser)
 
 static gboolean autoplug_continue_cb(GstElement * decodebin, GstPad *pad, GstCaps * caps, gpointer user)
 {
-    return !caps_is_compressed(caps);
+    gboolean ret;
+
+    caps = gst_caps_fixate(gst_caps_copy_nth(caps, 0));
+    ret = !caps_is_compressed(caps);
+    gst_caps_unref(caps);
+    return ret;
 }
 
 static GstAutoplugSelectResult autoplug_select_cb(GstElement *bin, GstPad *pad,
-- 
GitLab

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

[PATCH 6/6] winegstreamer: Fixate caps in autoplug_continue_cb.

[Marvin]

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 full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=146569

Your paranoid android.


=== debian11 (build log) ===

error: patch failed: dlls/winegstreamer/media_source.c:728
error: patch failed: dlls/winegstreamer/wg_parser.c:577
Task: Patch failed to apply

=== debian11b (build log) ===

error: patch failed: dlls/winegstreamer/media_source.c:728
error: patch failed: dlls/winegstreamer/wg_parser.c:577
Task: Patch failed to apply

[PATCH 5/6] winegstreamer: Don't only accept segment events when streams are enabled.

[Torge Matthies]

From: Torge Matthies <openglfreak(a)googlemail.com>

I don't know why this was done previously but this just creates a race condition, with no to me apparent benefit.
---
 dlls/winegstreamer/wg_parser.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c
index 93046e14766..7253013b6a3 100644
--- a/dlls/winegstreamer/wg_parser.c
+++ b/dlls/winegstreamer/wg_parser.c
@@ -577,24 +577,20 @@ static gboolean sink_event_cb(GstPad *pad, GstObject *parent, GstEvent *event)
     switch (event->type)
     {
         case GST_EVENT_SEGMENT:
-            pthread_mutex_lock(&parser->mutex);
-            if (stream->enabled)
-            {
-                const GstSegment *segment;
-
-                gst_event_parse_segment(event, &segment);
-
-                if (segment->format != GST_FORMAT_TIME)
-                {
-                    pthread_mutex_unlock(&parser->mutex);
-                    GST_FIXME("Unhandled format \"%s\".", gst_format_get_name(segment->format));
-                    break;
-                }
+        {
+            const GstSegment *segment;
 
-                gst_segment_copy_into(segment, &stream->segment);
+            gst_event_parse_segment(event, &segment);
+            if (segment->format != GST_FORMAT_TIME)
+            {
+                GST_FIXME("Unhandled format \"%s\".", gst_format_get_name(segment->format));
+                break;
             }
+            pthread_mutex_lock(&parser->mutex);
+            gst_segment_copy_into(segment, &stream->segment);
             pthread_mutex_unlock(&parser->mutex);
             break;
+        }
 
         case GST_EVENT_EOS:
             pthread_mutex_lock(&parser->mutex);
-- 
GitLab


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

[PATCH 4/6] winegstreamer: Do waits for samples on stream-specific work queues.

[Torge Matthies]

From: Torge Matthies <openglfreak(a)googlemail.com>

---
 dlls/winegstreamer/media_source.c | 175 +++++++++++++++++++++++++-----
 1 file changed, 145 insertions(+), 30 deletions(-)

diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c
index ecfa94dcfa1..772ac1e8db1 100644
--- a/dlls/winegstreamer/media_source.c
+++ b/dlls/winegstreamer/media_source.c
@@ -119,9 +119,29 @@ static HRESULT object_context_create(DWORD flags, IMFByteStream *stream, const W
     return S_OK;
 }
 
+enum stream_async_op
+{
+    STREAM_ASYNC_REQUEST_SAMPLE,
+};
+
+struct stream_async_command
+{
+    IUnknown IUnknown_iface;
+    LONG refcount;
+    enum stream_async_op op;
+    union
+    {
+        struct
+        {
+            IUnknown *token;
+        } request_sample;
+    } u;
+};
+
 struct media_stream
 {
     IMFMediaStream IMFMediaStream_iface;
+    IMFAsyncCallback async_commands_callback;
     LONG ref;
 
     IMFMediaSource *media_source;
@@ -130,6 +150,8 @@ struct media_stream
 
     wg_parser_stream_t wg_stream;
 
+    DWORD async_commands_queue;
+
     IUnknown **token_queue;
     LONG token_queue_count;
     LONG token_queue_cap;
@@ -144,7 +166,6 @@ enum source_async_op
     SOURCE_ASYNC_START,
     SOURCE_ASYNC_PAUSE,
     SOURCE_ASYNC_STOP,
-    SOURCE_ASYNC_REQUEST_SAMPLE,
 };
 
 struct source_async_command
@@ -160,11 +181,6 @@ struct source_async_command
             GUID format;
             PROPVARIANT position;
         } start;
-        struct
-        {
-            struct media_stream *stream;
-            IUnknown *token;
-        } request_sample;
     } u;
 };
 
@@ -209,6 +225,16 @@ static inline struct media_stream *impl_from_IMFMediaStream(IMFMediaStream *ifac
     return CONTAINING_RECORD(iface, struct media_stream, IMFMediaStream_iface);
 }
 
+static inline struct media_stream *impl_from_stream_async_commands_callback_IMFAsyncCallback(IMFAsyncCallback *iface)
+{
+    return CONTAINING_RECORD(iface, struct media_stream, async_commands_callback);
+}
+
+static inline struct stream_async_command *impl_from_stream_async_command_IUnknown(IUnknown *iface)
+{
+    return CONTAINING_RECORD(iface, struct stream_async_command, IUnknown_iface);
+}
+
 static inline struct media_source *impl_from_IMFMediaSource(IMFMediaSource *iface)
 {
     return CONTAINING_RECORD(iface, struct media_source, IMFMediaSource_iface);
@@ -239,7 +265,7 @@ static inline struct source_async_command *impl_from_async_command_IUnknown(IUnk
     return CONTAINING_RECORD(iface, struct source_async_command, IUnknown_iface);
 }
 
-static HRESULT WINAPI source_async_command_QueryInterface(IUnknown *iface, REFIID riid, void **obj)
+static HRESULT WINAPI async_command_QueryInterface(IUnknown *iface, REFIID riid, void **obj)
 {
     if (IsEqualIID(riid, &IID_IUnknown))
     {
@@ -253,6 +279,52 @@ static HRESULT WINAPI source_async_command_QueryInterface(IUnknown *iface, REFII
     return E_NOINTERFACE;
 }
 
+static ULONG WINAPI stream_async_command_AddRef(IUnknown *iface)
+{
+    struct stream_async_command *command = impl_from_stream_async_command_IUnknown(iface);
+    return InterlockedIncrement(&command->refcount);
+}
+
+static ULONG WINAPI stream_async_command_Release(IUnknown *iface)
+{
+    struct stream_async_command *command = impl_from_stream_async_command_IUnknown(iface);
+    ULONG refcount = InterlockedDecrement(&command->refcount);
+
+    if (!refcount)
+    {
+        if (command->op == STREAM_ASYNC_REQUEST_SAMPLE)
+        {
+            if (command->u.request_sample.token)
+                IUnknown_Release(command->u.request_sample.token);
+        }
+        free(command);
+    }
+
+    return refcount;
+}
+
+static const IUnknownVtbl stream_async_command_vtbl =
+{
+    async_command_QueryInterface,
+    stream_async_command_AddRef,
+    stream_async_command_Release,
+};
+
+static HRESULT stream_create_async_op(enum stream_async_op op, IUnknown **out)
+{
+    struct stream_async_command *command;
+
+    if (!(command = calloc(1, sizeof(*command))))
+        return E_OUTOFMEMORY;
+
+    command->IUnknown_iface.lpVtbl = &stream_async_command_vtbl;
+    command->refcount = 1;
+    command->op = op;
+
+    *out = &command->IUnknown_iface;
+    return S_OK;
+}
+
 static ULONG WINAPI source_async_command_AddRef(IUnknown *iface)
 {
     struct source_async_command *command = impl_from_async_command_IUnknown(iface);
@@ -271,11 +343,6 @@ static ULONG WINAPI source_async_command_Release(IUnknown *iface)
             IMFPresentationDescriptor_Release(command->u.start.descriptor);
             PropVariantClear(&command->u.start.position);
         }
-        else if (command->op == SOURCE_ASYNC_REQUEST_SAMPLE)
-        {
-            if (command->u.request_sample.token)
-                IUnknown_Release(command->u.request_sample.token);
-        }
         free(command);
     }
 
@@ -284,7 +351,7 @@ static ULONG WINAPI source_async_command_Release(IUnknown *iface)
 
 static const IUnknownVtbl source_async_command_vtbl =
 {
-    source_async_command_QueryInterface,
+    async_command_QueryInterface,
     source_async_command_AddRef,
     source_async_command_Release,
 };
@@ -532,7 +599,6 @@ static BOOL enqueue_token(struct media_stream *stream, IUnknown *token)
 
 static void flush_token_queue(struct media_stream *stream, BOOL send)
 {
-    struct media_source *source = impl_from_IMFMediaSource(stream->media_source);
     LONG i;
 
     for (i = 0; i < stream->token_queue_count; i++)
@@ -542,13 +608,12 @@ static void flush_token_queue(struct media_stream *stream, BOOL send)
             IUnknown *op;
             HRESULT hr;
 
-            if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_REQUEST_SAMPLE, &op)))
+            if (SUCCEEDED(hr = stream_create_async_op(STREAM_ASYNC_REQUEST_SAMPLE, &op)))
             {
-                struct source_async_command *command = impl_from_async_command_IUnknown(op);
-                command->u.request_sample.stream = stream;
+                struct stream_async_command *command = impl_from_stream_async_command_IUnknown(op);
                 command->u.request_sample.token = stream->token_queue[i];
 
-                hr = MFPutWorkItem(source->async_commands_queue, &source->async_commands_callback, op);
+                hr = MFPutWorkItem(stream->async_commands_queue, &stream->async_commands_callback, op);
                 IUnknown_Release(op);
             }
             if (FAILED(hr))
@@ -833,12 +898,58 @@ static HRESULT WINAPI source_async_commands_Invoke(IMFAsyncCallback *iface, IMFA
             if (FAILED(hr = media_source_stop(source)))
                 WARN("Failed to stop source %p, hr %#lx\n", source, hr);
             break;
-        case SOURCE_ASYNC_REQUEST_SAMPLE:
+    }
+
+    LeaveCriticalSection(&source->cs);
+
+    IUnknown_Release(state);
+
+    return S_OK;
+}
+
+static const IMFAsyncCallbackVtbl source_async_commands_callback_vtbl =
+{
+    callback_QueryInterface,
+    source_async_commands_callback_AddRef,
+    source_async_commands_callback_Release,
+    callback_GetParameters,
+    source_async_commands_Invoke,
+};
+
+static ULONG WINAPI stream_async_commands_callback_AddRef(IMFAsyncCallback *iface)
+{
+    struct media_stream *stream = impl_from_stream_async_commands_callback_IMFAsyncCallback(iface);
+    return IMFMediaStream_AddRef(&stream->IMFMediaStream_iface);
+}
+
+static ULONG WINAPI stream_async_commands_callback_Release(IMFAsyncCallback *iface)
+{
+    struct media_stream *stream = impl_from_stream_async_commands_callback_IMFAsyncCallback(iface);
+    return IMFMediaStream_Release(&stream->IMFMediaStream_iface);
+}
+
+static HRESULT WINAPI stream_async_commands_Invoke(IMFAsyncCallback *iface, IMFAsyncResult *result)
+{
+    struct media_stream *stream = impl_from_stream_async_commands_callback_IMFAsyncCallback(iface);
+    struct media_source *source = impl_from_IMFMediaSource(stream->media_source);
+    struct stream_async_command *command;
+    IUnknown *state;
+    HRESULT hr;
+
+    if (FAILED(hr = IMFAsyncResult_GetState(result, &state)))
+        return hr;
+
+    EnterCriticalSection(&source->cs);
+
+    command = impl_from_stream_async_command_IUnknown(state);
+    switch (command->op)
+    {
+        case STREAM_ASYNC_REQUEST_SAMPLE:
             if (source->state == SOURCE_PAUSED)
-                enqueue_token(command->u.request_sample.stream, command->u.request_sample.token);
+                enqueue_token(stream, command->u.request_sample.token);
             else if (source->state == SOURCE_RUNNING)
             {
-                if (FAILED(hr = wait_on_sample(command->u.request_sample.stream, command->u.request_sample.token)))
+                if (FAILED(hr = wait_on_sample(stream, command->u.request_sample.token)))
                     WARN("Failed to request sample, hr %#lx\n", hr);
             }
             break;
@@ -851,13 +962,13 @@ static HRESULT WINAPI source_async_commands_Invoke(IMFAsyncCallback *iface, IMFA
     return S_OK;
 }
 
-static const IMFAsyncCallbackVtbl source_async_commands_callback_vtbl =
+static const IMFAsyncCallbackVtbl stream_async_commands_callback_vtbl =
 {
     callback_QueryInterface,
-    source_async_commands_callback_AddRef,
-    source_async_commands_callback_Release,
+    stream_async_commands_callback_AddRef,
+    stream_async_commands_callback_Release,
     callback_GetParameters,
-    source_async_commands_Invoke,
+    stream_async_commands_Invoke,
 };
 
 static DWORD CALLBACK read_thread(void *arg)
@@ -1072,15 +1183,14 @@ static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown
         hr = MF_E_MEDIA_SOURCE_WRONGSTATE;
     else if (stream->eos)
         hr = MF_E_END_OF_STREAM;
-    else if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_REQUEST_SAMPLE, &op)))
+    else if (SUCCEEDED(hr = stream_create_async_op(STREAM_ASYNC_REQUEST_SAMPLE, &op)))
     {
-        struct source_async_command *command = impl_from_async_command_IUnknown(op);
-        command->u.request_sample.stream = stream;
+        struct stream_async_command *command = impl_from_stream_async_command_IUnknown(op);
         if (token)
             IUnknown_AddRef(token);
         command->u.request_sample.token = token;
 
-        hr = MFPutWorkItem(source->async_commands_queue, &source->async_commands_callback, op);
+        hr = MFPutWorkItem(stream->async_commands_queue, &stream->async_commands_callback, op);
         IUnknown_Release(op);
     }
 
@@ -1115,9 +1225,11 @@ static HRESULT media_stream_create(IMFMediaSource *source, IMFStreamDescriptor *
         return E_OUTOFMEMORY;
 
     object->IMFMediaStream_iface.lpVtbl = &media_stream_vtbl;
+    object->async_commands_callback.lpVtbl = &stream_async_commands_callback_vtbl;
     object->ref = 1;
 
-    if (FAILED(hr = MFCreateEventQueue(&object->event_queue)))
+    if (FAILED(hr = MFCreateEventQueue(&object->event_queue)) ||
+        FAILED(hr = MFAllocateWorkQueue(&object->async_commands_queue)))
     {
         free(object);
         return hr;
@@ -1570,6 +1682,7 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface)
         struct media_stream *stream = source->streams[source->stream_count];
         IMFStreamDescriptor_Release(source->descriptors[source->stream_count]);
         IMFMediaEventQueue_Shutdown(stream->event_queue);
+        MFUnlockWorkQueue(stream->async_commands_queue);
         IMFMediaStream_Release(&stream->IMFMediaStream_iface);
     }
     free(source->descriptors);
@@ -1708,6 +1821,8 @@ fail:
     {
         struct media_stream *stream = object->streams[object->stream_count];
         IMFStreamDescriptor_Release(object->descriptors[object->stream_count]);
+        if (stream->async_commands_queue)
+            MFUnlockWorkQueue(stream->async_commands_queue);
         IMFMediaStream_Release(&stream->IMFMediaStream_iface);
     }
     free(object->descriptors);
-- 
GitLab


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

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/winegstreamer/media_source.c:
> if (!wg_parser_stream_copy_buffer(stream->wg_stream, data, 0, wg_buffer->size))
>      {
> +        hr = HRESULT_FROM_WIN32(ERROR_RETRY);

`S_FALSE` looks more appropriate, similar to how the WM reader does it.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5917#note_74199

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/winegstreamer/wg_parser.c:
> pthread_mutex_lock(&parser->mutex);
>
> -    assert(stream->buffer);
> -
> +    if (stream->buffer)
> +    {
>          gst_buffer_unmap(stream->buffer, &stream->map_info);
>          gst_buffer_unref(stream->buffer);
>          stream->buffer = NULL;
> +    }

Maybe instead avoid calling `wg_parser_stream_release_buffer` if `wg_parser_stream_copy_buffer` failed in `media_stream_send_sample`?

Maybe even merging the change into c4c2d23e9c190d94dc9393a56a6293af679f49fb.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5917#note_74200

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/winegstreamer/wg_parser.c:
> static gboolean autoplug_continue_cb(GstElement * decodebin, GstPad *pad, GstCaps * caps, gpointer user)
>  {
> -    return !caps_is_compressed(caps);
> +    gboolean ret;
> +
> +    caps = gst_caps_fixate(gst_caps_copy_nth(caps, 0));
> +    ret = !caps_is_compressed(caps);
> +    gst_caps_unref(caps);
> +    return ret;

I think this could change the behavior and I would leave it aside for now as it's unrelated to the other changes here.

I'm also not sure this is the right fix for this, it only silences the error message.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5917#note_74201

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/winegstreamer/media_source.c:
> return E_OUTOFMEMORY;
>
> object->IMFMediaStream_iface.lpVtbl = &media_stream_vtbl;
> +    object->async_commands_callback.lpVtbl = &stream_async_commands_callback_vtbl;
>      object->ref = 1;
>
> -    if (FAILED(hr = MFCreateEventQueue(&object->event_queue)))
> +    if (FAILED(hr = MFCreateEventQueue(&object->event_queue)) ||
> +        FAILED(hr = MFAllocateWorkQueue(&object->async_commands_queue)))

Is this really necessary? I don't feel very excited with the idea of adding more concurrency here, and I don't think it's supposed to work like that, especially for file-based media sources.

You are working on the network media source right? It may be better to have a separate implementation for it, especially as it might be better to use Win32 HTTP/network APIs for compatibility purposes.

So, I would prefer to leave this change aside for now.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5917#note_74202

[Rémi Bernon]

I would also reorder 56d1d13a871b710a49a7d9825872eff1feaf5429 right after 3a88b3e0c9b87c2710b6f091cb9fb3c2c6ebc5a2, as they seem to be related.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5917#note_74203

[Torge Matthies (＠tmatthies)]

On Mon Jun 24 09:28:31 2024 +0000, Rémi Bernon wrote:
> Maybe instead avoid calling `wg_parser_stream_release_buffer` if
> `wg_parser_stream_copy_buffer` failed in `media_stream_send_sample`?
> Maybe even merging the change into c4c2d23e9c190d94dc9393a56a6293af679f49fb.
> The flush event may arrive between the `wg_parser_stream_copy_buffer` and `wg_parser_stream_release_buffer` calls, which would mean `wg_parser_stream_copy_buffer` succeeds but `wg_parser_stream_release_buffer` still asserts.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5917#note_74207

[Torge Matthies (＠tmatthies)]

On Mon Jun 24 09:28:32 2024 +0000, Rémi Bernon wrote:
> Is this really necessary? I don't feel very excited with the idea of
> adding more concurrency here, and I don't think it's supposed to work
> like that, especially for file-based media sources.
> You are working on the network media source right? It may be better to
> have a separate implementation for it, especially as it might be better
> to use Win32 HTTP/network APIs for compatibility purposes.
> So, I would prefer to leave this change aside for now.
> I was just working with the `uridecodebin`-based media source in Proton, not writing a new network media source.

Adding concurrency here works around a Gstreamer bug, which essentially requires that all (enabled) streams are constantly drained (or none are) to make forward progress. I am trying to write a minimal reproducer for it.

I assumed that having truly asynchronous sample requests would be a desirable thing anyway, but if that is not the case I can leave it out for now.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5917#note_74208

[Rémi Bernon]

On Mon Jun 24 09:46:50 2024 +0000, Torge Matthies wrote:
> I was just working with the `uridecodebin`-based media source in Proton,
> not writing a new network media source.
> Adding concurrency here works around a Gstreamer bug, which essentially
> requires that all (enabled) streams are constantly drained (or none are)
> to make forward progress. I am trying to write a minimal reproducer for it.
> I assumed that having truly asynchronous sample requests would be a
> desirable thing anyway, but if that is not the case I can leave it out
> for now.
> Given that native file-based media sources are just demuxers I think they don't need concurrency.

The way it is implemented right now, with a decoding pipeline and queues is the cause of the issue, and IMO it should be resolved by making the media source closer to native, like I want to do with !3606.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5917#note_74209

[Rémi Bernon]

On Mon Jun 24 09:55:25 2024 +0000, Torge Matthies wrote:
> The flush event may arrive between the `wg_parser_stream_copy_buffer`
> and `wg_parser_stream_release_buffer` calls, which would mean
> `wg_parser_stream_copy_buffer` succeeds but
> `wg_parser_stream_release_buffer` still asserts.
> I see, thanks.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5917#note_74211