[PATCH 0/1] MR10256: winegstreamer: Do not copy the incorrect buffer.
Ideally we probably want to get a buffer and complete the copying in one operation with the parser mutex locked, but that doesn't look practical. Or, the `wg_parser_stream_get_buffer()` return struct could include the GstBuffer and map info to be released later on the unix side, but it wouldn't make much sense to continue copying the buffer after a flush. Another issue is `offset += advance` occurs even if copying failed because `buffer` is null or, after this patch, because it is new. Perhaps `send_sample` should return S_FALSE in that case so we can elide offset advancement, but I don't know enough about quartz to be sure. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10256
From: Conor McCarthy <cmccarthy@codeweavers.com> It is assumed when calling wg_parser_stream_copy_buffer() the info in stream->map_info is for the buffer being copied. After a dynamic format change flushes stream->buffer, the null check in wg_parser_stream_copy_buffer() is insufficient because a new buffer may arrive before copying occurs. Assertion failure occurs if that buffer is smaller, and in any case, the copied data may be incompatible. This issue has been observed in SHOGUN: Total War Collection and Ys I & II Chronicles. --- dlls/winegstreamer/gst_private.h | 2 +- dlls/winegstreamer/main.c | 3 ++- dlls/winegstreamer/media_source.c | 2 +- dlls/winegstreamer/quartz_parser.c | 2 +- dlls/winegstreamer/unixlib.h | 4 +++- dlls/winegstreamer/wg_parser.c | 6 +++++- dlls/winegstreamer/wm_reader.c | 2 +- 7 files changed, 14 insertions(+), 7 deletions(-) diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index 431e7930208..0ca8c8cfcbc 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -90,7 +90,7 @@ void wg_parser_stream_disable(wg_parser_stream_t stream); bool wg_parser_stream_get_buffer(wg_parser_t parser, wg_parser_stream_t stream, struct wg_parser_buffer *buffer); bool wg_parser_stream_copy_buffer(wg_parser_stream_t stream, - void *data, uint32_t offset, uint32_t size); + uint64_t serial_id, void *data, uint32_t offset, uint32_t size); void wg_parser_stream_release_buffer(wg_parser_stream_t stream); void wg_parser_stream_notify_qos(wg_parser_stream_t stream, bool underflow, double proportion, int64_t diff, uint64_t timestamp); diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index 7ff79bda4ab..bb2b7140fe7 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -329,11 +329,12 @@ bool wg_parser_stream_get_buffer(wg_parser_t parser, wg_parser_stream_t stream, } bool wg_parser_stream_copy_buffer(wg_parser_stream_t stream, - void *data, uint32_t offset, uint32_t size) + uint64_t serial_id, void *data, uint32_t offset, uint32_t size) { struct wg_parser_stream_copy_buffer_params params = { .stream = stream, + .serial_id = serial_id, .data = data, .offset = offset, .size = size, diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 05166d78cdb..ac813228040 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -726,7 +726,7 @@ static HRESULT media_stream_send_sample(struct media_stream *stream, const struc if (FAILED(hr = IMFMediaBuffer_Lock(buffer, &data, NULL, NULL))) goto out; - if (!wg_parser_stream_copy_buffer(stream->wg_stream, data, 0, wg_buffer->size)) + if (!wg_parser_stream_copy_buffer(stream->wg_stream, wg_buffer->serial_id, data, 0, wg_buffer->size)) { hr = S_FALSE; wg_parser_stream_release_buffer(stream->wg_stream); diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c index f09e8b9382a..fcece396585 100644 --- a/dlls/winegstreamer/quartz_parser.c +++ b/dlls/winegstreamer/quartz_parser.c @@ -1257,7 +1257,7 @@ static HRESULT send_sample(struct parser_source *pin, IMediaSample *sample, IMediaSample_GetPointer(sample, &ptr); - if (!wg_parser_stream_copy_buffer(pin->wg_stream, ptr, offset, size)) + if (!wg_parser_stream_copy_buffer(pin->wg_stream, buffer->serial_id, ptr, offset, size)) { /* The GStreamer pin has been flushed. */ return S_OK; diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 5d11934fac2..d801002c8a1 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -198,13 +198,14 @@ struct wg_sample struct wg_parser_buffer { + UINT64 serial_id; /* pts and duration are in 100-nanosecond units. */ UINT64 pts, duration; UINT32 size; UINT32 stream; UINT8 discontinuity, preroll, delta, has_pts, has_duration; }; -C_ASSERT(sizeof(struct wg_parser_buffer) == 32); +C_ASSERT(sizeof(struct wg_parser_buffer) == 40); typedef UINT64 wg_parser_t; typedef UINT64 wg_parser_stream_t; @@ -288,6 +289,7 @@ struct wg_parser_stream_get_buffer_params struct wg_parser_stream_copy_buffer_params { wg_parser_stream_t stream; + UINT64 serial_id; void *data; UINT32 offset; UINT32 size; diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 1a29015356e..7170ce43f8f 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -114,6 +114,7 @@ struct wg_parser_stream pthread_cond_t event_cond, event_empty_cond; GstBuffer *buffer; GstMapInfo map_info; + uint64_t buffer_serial_id; bool flushing, eos, enabled, has_tags, has_buffer, no_more_pads; @@ -367,6 +368,7 @@ static NTSTATUS wg_parser_stream_get_buffer(void *args) * that this will need modification to wg_parser_stream_notify_qos() as * well. */ + wg_buffer->serial_id = stream->buffer_serial_id; if ((wg_buffer->has_pts = GST_BUFFER_PTS_IS_VALID(buffer))) wg_buffer->pts = GST_BUFFER_PTS(buffer) / 100; if ((wg_buffer->has_duration = GST_BUFFER_DURATION_IS_VALID(buffer))) @@ -391,8 +393,9 @@ static NTSTATUS wg_parser_stream_copy_buffer(void *args) pthread_mutex_lock(&parser->mutex); - if (!stream->buffer) + if (!stream->buffer || stream->buffer_serial_id != params->serial_id) { + GST_DEBUG("Ignoring pre-flush copy op."); pthread_mutex_unlock(&parser->mutex); return VFW_E_WRONG_STATE; } @@ -754,6 +757,7 @@ static GstFlowReturn sink_chain_cb(GstPad *pad, GstObject *parent, GstBuffer *bu } stream->buffer = buffer; + ++stream->buffer_serial_id; pthread_mutex_unlock(&parser->mutex); pthread_cond_signal(&stream->event_cond); diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index 4f41f1a1ceb..8cc2680cb4d 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -1802,7 +1802,7 @@ static HRESULT wm_reader_read_stream_sample(struct wm_reader *reader, struct wg_ ERR("Failed to get data pointer, hr %#lx.\n", hr); size = min(buffer->size - stream->current_buffer_offset, capacity); - if (!wg_parser_stream_copy_buffer(stream->wg_stream, data, stream->current_buffer_offset, size)) + if (!wg_parser_stream_copy_buffer(stream->wg_stream, buffer->serial_id, data, stream->current_buffer_offset, size)) { /* The GStreamer pin has been flushed. */ INSSBuffer_Release(*sample); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10256
``` It is assumed when calling wg_parser_stream_copy_buffer() the info in stream->map_info is for the buffer being copied. After a dynamic format change flushes stream->buffer, the null check in wg_parser_stream_copy_buffer() is insufficient because a new buffer may arrive before copying occurs. Assertion failure occurs if that buffer is smaller, and in any case, the copied data may be incompatible. ``` This sounds like a frontend bug. After a dynamic format change, the buffer *will* be wrong, and we shouldn't try to copy it. Looking at the code though, we don't? We do call release_buffer, which is probably wrong. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10256#note_131471
Looking at the code though, we don't? We do call release_buffer, which is probably wrong.
We should probably call release_buffer *then* seek. But it sounds like you're encountering something different. How are we getting to copy_buffer? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10256#note_131473
We call `copy_buffer()` in another thread, quartz_parser `stream_thread()`, where if a flush occurs between `wg_parser_stream_get_buffer()` and `send_buffer()`, we can have a new buffer by the time it reaches `wg_parser_stream_copy_buffer()`. From what I understand, without some form of locking during the calls to `get_buffer()` and `send_buffer()`, there's no guarantee `send_buffer()` will send the buffer whose info was fetched in `get_buffer()`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10256#note_131561
Ah I see the problem, the one stream flushes the other. For normal seeks this isn't a problem of course because we flush all the streams on the frontend side. But we can't do that when seeking from the streaming thread... well, you can, but synchronization makes an already very complicated bit of logic worse. And synchronization mostly isn't a problem here; it's just this specific race. I believe it should be possible to deal with this without needing the frontends. You can store the stream data in the wg_parser_stream. Once you've done that, though, it's easier not to even store an identifier, but simply a flag "have we called wg_parser_stream_get_buffer()", and then reset that flag on release_buffer() but *also* reset it if a seek ends up flushing the buffer. Then copy_buffer() fails if we get there, which isn't even a problem; in fact we already handle it. This is a bit more declarative, I think. get_buffer() / release_buffer() manage a state that isn't synchronized, so if it can be altered between the two calls, the wg backend should handle that and report that a flush interrupted the sequence, so the caller can give up and start over. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10256#note_131777
participants (3)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Elizabeth Figura (＠zfigura)