mpv-player / mpv

🎥 Command line video player
https://mpv.io
Other
28.11k stars 2.88k forks source link

libmpv: leaking 1 handle with each new mpv instance #14472

Closed wbtcpip2 closed 2 months ago

wbtcpip2 commented 3 months ago

mpv Information

mpv v0.38.0-568-g4969d6e0 Copyright © 2000-2024 mpv/MPlayer/mplayer2 projects
 built on Jun 30 2024 19:59:51
libplacebo version: v7.349.0 (v7.349.0-rc1-3-g1fd3c7b-dirty)
FFmpeg version: N-116061-gb818dff8d
FFmpeg library versions:
   libavcodec      61.9.100
   libavdevice     61.2.100
   libavfilter     10.2.102
   libavformat     61.4.100
   libavutil       59.27.100
   libswresample   5.2.100
   libswscale      8.2.100

Other Information

Reproduction Steps

this is a follow up of #14451 (that was closed)

I'm using this test program linked against the libmpv DLL: int main() { std::cin.ignore(10000, '\n'); std::cout << "Starting\n"; for (int i = 0; i < 500; i++) { auto handle = mpv_create(); mpv_initialize(handle); mpv_terminate_destroy(handle); } std::cout << "Finished\n"; std::cin.ignore(10000, '\n'); }

handles count increases linearly while the loop is running and never goes down again

before #14457 there was memory leak but not handles leak after #14457 memory leak is fixed but there is 1 handle leak.

@kasper93 could you please check if after your commit there is an handle that is not closed?

Expected Behavior

not to have 1 handle leak after every mpv instance

Actual Behavior

1 handle leak after each mpv instance

Log File

mpv-log.txt

Sample Files

No response

I carefully read all instruction and confirm that I did the following:

kasper93 commented 3 months ago

The handle comes from luajit allocator, you would better off reporting to them. It is not related to mpv.

a10r commented 3 months ago

I can confirm this, but I think this is unrelated to luajit. It happens in older builds as well, and even in builds with -Dlua=disabled.

It is not necessary to call mpv_initialize in this case, it is enough to call mpv_create followed by mpv_terminate_destroy in a loop.

According to Process Explorer the leaked handles are thread handles, and the thread ID matches the mpv "core" thread ID. Also, this does not happen if I compile with -Dwin32-threads=disabled.

I think this is what's happening: When terminating, the core thread is joined through mp_thread_join_id (in threads-win32.h), but OpenThread returns a different handle to the core thread so the handle originally created in mpv_create is still open.

wbtcpip2 commented 3 months ago

@a10r thank you for your report. Here i'm not seeing the same. But i'm only using libmpv i686 (32 bits). Here the leaked handle started after the luajit fix. in all the previous versions the handle numbers is stable. I cannot try with 64 bit libmpv.dll. Could you please try to you if you see any difference between 32 and 64 bit version?

a10r commented 3 months ago

@wbtcpip2 I tried shinchiro's 32bit build from a month ago and I'm seeing the same 1 thread handle leak with it as with 64bit. The win32-threads feature is only 8 months old, maybe your build is older.

wbtcpip2 commented 3 months ago

@wbtcpip2 I tried shinchiro's 32bit build from a month ago and I'm seeing the same 1 thread handle leak with it as with 64bit. The win32-threads feature is only 8 months old, maybe your build is older.

yes that's a possibility, however using this one: https://github.com/wbtcpip2/mpv-winbuild/releases/tag/2024-06-30-4969d6e and just removing the line "mpv_initialize(handle)" i'm not getting any handle leak.

i forgot to mention that i only use gcc builds.

could you please try this to you: https://github.com/wbtcpip2/mpv-winbuild/releases/tag/2024-06-30-4969d6e and tell me if you get the same leak without mpv_initialize

a10r commented 3 months ago

could you please try this to you: https://github.com/wbtcpip2/mpv-winbuild/releases/tag/2024-06-30-4969d6e and tell me if you get the same leak without mpv_initialize

Yes, same leak with that build as well.

wbtcpip2 commented 3 months ago

here do not leaks handle removing mpv_initialize but i confirm that whit mpv_initialize i'm getting a handle leak and according to Process Explorer the leaked handles are thread handles like you described.

kasper93 commented 3 months ago

What product are you using libmpv for?

To be honest I didn't really validate if we are not leaking handles from mpv, wasn't really important at any point. I can look next week when I have time, but feel free to send patches if you already know where it is leaking from.

And yes, we are clearly leaking at least one handle from mpv_create, which would be fixed by

diff --git a/player/client.c b/player/client.c
index 85854e31ff..8623309599 100644
--- a/player/client.c
+++ b/player/client.c
@@ -636,6 +636,7 @@ mpv_handle *mpv_create(void)
         mp_destroy(mpctx);
         return NULL;
     }
+    mp_thread_detach(thread);

     return ctx;
 }

Those threads are suppose to be detached, they are joined later, by id, same as all client threads.

a10r commented 3 months ago

What product are you using libmpv for?

In my case, a video wall for IP cameras. Though if OP hadn't brought it up I wouldn't have noticed or bothered to check for leaked handles either.

wbtcpip2 commented 3 months ago

i maintain an old 32 bit windows desktop application for a web tv channel that run nonstop with multiple video players concurrently (3 or 4). each hour consumes at least 40 new libmpv instances so a resource leak becomes visible very easy. Also being a 32 bit app the limit of 4gb virtual memory force me to check for the leaks. currently i'm using libvlc but i'm planning to switch to libmpv when leaks are possibly fixed