Streams hold a reference to the source, but this reference should not be taken until `Start()` is called because freeing the media source depends on release in `Shutdown()` of the stream's reference to the source. We could create the streams in `media_source_create()` and take source refs for them in `Start()`, but that's potentially confusing and fragile.
Streams can persist after the media source is destroyed. In that case, they cannot access the source object and it should be released and set to null. Windows behaviour is to release references to the source in `Shutdown()`. If we don't do this and a buggy app were to leak a stream object, the media source object would also leak and `wg_parser_destroy()` would not be called.
-- v7: winegstreamer: Release stream references to the media source in Shutdown(). winegstreamer: Do not create MF stream objects until the media source is started. mfplat/tests: Test video stream release after media source release. mfplat/tests: Add more media source refcount checks.
From: Conor McCarthy cmccarthy@codeweavers.com
Shows that the stream does not hold a reference until Start() is called, and releases it during Shutdown(). --- dlls/mfplat/tests/mfplat.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 0a2471d3849..7b736cd1334 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1383,6 +1383,10 @@ static void test_source_resolver(void) ok(mediasource != NULL, "got %p\n", mediasource); ok(obj_type == MF_OBJECT_MEDIASOURCE, "got %d\n", obj_type);
+ /* Test that no extra refs to mediasource are held if Start() was not called */ + todo_wine + EXPECT_REF(mediasource, 1); + IMFMediaSource_Shutdown(mediasource); refcount = IMFMediaSource_Release(mediasource); ok(!refcount, "Unexpected refcount %ld\n", refcount); @@ -1479,15 +1483,24 @@ static void test_source_resolver(void) ok(mediasource != NULL, "got %p\n", mediasource); ok(obj_type == MF_OBJECT_MEDIASOURCE, "got %d\n", obj_type);
+ todo_wine + EXPECT_REF(mediasource, 1); + check_interface(mediasource, &IID_IMFGetService, TRUE); check_service_interface(mediasource, &MF_RATE_CONTROL_SERVICE, &IID_IMFRateSupport, TRUE);
hr = IMFMediaSource_QueryInterface(mediasource, &IID_IMFGetService, (void**)&get_service); ok(hr == S_OK, "Failed to get service interface, hr %#lx.\n", hr);
+ todo_wine + EXPECT_REF(mediasource, 2); + hr = IMFGetService_GetService(get_service, &MF_RATE_CONTROL_SERVICE, &IID_IMFRateSupport, (void**)&rate_support); ok(hr == S_OK, "Failed to get rate support interface, hr %#lx.\n", hr);
+ todo_wine + EXPECT_REF(mediasource, 3); + hr = IMFRateSupport_GetFastestRate(rate_support, MFRATE_FORWARD, FALSE, &rate); ok(hr == S_OK, "Failed to query fastest rate, hr %#lx.\n", hr); ok(rate == 1e6f, "Unexpected fastest rate %f.\n", rate); @@ -1576,6 +1589,10 @@ static void test_source_resolver(void) hr = IMFMediaSource_Start(mediasource, descriptor, &GUID_NULL, &var); ok(hr == S_OK, "Failed to start media source, hr %#lx.\n", hr);
+ /* The stream holds a reference. It is unclear which object holds the fifth + * reference in Windows, but it's released after MENewStream is retrieved. */ + EXPECT_REF(mediasource, 5); + video_stream = NULL; if (get_event((IMFMediaEventGenerator *)mediasource, MENewStream, &var)) { @@ -1583,6 +1600,8 @@ static void test_source_resolver(void) video_stream = (IMFMediaStream *)var.punkVal; }
+ EXPECT_REF(mediasource, 4); + hr = IMFMediaSource_Pause(mediasource); ok(hr == S_OK, "Failed to pause media source, hr %#lx.\n", hr); if (get_event((IMFMediaEventGenerator *)mediasource, MESourcePaused, &var)) @@ -1691,14 +1710,23 @@ static void test_source_resolver(void) hr = IMFMediaSource_Shutdown(mediasource); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+ /* During shutdown, circular references such as source <-> stream should be released. */ + EXPECT_REF(mediasource, 3); + ok(bytestream_closed, "Missing IMFByteStream::Close call\n");
hr = IMFMediaSource_CreatePresentationDescriptor(mediasource, NULL); ok(hr == MF_E_SHUTDOWN, "Unexpected hr %#lx.\n", hr);
IMFRateSupport_Release(rate_support); + + EXPECT_REF(mediasource, 2); + IMFGetService_Release(get_service); - IMFMediaSource_Release(mediasource); + + refcount = IMFMediaSource_Release(mediasource); + ok(!refcount, "Unexpected refcount %ld\n", refcount); + IMFByteStream_Release(stream);
/* Create directly through scheme handler. */
From: Conor McCarthy cmccarthy@codeweavers.com
--- dlls/mfplat/tests/mfplat.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 7b736cd1334..17ea603d9e2 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1701,7 +1701,6 @@ static void test_source_resolver(void)
get_event((IMFMediaEventGenerator *)mediasource, MEEndOfPresentation, NULL);
- IMFMediaStream_Release(video_stream); IMFMediaTypeHandler_Release(handler); IMFPresentationDescriptor_Release(descriptor);
@@ -1711,6 +1710,7 @@ static void test_source_resolver(void) ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
/* During shutdown, circular references such as source <-> stream should be released. */ + todo_wine EXPECT_REF(mediasource, 3);
ok(bytestream_closed, "Missing IMFByteStream::Close call\n"); @@ -1720,11 +1720,14 @@ static void test_source_resolver(void)
IMFRateSupport_Release(rate_support);
+ todo_wine EXPECT_REF(mediasource, 2);
IMFGetService_Release(get_service);
+ /* Holding a reference to the video stream does not prevent release of the media source. */ refcount = IMFMediaSource_Release(mediasource); + todo_wine ok(!refcount, "Unexpected refcount %ld\n", refcount);
IMFByteStream_Release(stream); @@ -1759,6 +1762,12 @@ static void test_source_resolver(void)
IMFSourceResolver_Release(resolver);
+ hr = IMFMediaStream_GetMediaSource(video_stream, &mediasource); + ok(hr == MF_E_SHUTDOWN, "Unexpected hr %#lx.\n", hr); + + refcount = IMFMediaStream_Release(video_stream); + ok(!refcount, "Unexpected refcount %ld\n", refcount); + hr = MFShutdown(); ok(hr == S_OK, "Failed to shut down, hr %#lx.\n", hr);
From: Conor McCarthy cmccarthy@codeweavers.com
Streams hold a reference to the source, but this reference should not be taken until Start() is called because freeing the media source depends on release in Shutdown() of the stream's reference to the source. We could create the streams in media_source_create() and take source refs for them in Start(), but that's potentially confusing and fragile. --- dlls/mfplat/tests/mfplat.c | 5 +-- dlls/winegstreamer/media_source.c | 56 +++++++++++++++++++++---------- 2 files changed, 40 insertions(+), 21 deletions(-)
diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 17ea603d9e2..7c6147ffd13 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1384,7 +1384,6 @@ static void test_source_resolver(void) ok(obj_type == MF_OBJECT_MEDIASOURCE, "got %d\n", obj_type);
/* Test that no extra refs to mediasource are held if Start() was not called */ - todo_wine EXPECT_REF(mediasource, 1);
IMFMediaSource_Shutdown(mediasource); @@ -1483,7 +1482,6 @@ static void test_source_resolver(void) ok(mediasource != NULL, "got %p\n", mediasource); ok(obj_type == MF_OBJECT_MEDIASOURCE, "got %d\n", obj_type);
- todo_wine EXPECT_REF(mediasource, 1);
check_interface(mediasource, &IID_IMFGetService, TRUE); @@ -1492,13 +1490,11 @@ static void test_source_resolver(void) hr = IMFMediaSource_QueryInterface(mediasource, &IID_IMFGetService, (void**)&get_service); ok(hr == S_OK, "Failed to get service interface, hr %#lx.\n", hr);
- todo_wine EXPECT_REF(mediasource, 2);
hr = IMFGetService_GetService(get_service, &MF_RATE_CONTROL_SERVICE, &IID_IMFRateSupport, (void**)&rate_support); ok(hr == S_OK, "Failed to get rate support interface, hr %#lx.\n", hr);
- todo_wine EXPECT_REF(mediasource, 3);
hr = IMFRateSupport_GetFastestRate(rate_support, MFRATE_FORWARD, FALSE, &rate); @@ -1591,6 +1587,7 @@ static void test_source_resolver(void)
/* The stream holds a reference. It is unclear which object holds the fifth * reference in Windows, but it's released after MENewStream is retrieved. */ + todo_wine EXPECT_REF(mediasource, 5);
video_stream = NULL; diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 05166d78cdb..414f146431c 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -187,6 +187,7 @@ struct media_source UINT64 duration;
IMFStreamDescriptor **descriptors; + wg_parser_stream_t *wg_streams; struct media_stream **streams; ULONG stream_count;
@@ -582,6 +583,9 @@ static HRESULT media_stream_start(struct media_stream *stream, BOOL active, BOOL &GUID_NULL, S_OK, position); }
+static HRESULT media_stream_create(IMFMediaSource *source, IMFStreamDescriptor *descriptor, + wg_parser_stream_t wg_stream, struct media_stream **out); + static HRESULT media_source_start(struct media_source *source, IMFPresentationDescriptor *descriptor, GUID *format, PROPVARIANT *position) { @@ -596,6 +600,21 @@ static HRESULT media_source_start(struct media_source *source, IMFPresentationDe if (source->state == SOURCE_SHUTDOWN) return MF_E_SHUTDOWN;
+ /* if starting for the first time, create the streams */ + if (source->stream_count && !source->streams[0]) + { + assert(source->state == SOURCE_STOPPED); + + for (i = 0; i < source->stream_count; ++i) + { + if (FAILED(hr = media_stream_create(&source->IMFMediaSource_iface, + source->descriptors[i], source->wg_streams[i], &source->streams[i]))) + return hr; + } + free(source->wg_streams); + source->wg_streams = NULL; + } + /* seek to beginning on stop->play */ if (source->state == SOURCE_STOPPED && position->vt == VT_EMPTY) { @@ -1590,11 +1609,15 @@ 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_QueueEventParamVar(stream->event_queue, MEError, &GUID_NULL, MF_E_SHUTDOWN, NULL); - IMFMediaEventQueue_Shutdown(stream->event_queue); - IMFMediaStream_Release(&stream->IMFMediaStream_iface); + if (stream) + { + IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEError, &GUID_NULL, MF_E_SHUTDOWN, NULL); + IMFMediaEventQueue_Shutdown(stream->event_queue); + IMFMediaStream_Release(&stream->IMFMediaStream_iface); + } } free(source->descriptors); + free(source->wg_streams); free(source->streams);
LeaveCriticalSection(&source->cs); @@ -1628,13 +1651,12 @@ static void media_source_init_descriptors(struct media_source *source)
for (i = 0; i < source->stream_count; i++) { - struct media_stream *stream = source->streams[i]; - IMFStreamDescriptor *descriptor = stream->descriptor; + IMFStreamDescriptor *descriptor = source->descriptors[i];
- if (FAILED(hr = stream_descriptor_set_tag(descriptor, stream->wg_stream, + if (FAILED(hr = stream_descriptor_set_tag(descriptor, source->wg_streams[i], &MF_SD_LANGUAGE, WG_PARSER_TAG_LANGUAGE))) WARN("Failed to set stream descriptor language, hr %#lx\n", hr); - if (FAILED(hr = stream_descriptor_set_tag(descriptor, stream->wg_stream, + if (FAILED(hr = stream_descriptor_set_tag(descriptor, source->wg_streams[i], &MF_SD_STREAM_NAME, WG_PARSER_TAG_NAME))) WARN("Failed to set stream descriptor name, hr %#lx\n", hr); } @@ -1687,6 +1709,7 @@ static HRESULT media_source_create(struct object_context *context, IMFMediaSourc stream_count = wg_parser_get_stream_count(parser);
if (!(object->descriptors = calloc(stream_count, sizeof(*object->descriptors))) + || !(object->wg_streams = calloc(stream_count, sizeof(*object->wg_streams))) || !(object->streams = calloc(stream_count, sizeof(*object->streams)))) { hr = E_OUTOFMEMORY; @@ -1695,24 +1718,24 @@ static HRESULT media_source_create(struct object_context *context, IMFMediaSourc
for (i = 0; i < stream_count; ++i) { + /* Each MF stream holds a reference to the source, and that ref should be released + * in Shutdown(). It is valid to create and release an MF source without ever + * calling Start() and Shutdown(), and in that case, if streams take a source ref + * on source creation, the refcount never reaches zero and Shutdown() is never + * called. Therefore, streams are created in media_source_start(). The wg streams + * are needed now to get the format and duration. Their buffer is freed in Start(). */ wg_parser_stream_t wg_stream = wg_parser_get_stream(object->wg_parser, i); IMFStreamDescriptor *descriptor; - struct media_stream *stream; struct wg_format format;
wg_parser_stream_get_current_format(wg_stream, &format); if (FAILED(hr = stream_descriptor_create(i, &format, &descriptor))) goto fail; - if (FAILED(hr = media_stream_create(&object->IMFMediaSource_iface, descriptor, wg_stream, &stream))) - { - IMFStreamDescriptor_Release(descriptor); - goto fail; - }
object->duration = max(object->duration, wg_parser_stream_get_duration(wg_stream)); IMFStreamDescriptor_AddRef(descriptor); object->descriptors[i] = descriptor; - object->streams[i] = stream; + object->wg_streams[i] = wg_stream; object->stream_count++; }
@@ -1726,13 +1749,12 @@ static HRESULT media_source_create(struct object_context *context, IMFMediaSourc fail: WARN("Failed to construct MFMediaSource, hr %#lx.\n", hr);
- while (object->streams && object->stream_count--) + while (object->descriptors && object->stream_count--) { - struct media_stream *stream = object->streams[object->stream_count]; IMFStreamDescriptor_Release(object->descriptors[object->stream_count]); - IMFMediaStream_Release(&stream->IMFMediaStream_iface); } free(object->descriptors); + free(object->wg_streams); free(object->streams);
if (stream_count != UINT_MAX)
From: Conor McCarthy cmccarthy@codeweavers.com
Streams can persist after the media source is destroyed. In that case, they cannot access the source object and it should be released and set to null. Windows behaviour is to release references to the source in Shutdown(). --- dlls/mfplat/tests/mfplat.c | 3 -- dlls/winegstreamer/media_source.c | 51 ++++++++++++++++++++++++++++--- 2 files changed, 46 insertions(+), 8 deletions(-)
diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 7c6147ffd13..a845bfb1487 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1707,7 +1707,6 @@ static void test_source_resolver(void) ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
/* During shutdown, circular references such as source <-> stream should be released. */ - todo_wine EXPECT_REF(mediasource, 3);
ok(bytestream_closed, "Missing IMFByteStream::Close call\n"); @@ -1717,14 +1716,12 @@ static void test_source_resolver(void)
IMFRateSupport_Release(rate_support);
- todo_wine EXPECT_REF(mediasource, 2);
IMFGetService_Release(get_service);
/* Holding a reference to the video stream does not prevent release of the media source. */ refcount = IMFMediaSource_Release(mediasource); - todo_wine ok(!refcount, "Unexpected refcount %ld\n", refcount);
IMFByteStream_Release(stream); diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 414f146431c..a72c7da8334 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -124,6 +124,8 @@ struct media_stream IMFMediaStream IMFMediaStream_iface; LONG ref;
+ SRWLOCK source_lock; + IMFMediaSource *media_source; IMFMediaEventQueue *event_queue; IMFStreamDescriptor *descriptor; @@ -543,6 +545,8 @@ static void flush_token_queue(struct media_stream *stream, BOOL send) IUnknown *op; HRESULT hr;
+ assert(stream->media_source); + if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_REQUEST_SAMPLE, &op))) { struct source_async_command *command = impl_from_async_command_IUnknown(op); @@ -998,7 +1002,11 @@ static ULONG WINAPI media_stream_Release(IMFMediaStream *iface)
if (!ref) { - IMFMediaSource_Release(stream->media_source); + if (stream->media_source) + { + IMFMediaSource_Release(stream->media_source); + stream->media_source = NULL; + } IMFStreamDescriptor_Release(stream->descriptor); IMFMediaEventQueue_Release(stream->event_queue); flush_token_queue(stream, FALSE); @@ -1045,21 +1053,38 @@ static HRESULT WINAPI media_stream_QueueEvent(IMFMediaStream *iface, MediaEventT return IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, event_type, ext_type, hr, value); }
+static struct media_source *media_stream_get_media_source(struct media_stream *stream) +{ + IMFMediaSource *source_iface; + + AcquireSRWLockShared(&stream->source_lock); + if ((source_iface = stream->media_source)) + IMFMediaSource_AddRef(source_iface); + ReleaseSRWLockShared(&stream->source_lock); + + return source_iface ? impl_from_IMFMediaSource(source_iface) : NULL; +} + static HRESULT WINAPI media_stream_GetMediaSource(IMFMediaStream *iface, IMFMediaSource **out) { struct media_stream *stream = impl_from_IMFMediaStream(iface); - struct media_source *source = impl_from_IMFMediaSource(stream->media_source); + struct media_source *source = media_stream_get_media_source(stream); HRESULT hr = S_OK;
TRACE("%p, %p.\n", iface, out);
+ if (!source) + return MF_E_SHUTDOWN; + EnterCriticalSection(&source->cs);
if (source->state == SOURCE_SHUTDOWN) + { + IMFMediaSource_Release(&source->IMFMediaSource_iface); hr = MF_E_SHUTDOWN; + } else { - IMFMediaSource_AddRef(&source->IMFMediaSource_iface); *out = &source->IMFMediaSource_iface; }
@@ -1071,11 +1096,14 @@ static HRESULT WINAPI media_stream_GetMediaSource(IMFMediaStream *iface, IMFMedi static HRESULT WINAPI media_stream_GetStreamDescriptor(IMFMediaStream* iface, IMFStreamDescriptor **descriptor) { struct media_stream *stream = impl_from_IMFMediaStream(iface); - struct media_source *source = impl_from_IMFMediaSource(stream->media_source); + struct media_source *source = media_stream_get_media_source(stream); HRESULT hr = S_OK;
TRACE("%p, %p.\n", iface, descriptor);
+ if (!source) + return MF_E_SHUTDOWN; + EnterCriticalSection(&source->cs);
if (source->state == SOURCE_SHUTDOWN) @@ -1088,18 +1116,22 @@ static HRESULT WINAPI media_stream_GetStreamDescriptor(IMFMediaStream* iface, IM
LeaveCriticalSection(&source->cs);
+ IMFMediaSource_Release(&source->IMFMediaSource_iface); return hr; }
static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown *token) { struct media_stream *stream = impl_from_IMFMediaStream(iface); - struct media_source *source = impl_from_IMFMediaSource(stream->media_source); + struct media_source *source = media_stream_get_media_source(stream); IUnknown *op; HRESULT hr;
TRACE("%p, %p.\n", iface, token);
+ if (!source) + return MF_E_SHUTDOWN; + EnterCriticalSection(&source->cs);
if (source->state == SOURCE_SHUTDOWN) @@ -1122,6 +1154,7 @@ static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown
LeaveCriticalSection(&source->cs);
+ IMFMediaSource_Release(&source->IMFMediaSource_iface); return hr; }
@@ -1153,6 +1186,8 @@ static HRESULT media_stream_create(IMFMediaSource *source, IMFStreamDescriptor * object->IMFMediaStream_iface.lpVtbl = &media_stream_vtbl; object->ref = 1;
+ InitializeSRWLock(&object->source_lock); + if (FAILED(hr = MFCreateEventQueue(&object->event_queue))) { free(object); @@ -1613,6 +1648,12 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) { IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEError, &GUID_NULL, MF_E_SHUTDOWN, NULL); IMFMediaEventQueue_Shutdown(stream->event_queue); + /* Media Foundation documentation says circular references such as + * those between the source and its streams should be released here. */ + IMFMediaSource_Release(stream->media_source); + AcquireSRWLockExclusive(&stream->source_lock); + stream->media_source = NULL; + ReleaseSRWLockExclusive(&stream->source_lock); IMFMediaStream_Release(&stream->IMFMediaStream_iface); } }
On Thu Nov 14 16:11:49 2024 +0000, Rémi Bernon wrote:
This doesn't seem right, you'll be accessing source->cs without holding a reference on the source. There's a race condition if the stream is being disconnected in another thread. You will probably need to introduce a per-stream critical section, to guard the stream source reference. Other stream member accesses would probably need to be guarded as well if we want to be correct.
I fixed this with an SRW lock, because it allows shared access to get a source ref.
Rebased, edited a comment, and tidied up stream creation without changing the effect of the code.
On Wed Nov 20 14:14:17 2024 +0000, Conor McCarthy wrote:
The tests seem to show that Windows 10 is different. Preserving source references in the streams would mean that keeping a stream after releasing the source prevents source destruction until the stream is released, but in the tests the source is destroyed while the stream exists. Perhaps they've changed the way things are done since Win7.
What you're testing is some source implementation, depending on format there might be different implementation, with different approaches.
Nikolay Sivov (@nsivov) commented about dlls/winegstreamer/media_source.c:
return MF_E_SHUTDOWN;
- /* if starting for the first time, create the streams */
- if (source->stream_count && !source->streams[0])
- {
assert(source->state == SOURCE_STOPPED);for (i = 0; i < source->stream_count; ++i){if (FAILED(hr = media_stream_create(&source->IMFMediaSource_iface,source->descriptors[i], source->wg_streams[i], &source->streams[i])))return hr;}free(source->wg_streams);source->wg_streams = NULL;- }
Some sample implementation create only selected streams here.
The idea of creating stream objects on Start() looks fine to me, and some official samples do that. However having tests with exact refcounts, of implementation that depends on format potentially that we don't control, will likely be unstable. Could we instead test for something more important, like having source/stream released properly on shutdown, if that's what we need to fix.
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Nikolay Sivov (＠nsivov)