[PATCH v3 0/4] MR2958: mf: Various refcount leak fixes for session.

[Bernhard Kölbl (＠besentv)]

--
  v3: mfmediaengine/tests: Add refcount check for final release of media engine.

https://gitlab.winehq.org/wine/wine/-/merge_requests/2958

[PATCH v3 1/4] mf: Clear queued topologies on session shutdown.

[Bernhard Kölbl]

From: Bernhard Kölbl

This prevents media engine from leaking one reference to itself,
due to sharing its refcount with the sample grabber sink calback,
which was in turned gets refrenced through nodes in the queued topoligies.

Signed-off-by: Bernhard Kölbl 
---
 dlls/mf/session.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/dlls/mf/session.c b/dlls/mf/session.c
index 8b29c8487fb..3641cc74b83 100644
--- a/dlls/mf/session.c
+++ b/dlls/mf/session.c
@@ -2225,6 +2225,7 @@ static HRESULT WINAPI mfsession_Shutdown(IMFMediaSession *iface)
         IMFPresentationClock_Release(session->clock);
         session->clock = NULL;
         session_clear_presentation(session);
+        session_clear_queued_topologies(session);
         session_submit_simple_command(session, SESSION_CMD_SHUTDOWN);
     }
     LeaveCriticalSection(&session->cs);

[PATCH v3 2/4] mf: Don't leak sink in session_get_renderer_node_service().

[Bernhard Kölbl]

From: Bernhard Kölbl

Signed-off-by: Bernhard Kölbl 
---
 dlls/mf/session.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/dlls/mf/session.c b/dlls/mf/session.c
index 3641cc74b83..555a71dcdf7 100644
--- a/dlls/mf/session.c
+++ b/dlls/mf/session.c
@@ -2397,6 +2397,7 @@ static HRESULT session_get_renderer_node_service(struct media_session *session,
                             if (FAILED(hr = MFGetService((IUnknown *)sink, service, riid, obj)))
                                 WARN("Failed to get service from renderer node, %#lx.\n", hr);
                         }
+                        IMFMediaSink_Release(sink);
                     }
                     IMFStreamSink_Release(stream_sink);
                 }

[PATCH v3 3/4] mf: Don't make stream sink shutdown dependent on IMFActivate presence in node.

[Bernhard Kölbl]

From: Bernhard Kölbl

Nodes only hold a ref to an activate object when they are a sink anyway
(see session_bind_output_nodes() for reference).
This prevents media engine from getting references to its samplegrabber sink callback leaked.

Signed-off-by: Bernhard Kölbl 
---
 dlls/mf/session.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/mf/session.c b/dlls/mf/session.c
index 555a71dcdf7..efe568f987f 100644
--- a/dlls/mf/session.c
+++ b/dlls/mf/session.c
@@ -762,7 +762,7 @@ static void session_shutdown_current_topology(struct media_session *session)
                         WARN("Failed to shut down activation object for the sink, hr %#lx.\n", hr);
                     IMFActivate_Release(activate);
                 }
-                else if (SUCCEEDED(topology_node_get_object(node, &IID_IMFStreamSink, (void **)&stream_sink)))
+                if (SUCCEEDED(topology_node_get_object(node, &IID_IMFStreamSink, (void **)&stream_sink)))
                 {
                     if (SUCCEEDED(IMFStreamSink_GetMediaSink(stream_sink, &sink)))
                     {

[PATCH v3 4/4] mfmediaengine/tests: Add refcount check for final release of media engine.

[Bernhard Kölbl]

From: Bernhard Kölbl

Signed-off-by: Bernhard Kölbl 
---
 dlls/mfmediaengine/tests/mfmediaengine.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/dlls/mfmediaengine/tests/mfmediaengine.c b/dlls/mfmediaengine/tests/mfmediaengine.c
index 655b19af050..85e1ef4e7e4 100644
--- a/dlls/mfmediaengine/tests/mfmediaengine.c
+++ b/dlls/mfmediaengine/tests/mfmediaengine.c
@@ -1227,6 +1227,7 @@ static void test_TransferVideoFrame(void)
     UINT token;
     HRESULT hr;
     DWORD res;
+    ULONG ref;

stream = load_resource(L"i420-64x64.avi", L"video/avi");

@@ -1318,7 +1319,8 @@ static void test_TransferVideoFrame(void)

done:
     IMFMediaEngineEx_Shutdown(media_engine);
-    IMFMediaEngineEx_Release(media_engine);
+    ref = IMFMediaEngineEx_Release(media_engine);
+    ok(!ref, "Unexpected ref %lu.\n", ref);

ID3D11Texture2D_Release(texture);
     ID3D11Device_Release(device);

[PATCH v3 4/4] mfmediaengine/tests: Add refcount check for final release of media engine.

[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=133378

Your paranoid android.

=== w10pro64_ja (64 bit report) ===

mfmediaengine:
mfmediaengine.c:1314: Test failed: Unexpected 56% diff

[Bernhard Kölbl (＠besentv)]

On Sat Jun  3 08:21:37 2023 +0000, Nikolay Sivov wrote:
> Sure, if it's reliable. I'd expect after shutdown it should be releasing everything.

So I only added the test to test_TransferVideoFrame, because, as it turns out, the other ones shutdown and release too early. (It generally seems like MediaEngine ist still in startup for these tests and this can be confirmed by adding Sleep(100) before shutting down.)

[Nikolay Sivov (＠nsivov)]

On Mon Jun  5 10:07:26 2023 +0000, Bernhard Kölbl wrote:
> So I only added the test to test_TransferVideoFrame, because, as it
> turns out, the other ones shutdown and release too early. (It generally
> seems like MediaEngine ist still in startup for these tests and this can
> be confirmed by adding Sleep(100) before shutting down.)

The reason I asked about separate MR was to detach it from mfmediaengine changes. Tests you added, or commit messages do not belong to the session changes.

[Bernhard Kölbl (＠besentv)]

On Mon Jun  5 12:13:45 2023 +0000, Nikolay Sivov wrote:
> The reason I asked about separate MR was to detach it from mfmediaengine
> changes. Tests you added, or commit messages do not belong to the
> session changes.

So, should I remove the mediaengine test again? / Put in a separate MR?