[PATCH v2 0/2] MR6783: winegstreamer: Fix a memory/resource leak.

[Conor McCarthy (＠cmccarthy)]

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.

--
  v2: 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

[PATCH v2 1/2] mfplat/tests: Test the refcount before the media source is released.

[Conor McCarthy]

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..22798d627d6 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 steams 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

[PATCH v2 2/2] winegstreamer: Introduce an internal refcount in media source objects.

[Conor McCarthy]

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 22798d627d6..9e4bdd63167 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 steams 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

[PATCH v2 2/2] winegstreamer: Introduce an internal refcount in media source objects.

[Marvin]

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=149523

Your paranoid android.


=== w1064_2qxl (64 bit report) ===

mfplat:
mfplat.c:1323: Test failed: Unexpected refcount 4, expected 1.

[Conor McCarthy (＠cmccarthy)]

Test now uses `EXPECT_REF` and I added a comment for it.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6783#note_86825

[Nicholas Ansell (＠roadrunnerto)]

Nicholas Ansell (@roadrunnerto) commented about dlls/mfplat/tests/mfplat.c:
> ok(mediasource != NULL, "got %p\n", mediasource);
>      ok(obj_type == MF_OBJECT_MEDIASOURCE, "got %d\n", obj_type);
>
> +    /* test that contained steams do not hold externally visible refs to mediasource */

```suggestion:-0+0
    /* test that contained streams do not hold externally visible refs to mediasource */
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6783#note_86829