[PATCH v4 0/5] MR6783: winegstreamer: Fix a memory/resource leak.
References held by contained stream objects must use an internal refcount, otherwise they prevent release of the media source if Start() and Shutdown() are not called. -- v4: 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. mfplat/tests: Always free media events sent to the callback. https://gitlab.winehq.org/wine/wine/-/merge_requests/6783
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/mfplat/tests/mfplat.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 752be9ea260..40bb12b683b 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -986,12 +986,14 @@ static BOOL get_event(IMFMediaEventGenerator *generator, MediaEventType expected ok(hr == S_OK, "Failed to get value of event, hr %#lx.\n", hr); } + IMFMediaEvent_Release(callback->media_event); break; } + + if (callback->media_event) + IMFMediaEvent_Release(callback->media_event); } - if (callback->media_event) - IMFMediaEvent_Release(callback->media_event); IMFAsyncCallback_Release(&callback->IMFAsyncCallback_iface); return ret; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6783
From: Conor McCarthy <cmccarthy(a)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 40bb12b683b..6535890b6c7 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1321,6 +1321,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); + refcount = IMFMediaSource_Release(mediasource); todo_wine ok(!refcount, "Unexpected refcount %ld\n", refcount); @@ -1409,15 +1413,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); @@ -1506,6 +1519,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)) { @@ -1513,6 +1530,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)) @@ -1621,14 +1640,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. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6783
From: Conor McCarthy <cmccarthy(a)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 6535890b6c7..4774e18db0e 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1631,7 +1631,6 @@ static void test_source_resolver(void) get_event((IMFMediaEventGenerator *)mediasource, MEEndOfPresentation, NULL); - IMFMediaStream_Release(video_stream); IMFMediaTypeHandler_Release(handler); IMFPresentationDescriptor_Release(descriptor); @@ -1641,6 +1640,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"); @@ -1650,11 +1650,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); @@ -1689,6 +1692,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 | 6 +--- dlls/winegstreamer/media_source.c | 58 ++++++++++++++++++++++--------- 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 4774e18db0e..c128f0d22c6 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1322,11 +1322,9 @@ 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); refcount = IMFMediaSource_Release(mediasource); - todo_wine ok(!refcount, "Unexpected refcount %ld\n", refcount); IMFByteStream_Release(stream); @@ -1413,7 +1411,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); @@ -1422,13 +1419,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); @@ -1521,6 +1516,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 030d0c1b9a2..ea0f2597c3b 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,26 @@ 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) + { + wg_parser_stream_t wg_stream = source->wg_streams[i]; + struct media_stream *stream; + + if (FAILED(hr = media_stream_create(&source->IMFMediaSource_iface, + source->descriptors[i], wg_stream, &stream))) + return hr; + + source->streams[i] = stream; + } + free(source->wg_streams); + source->wg_streams = NULL; + } + /* seek to beginning on stop->play */ if (source->state == SOURCE_STOPPED && position->vt == VT_EMPTY) { @@ -1569,10 +1593,14 @@ 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_Shutdown(stream->event_queue); - IMFMediaStream_Release(&stream->IMFMediaStream_iface); + if (stream) + { + IMFMediaEventQueue_Shutdown(stream->event_queue); + IMFMediaStream_Release(&stream->IMFMediaStream_iface); + } } free(source->descriptors); + free(source->wg_streams); free(source->streams); LeaveCriticalSection(&source->cs); @@ -1606,13 +1634,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); } @@ -1665,6 +1692,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; @@ -1673,24 +1701,23 @@ static HRESULT media_source_create(struct object_context *context, IMFMediaSourc for (i = 0; i < stream_count; ++i) { + /* It is valid to create and release a MF source without ever calling Start() and + * Shutdown(). Each MF stream holds a reference to the source, and that ref should + * be released in Shutdown(), so streams are not created here. + * 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++; } @@ -1704,13 +1731,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 | 3 -- dlls/winegstreamer/media_source.c | 58 ++++++++++++++++++++----------- 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index c128f0d22c6..dabc4b1f1bc 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1636,7 +1636,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"); @@ -1646,14 +1645,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 ea0f2597c3b..38062296763 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -543,6 +543,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); @@ -986,7 +988,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); @@ -1036,20 +1042,21 @@ static HRESULT WINAPI media_stream_QueueEvent(IMFMediaStream *iface, MediaEventT 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; HRESULT hr = S_OK; TRACE("%p, %p.\n", iface, out); + if (!stream->media_source) + return MF_E_SHUTDOWN; + + source = impl_from_IMFMediaSource(stream->media_source); + assert(source->state != SOURCE_SHUTDOWN); + EnterCriticalSection(&source->cs); - if (source->state == SOURCE_SHUTDOWN) - hr = MF_E_SHUTDOWN; - else - { - IMFMediaSource_AddRef(&source->IMFMediaSource_iface); - *out = &source->IMFMediaSource_iface; - } + IMFMediaSource_AddRef(&source->IMFMediaSource_iface); + *out = &source->IMFMediaSource_iface; LeaveCriticalSection(&source->cs); @@ -1059,20 +1066,21 @@ 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; HRESULT hr = S_OK; TRACE("%p, %p.\n", iface, descriptor); + if (!stream->media_source) + return MF_E_SHUTDOWN; + + source = impl_from_IMFMediaSource(stream->media_source); + assert(source->state != SOURCE_SHUTDOWN); + EnterCriticalSection(&source->cs); - if (source->state == SOURCE_SHUTDOWN) - hr = MF_E_SHUTDOWN; - else - { - IMFStreamDescriptor_AddRef(stream->descriptor); - *descriptor = stream->descriptor; - } + IMFStreamDescriptor_AddRef(stream->descriptor); + *descriptor = stream->descriptor; LeaveCriticalSection(&source->cs); @@ -1082,17 +1090,21 @@ static HRESULT WINAPI media_stream_GetStreamDescriptor(IMFMediaStream* iface, IM 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; IUnknown *op; HRESULT hr; TRACE("%p, %p.\n", iface, token); + if (!stream->media_source) + return MF_E_SHUTDOWN; + + source = impl_from_IMFMediaSource(stream->media_source); + assert(source->state != SOURCE_SHUTDOWN); + EnterCriticalSection(&source->cs); - if (source->state == SOURCE_SHUTDOWN) - hr = MF_E_SHUTDOWN; - else if (!stream->active) + if (!stream->active) hr = MF_E_MEDIA_SOURCE_WRONGSTATE; else if (stream->eos) hr = MF_E_END_OF_STREAM; @@ -1596,6 +1608,10 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) if (stream) { 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); + stream->media_source = NULL; IMFMediaStream_Release(&stream->IMFMediaStream_iface); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6783
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149590 Your paranoid android. === w8 (32 bit report) === mfplat: mfplat.c:1325: Test failed: Unexpected refcount 4, expected 1. mfplat.c:1328: Test failed: Unexpected refcount 3 mfplat.c:1414: Test failed: Unexpected refcount 4, expected 1. mfplat.c:1422: Test failed: Unexpected refcount 5, expected 2. mfplat.c:1427: Test failed: Unexpected refcount 6, expected 3. mfplat.c:1520: Test failed: Unexpected refcount 8, expected 5. === w8adm (32 bit report) === mfplat: mfplat.c:1414: Test failed: Unexpected refcount 3, expected 1. === w1064v1809 (32 bit report) === mfplat: mfplat.c:1325: Test failed: Unexpected refcount 2, expected 1. === w1064_tsign (32 bit report) === mfplat: mfplat.c:1414: Test failed: Unexpected refcount 4, expected 1. === w1064_adm (64 bit report) === mfplat: mfplat.c:1325: Test failed: Unexpected refcount 4, expected 1. mfplat.c:1414: Test failed: Unexpected refcount 4, expected 1. === w10pro64_ja (64 bit report) === mfplat: mfplat.c:1414: Test failed: Unexpected refcount 4, expected 1.
Changed to delay stream object creation until `Start()`, and also release stream references to the source on `Shutdown()`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6783#note_87010
participants (3)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Marvin