From: Anton Baskanov baskanov@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;
From: Anton Baskanov baskanov@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); }
From: Anton Baskanov baskanov@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(); }
From: Anton Baskanov baskanov@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);
From: Anton Baskanov baskanov@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) {
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.
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?
I don't know much about quartz, but from a gut feeling I'd say that the critical section addition is not required.
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.
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.
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.
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.
-
Anton Baskanov -
Anton Baskanov (＠baskanov)
-
Rémi Bernon (＠rbernon)
-
Zebediah Figura (＠zfigura)