[PATCH v8 0/4] MR6783: winegstreamer: Fix memory/resource leaks in media sources.
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. -- v8: 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. https://gitlab.winehq.org/wine/wine/-/merge_requests/6783
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Shows that the source is freed even if Shutdown() is not explicitly called. --- dlls/mfplat/tests/mfplat.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 0a2471d3849..6c1c8fa01aa 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1383,8 +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); - IMFMediaSource_Shutdown(mediasource); + /* Release without calling Shutdown(). In this case, Shutdown() should be called internally when + * releasing the last ref, which will release any references held by contained media streams. */ refcount = IMFMediaSource_Release(mediasource); + todo_wine ok(!refcount, "Unexpected refcount %ld\n", refcount); IMFByteStream_Release(stream); @@ -1698,7 +1700,10 @@ static void test_source_resolver(void) IMFRateSupport_Release(rate_support); 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. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6783
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/mfplat/tests/mfplat.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 6c1c8fa01aa..81ae06f1fdd 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1684,7 +1684,6 @@ static void test_source_resolver(void) get_event((IMFMediaEventGenerator *)mediasource, MEEndOfPresentation, NULL); - IMFMediaStream_Release(video_stream); IMFMediaTypeHandler_Release(handler); IMFPresentationDescriptor_Release(descriptor); @@ -1701,7 +1700,9 @@ static void test_source_resolver(void) IMFRateSupport_Release(rate_support); 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); @@ -1736,6 +1737,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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6783
From: Conor McCarthy <cmccarthy(a)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 | 1 - dlls/winegstreamer/media_source.c | 56 +++++++++++++++++++++---------- 2 files changed, 39 insertions(+), 18 deletions(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 81ae06f1fdd..49c62e5e5be 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1386,7 +1386,6 @@ static void test_source_resolver(void) /* Release without calling Shutdown(). In this case, Shutdown() should be called internally when * releasing the last ref, which will release any references held by contained media streams. */ 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 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) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6783
From: Conor McCarthy <cmccarthy(a)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 | 1 - dlls/winegstreamer/media_source.c | 51 ++++++++++++++++++++++++++++--- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 49c62e5e5be..f058536640a 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1701,7 +1701,6 @@ static void test_source_resolver(void) /* 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); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6783
On Thu Jul 24 09:55:04 2025 +0000, Nikolay Sivov wrote:
Some sample implementation create only selected streams here. I left this unchanged, because wg streams are all created with the wg parser, and we still need to call `wg_parser_stream_disable()` on unselected streams even if we haven't created a `struct media_stream` for them, so it would be a bit untidy.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6783#note_110972
On Fri Jul 25 01:21:11 2025 +0000, Nikolay Sivov wrote:
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. Done.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6783#note_110971
participants (2)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)