[PATCH v3 0/2] 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. -- v3: winegstreamer: Introduce an internal refcount in media source objects. mfplat/tests: Test the refcount before the media source is released. https://gitlab.winehq.org/wine/wine/-/merge_requests/6783
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Shows that any media source references held by contained objects do not affect the externally visible refcount. --- dlls/mfplat/tests/mfplat.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 752be9ea260..753b208ee9a 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1319,6 +1319,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 contained streams do not hold externally visible refs to mediasource */ + todo_wine + EXPECT_REF(mediasource, 1); + refcount = IMFMediaSource_Release(mediasource); todo_wine ok(!refcount, "Unexpected refcount %ld\n", refcount); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6783
From: Conor McCarthy <cmccarthy(a)codeweavers.com> 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. --- dlls/mfplat/tests/mfplat.c | 2 -- dlls/winegstreamer/media_source.c | 34 +++++++++++++++++++++++-------- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 753b208ee9a..9ae8f0a5a4b 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -1320,11 +1320,9 @@ static void test_source_resolver(void) ok(obj_type == MF_OBJECT_MEDIASOURCE, "got %d\n", obj_type); /* test that contained streams do not hold externally visible refs to mediasource */ - todo_wine EXPECT_REF(mediasource, 1); 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 030d0c1b9a2..a10731a563d 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -176,6 +176,7 @@ struct media_source IMFRateControl IMFRateControl_iface; IMFAsyncCallback async_commands_callback; LONG ref; + LONG internal_ref; DWORD async_commands_queue; IMFMediaEventQueue *event_queue; IMFByteStream *byte_stream; @@ -239,6 +240,27 @@ static inline struct source_async_command *impl_from_async_command_IUnknown(IUnk return CONTAINING_RECORD(iface, struct source_async_command, IUnknown_iface); } +static void media_source_internal_addref(IMFMediaSource *iface) +{ + struct media_source *source = impl_from_IMFMediaSource(iface); + InterlockedIncrement(&source->internal_ref); +} + +static void media_source_internal_release(IMFMediaSource *iface) +{ + struct media_source *source = impl_from_IMFMediaSource(iface); + + if (!InterlockedDecrement(&source->internal_ref)) + { + IMFMediaEventQueue_Release(source->event_queue); + IMFByteStream_Release(source->byte_stream); + wg_parser_destroy(source->wg_parser); + source->cs.DebugInfo->Spare[0] = 0; + DeleteCriticalSection(&source->cs); + free(source); + } +} + static HRESULT WINAPI source_async_command_QueryInterface(IUnknown *iface, REFIID riid, void **obj) { if (IsEqualIID(riid, &IID_IUnknown)) @@ -962,7 +984,7 @@ static ULONG WINAPI media_stream_Release(IMFMediaStream *iface) if (!ref) { - IMFMediaSource_Release(stream->media_source); + media_source_internal_release(stream->media_source); IMFStreamDescriptor_Release(stream->descriptor); IMFMediaEventQueue_Release(stream->event_queue); flush_token_queue(stream, FALSE); @@ -1123,7 +1145,7 @@ static HRESULT media_stream_create(IMFMediaSource *source, IMFStreamDescriptor * return hr; } - IMFMediaSource_AddRef(source); + media_source_internal_addref(source); object->media_source = source; IMFStreamDescriptor_AddRef(descriptor); object->descriptor = descriptor; @@ -1362,12 +1384,7 @@ static ULONG WINAPI media_source_Release(IMFMediaSource *iface) if (!ref) { IMFMediaSource_Shutdown(iface); - IMFMediaEventQueue_Release(source->event_queue); - IMFByteStream_Release(source->byte_stream); - wg_parser_destroy(source->wg_parser); - source->cs.DebugInfo->Spare[0] = 0; - DeleteCriticalSection(&source->cs); - free(source); + media_source_internal_release(iface); } return ref; @@ -1635,6 +1652,7 @@ static HRESULT media_source_create(struct object_context *context, IMFMediaSourc object->IMFRateControl_iface.lpVtbl = &media_source_rate_control_vtbl; object->async_commands_callback.lpVtbl = &source_async_commands_callback_vtbl; object->ref = 1; + object->internal_ref = 1; object->byte_stream = context->stream; IMFByteStream_AddRef(context->stream); object->file_size = context->file_size; -- 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=149525 Your paranoid android. === w8adm (32 bit report) === mfplat: mfplat.c:1323: Test failed: Unexpected refcount 4, expected 1. === w1064v1507 (32 bit report) === mfplat: mfplat.c:1323: Test failed: Unexpected refcount 4, expected 1. mfplat.c:1326: Test failed: Unexpected refcount 3 === w1064_adm (64 bit report) === mfplat: mfplat.c:1323: Test failed: Unexpected refcount 4, expected 1.
While this works, and seems to fix hangs as well as leaks in Killsquad, additional refcount tests show that in Windows streams are created when `Start()` is called, and their references to the source are externally visible, so I will modify this to conform. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6783#note_86890
participants (3)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Marvin