[PATCH v2 0/4] MR5917: Misc. winegstreamer fixes
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. -- v2: winegstreamer: Handle Gstreamer pipeline flushes gracefully in the media source. winegstreamer: Ignore an assert in wg_parser. winegstreamer: Don't only accept segment events when streams are enabled. https://gitlab.winehq.org/wine/wine/-/merge_requests/5917
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 021aa66ae12..b942273d02b 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -576,24 +576,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
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 b942273d02b..7253013b6a3 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
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
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..030d0c1b9a2 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 = S_FALSE; 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 != S_FALSE) + return hr; + } return media_stream_send_eos(source, stream); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5917
On Mon Jun 24 10:26:51 2024 +0000, Torge Matthies wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/5917/diffs?diff_id=119225&start_sha=cec9c6c8b06b98a3bd0a25d946402874ed4af4d2#d19138de09a66cf3bcf4e698b2f5c81e9daf14a2_534_529) Removed commit from this MR
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5917#note_74217
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5917
I don't know why this was done previously but this just creates a race condition, with no to me apparent benefit.
Probably because I in general didn't consider the case where a stream could be re-enabled mid-flush. Or, alternatively, I assumed without checking that a reconfigure would trigger a new segment (which seems to be generally not the case). I assume that's the race that's being fixed here, although the commit messages don't specify. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5917#note_74242
This merge request was approved by Elizabeth Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5917
participants (4)
-
Elizabeth Figura (＠zfigura) -
Rémi Bernon
-
Torge Matthies
-
Torge Matthies (＠tmatthies)