[PATCH 0/5] MR1384: winegstreamer: Fix some quartz transform issues.
From: Anton Baskanov <baskanov(a)gmail.com> Otherwise, the streaming thread might try to access it while it's being destroyed. --- dlls/winegstreamer/quartz_transform.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/winegstreamer/quartz_transform.c b/dlls/winegstreamer/quartz_transform.c index 6520f697ce4..ccdec9454a6 100644 --- a/dlls/winegstreamer/quartz_transform.c +++ b/dlls/winegstreamer/quartz_transform.c @@ -129,7 +129,9 @@ static HRESULT transform_cleanup_stream(struct strmbase_filter *iface) { IMemAllocator_Decommit(filter->source.pAllocator); + EnterCriticalSection(&filter->filter.stream_cs); wg_transform_destroy(filter->transform); + LeaveCriticalSection(&filter->filter.stream_cs); } return S_OK; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1384
From: Anton Baskanov <baskanov(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53342 --- dlls/winegstreamer/quartz_transform.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/winegstreamer/quartz_transform.c b/dlls/winegstreamer/quartz_transform.c index ccdec9454a6..61044ac78dc 100644 --- a/dlls/winegstreamer/quartz_transform.c +++ b/dlls/winegstreamer/quartz_transform.c @@ -131,6 +131,7 @@ static HRESULT transform_cleanup_stream(struct strmbase_filter *iface) EnterCriticalSection(&filter->filter.stream_cs); wg_transform_destroy(filter->transform); + wg_sample_queue_flush(filter->sample_queue, true); LeaveCriticalSection(&filter->filter.stream_cs); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1384
From: Anton Baskanov <baskanov(a)gmail.com> --- dlls/quartz/tests/mpegsplit.c | 42 +++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/dlls/quartz/tests/mpegsplit.c b/dlls/quartz/tests/mpegsplit.c index b2cced84c68..071b70a7f59 100644 --- a/dlls/quartz/tests/mpegsplit.c +++ b/dlls/quartz/tests/mpegsplit.c @@ -1962,6 +1962,47 @@ static void test_large_file(void) ok(ret, "Failed to delete file, error %lu.\n", GetLastError()); } +static void test_source_allocator(void) +{ + const WCHAR *filename = load_resource(L"test.mp3"); + IBaseFilter *filter = create_mpeg_splitter(); + IFilterGraph2 *graph = connect_input(filter, filename); + ALLOCATOR_PROPERTIES props; + struct testfilter testsink; + IPin *source; + HRESULT hr; + ULONG ref; + DWORD ret; + + testfilter_init(&testsink); + IFilterGraph2_AddFilter(graph, &testsink.filter.IBaseFilter_iface, L"sink"); + IBaseFilter_FindPin(filter, L"Audio", &source); + + hr = IFilterGraph2_ConnectDirect(graph, source, &testsink.sink.pin.IPin_iface, NULL); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + + ok(!!testsink.sink.pAllocator, "Expected an allocator.\n"); + hr = IMemAllocator_GetProperties(testsink.sink.pAllocator, &props); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + todo_wine ok(props.cBuffers == 100, "Got %ld buffers.\n", props.cBuffers); + todo_wine ok(props.cbBuffer == 65541, "Got size %ld.\n", props.cbBuffer); + ok(props.cbAlign == 1, "Got alignment %ld.\n", props.cbAlign); + ok(!props.cbPrefix, "Got prefix %ld.\n", props.cbPrefix); + + IFilterGraph2_Disconnect(graph, source); + IFilterGraph2_Disconnect(graph, &testsink.sink.pin.IPin_iface); + + IPin_Release(source); + ref = IFilterGraph2_Release(graph); + ok(!ref, "Got outstanding refcount %ld.\n", ref); + ref = IBaseFilter_Release(filter); + ok(!ref, "Got outstanding refcount %ld.\n", ref); + ref = IBaseFilter_Release(&testsink.filter.IBaseFilter_iface); + ok(!ref, "Got outstanding refcount %ld.\n", ref); + ret = DeleteFileW(filename); + ok(ret, "Failed to delete file, error %lu.\n", GetLastError()); +} + START_TEST(mpegsplit) { IBaseFilter *filter; @@ -1988,6 +2029,7 @@ START_TEST(mpegsplit) test_seeking(); test_streaming(); test_large_file(); + test_source_allocator(); CoUninitialize(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1384
From: Anton Baskanov <baskanov(a)gmail.com> mpg123audiodec requires at least 3 buffers as it will keep references to the last 2 samples. --- dlls/quartz/tests/mpegsplit.c | 4 ++-- dlls/winegstreamer/quartz_parser.c | 9 ++++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/dlls/quartz/tests/mpegsplit.c b/dlls/quartz/tests/mpegsplit.c index 071b70a7f59..a777215db89 100644 --- a/dlls/quartz/tests/mpegsplit.c +++ b/dlls/quartz/tests/mpegsplit.c @@ -1984,8 +1984,8 @@ static void test_source_allocator(void) ok(!!testsink.sink.pAllocator, "Expected an allocator.\n"); hr = IMemAllocator_GetProperties(testsink.sink.pAllocator, &props); ok(hr == S_OK, "Got hr %#lx.\n", hr); - todo_wine ok(props.cBuffers == 100, "Got %ld buffers.\n", props.cBuffers); - todo_wine ok(props.cbBuffer == 65541, "Got size %ld.\n", props.cbBuffer); + ok(props.cBuffers == 100, "Got %ld buffers.\n", props.cBuffers); + ok(props.cbBuffer == 65541, "Got size %ld.\n", props.cbBuffer); ok(props.cbAlign == 1, "Got alignment %ld.\n", props.cbAlign); ok(!props.cbPrefix, "Got prefix %ld.\n", props.cbPrefix); diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c index 87185312585..e9993e0f51a 100644 --- a/dlls/winegstreamer/quartz_parser.c +++ b/dlls/winegstreamer/quartz_parser.c @@ -1649,6 +1649,7 @@ static HRESULT WINAPI GSTOutPin_DecideBufferSize(struct strmbase_source *iface, IMemAllocator *allocator, ALLOCATOR_PROPERTIES *props) { struct parser_source *pin = impl_source_from_IPin(&iface->pin.IPin_iface); + unsigned int buffer_count = 1; unsigned int buffer_size = 16384; ALLOCATOR_PROPERTIES ret_props; @@ -1664,11 +1665,17 @@ static HRESULT WINAPI GSTOutPin_DecideBufferSize(struct strmbase_source *iface, WAVEFORMATEX *format = (WAVEFORMATEX *)pin->pin.pin.mt.pbFormat; buffer_size = format->nAvgBytesPerSec; } + else if (IsEqualGUID(&pin->pin.pin.mt.subtype, &MEDIASUBTYPE_MPEG1AudioPayload) + || IsEqualGUID(&pin->pin.pin.mt.subtype, &MEDIASUBTYPE_MP3)) + { + buffer_count = 100; + buffer_size = 65541; + } /* We do need to drop any buffers that might have been sent with the old * caps, but this will be handled in parser_init_stream(). */ - props->cBuffers = max(props->cBuffers, 1); + props->cBuffers = max(props->cBuffers, buffer_count); props->cbBuffer = max(props->cbBuffer, buffer_size); props->cbAlign = max(props->cbAlign, 1); return IMemAllocator_SetProperties(allocator, props, &ret_props); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1384
From: Anton Baskanov <baskanov(a)gmail.com> This is required to avoid glitches when seeking, as some formats (e.g. MP3) may use data from previous frames. --- dlls/winegstreamer/unixlib.h | 1 + dlls/winegstreamer/wg_sample.c | 4 ++++ dlls/winegstreamer/wg_transform.c | 4 ++++ 3 files changed, 9 insertions(+) diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index be76d366cae..09aa68eb4cc 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -135,6 +135,7 @@ enum wg_sample_flag WG_SAMPLE_FLAG_HAS_PTS = 2, WG_SAMPLE_FLAG_HAS_DURATION = 4, WG_SAMPLE_FLAG_SYNC_POINT = 8, + WG_SAMPLE_FLAG_DISCONTINUITY = 0x10, }; struct wg_sample diff --git a/dlls/winegstreamer/wg_sample.c b/dlls/winegstreamer/wg_sample.c index f48639a6822..b50a30a4828 100644 --- a/dlls/winegstreamer/wg_sample.c +++ b/dlls/winegstreamer/wg_sample.c @@ -354,6 +354,8 @@ HRESULT wg_transform_push_quartz(struct wg_transform *transform, struct wg_sampl if (IMediaSample_IsSyncPoint(sample->u.quartz.sample) == S_OK) wg_sample->flags |= WG_SAMPLE_FLAG_SYNC_POINT; + if (IMediaSample_IsDiscontinuity(sample->u.quartz.sample) == S_OK) + wg_sample->flags |= WG_SAMPLE_FLAG_DISCONTINUITY; wg_sample_queue_begin_append(queue, wg_sample); hr = wg_transform_push_data(transform, wg_sample); @@ -397,6 +399,8 @@ HRESULT wg_transform_read_quartz(struct wg_transform *transform, struct wg_sampl value = !!(wg_sample->flags & WG_SAMPLE_FLAG_SYNC_POINT); IMediaSample_SetSyncPoint(sample->u.quartz.sample, value); + value = !!(wg_sample->flags & WG_SAMPLE_FLAG_DISCONTINUITY); + IMediaSample_SetDiscontinuity(sample->u.quartz.sample, value); return S_OK; } diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 34b8d25c2e5..a52d7e1f722 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -648,6 +648,8 @@ NTSTATUS wg_transform_push_data(void *args) GST_BUFFER_DURATION(buffer) = sample->duration * 100; if (!(sample->flags & WG_SAMPLE_FLAG_SYNC_POINT)) GST_BUFFER_FLAG_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT); + if (sample->flags & WG_SAMPLE_FLAG_DISCONTINUITY) + GST_BUFFER_FLAG_SET(buffer, GST_BUFFER_FLAG_DISCONT); gst_atomic_queue_push(transform->input_queue, buffer); params->result = S_OK; @@ -781,6 +783,8 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, GstCaps *caps, gsi } if (!GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT)) sample->flags |= WG_SAMPLE_FLAG_SYNC_POINT; + if (GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DISCONT)) + sample->flags |= WG_SAMPLE_FLAG_DISCONTINUITY; if (needs_copy) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1384
Patch 4/5 is a bit questionable, as 100 buffers is clearly overkill. Native uses a custom allocator so it likely doesn't really allocate all these buffers. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1384#note_15979
Rémi Bernon (@rbernon) commented about dlls/winegstreamer/wg_sample.c:
if (IMediaSample_IsSyncPoint(sample->u.quartz.sample) == S_OK) wg_sample->flags |= WG_SAMPLE_FLAG_SYNC_POINT; + if (IMediaSample_IsDiscontinuity(sample->u.quartz.sample) == S_OK)
Would you mind doing the same for MF with the `MFSampleExtension_Discontinuity` attribute? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1384#note_15988
I don't know much about quartz, but from a gut feeling I'd say that the critical section addition is not required. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1384#note_15989
On Wed Nov 16 08:18:47 2022 +0000, Rémi Bernon wrote:
I don't know much about quartz, but from a gut feeling I'd say that the critical section addition is not required. ie: I expect the stream to be ended already on cleanup, or bad things would happen anyway.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1384#note_15990
On Wed Nov 16 08:18:47 2022 +0000, Rémi Bernon wrote:
ie: I expect the stream to be ended already on cleanup, or bad things would happen anyway. The name is probably misleading, but transform_cleanup_stream() is called from IMediaFilter::Stop() in strmbase. The filter graph stops filters beginning from renderers and going upstream, so the streaming thread will still be active when transform_cleanup_stream() is called and until the parser is stopped.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1384#note_16001
On Wed Nov 16 19:13:13 2022 +0000, Anton Baskanov wrote:
Patch 4/5 is a bit questionable, as 100 buffers is clearly overkill. Native uses a custom allocator so it likely doesn't really allocate all these buffers. Yes, if we're not going to use a custom allocator then we probably shouldn't allocate quite so many buffers.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1384#note_16046
On Wed Nov 16 11:35:38 2022 +0000, Anton Baskanov wrote:
The name is probably misleading, but transform_cleanup_stream() is called from IMediaFilter::Stop() in strmbase. The filter graph stops filters beginning from renderers and going upstream, so the streaming thread will still be active when transform_cleanup_stream() is called and until the parser is stopped. The patch is definitely not pretty, but it works, and solving this in a better way is hard and needs more thought.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1384#note_16047
participants (4)
-
Anton Baskanov -
Anton Baskanov (＠baskanov)
-
Rémi Bernon
-
Zebediah Figura (＠zfigura)