[PATCH v3 0/4] MR2958: mf: Various refcount leak fixes for session.
-- v3: mfmediaengine/tests: Add refcount check for final release of media engine. https://gitlab.winehq.org/wine/wine/-/merge_requests/2958
From: Bernhard Kölbl <besentv(a)gmail.com> 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 <besentv(a)gmail.com> --- 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2958
From: Bernhard Kölbl <besentv(a)gmail.com> Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- 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); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2958
From: Bernhard Kölbl <besentv(a)gmail.com> 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 <besentv(a)gmail.com> --- 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))) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2958
From: Bernhard Kölbl <besentv(a)gmail.com> Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2958
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
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.)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2958#note_34639
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2958#note_34644
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?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2958#note_34768
participants (3)
-
Bernhard Kölbl -
Marvin -
Nikolay Sivov (＠nsivov)