mpv-player / mpv

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

Memory leak with destroy #9860

Closed cipher1985 closed 9 months ago

cipher1985 commented 2 years ago

Important Information

Once create, initialize and LoadFile are used, memory leakage will occur in destroy.

Mpv version and platform

libmpv: mpv-dev-i686-20220206-git-0197729 system: window 10 ide: vs2019

Code segment

mpv_handle mpv = nullptr; void play() { stop(); mpv = mpv_create(); mpv_initialize(mpv); const char args[] = { "loadfile", "1.mp4", NULL }; mpv_command_async(mpv, 0, args); } void stop() { if(mpv) { mpv_terminate_destroy(mpv); } }

Reproduction steps

Call the function play().

Expected behavior

No more allocates memory.

Actual behavior

Every use play function will allocates more 2-3M memory until it crashes.

Hrxn commented 2 years ago

Not sure what you have done to build mpv with MSVC, nice to know that it's apparently possible, but, as far as I know, it's not officially supported.

cipher1985 commented 2 years ago

@cipher1985 Did you use https://github.com/jhuix/mpv-vsbuild?

Or what? Also please use normal VS 2022 compiler.

the libmpv use precompiler https://sourceforge.net/projects/mpv-player-windows/files/libmpv/ mpv-dev-i686-20220213-git-bc9805c.7z

cipher1985 commented 2 years ago

I use the new libmpv download with this url : https://sourceforge.net/projects/mpv-player-windows/files/libmpv/?css-reload=2 the precompiler ver: mpv-dev-i686-20220612-git-602995f.7z but the memory also grow up.

cipher1985 commented 2 years ago

Which wrong with the code? ide: vs2019 system: win10 whole code:

include

pragma comment(lib,"libmpv.dll.a")

mpv_handle mpv = nullptr; void stop() { if (mpv) { mpv_terminate_destroy(mpv); } } void play() { stop(); mpv = mpv_create(); mpv_initialize(mpv); const char args[] = { "loadfile", "1.mp4", NULL }; mpv_command_async(mpv, 0, args); } int main() { for (int i = 0; i < 1000; ++i) { play(); //Why this function everytime make the memory grow up 2-3M? Sleep(2000); } stop(); return 0; }

wbtcpip2 commented 1 year ago

i was able to narrow down the problem

without playing anything a loop like this:

mpvhandle = mpv_create() mpv_initialize(mpvhandle) Sleep 10 mpv_destroy(mpvhandle)

cause approx 1 MB of memory leak at every cycle on win32

removing mpv_initialize there is no leak...so definitvely libmpv is not unitializing something at destroy

can someone test it and confirm it?

wbtcpip2 commented 1 year ago

the difference between memory allocated by mpv_initialize and freed by mpv_destroy is 768K after 1000 cycles there is a leak of 768 MB please fix it

wbtcpip2 commented 1 year ago

I tested all available versions of libmpv.

the memory leak started on April 12, 2020 with this version: https://master.dl.sourceforge.net/project/mpv-player-windows/libmpv/mpv-dev-i686-20200412-git-530a086.7z?viasf=1 mpv-version=mpv 0.32.0-352-g530a0863b8 ffmpeg-version=git-2020-04-12-f1894c206

the previous version has no memory leak: mpv-version=mpv 0.32.0-328-gc5f8ec76b1 ffmpeg-version=git-2020-04-05-8b1f07ef5

now it's possible to compare the code changes in mpv_initialize and mpv_destroy between the 2 versions? i woud really use the latest version and fix this massive leak

haasn commented 1 year ago

Actually, for me, this code just deadlocks on the second mpv_initialize call...

Edit: nvm, bug in my test code

haasn commented 1 year ago

I cannot reproduce this memory leak. On my end, memory usage stays more or less constant (going up and down by a few MB in each cycle, but not growing endlessly):

#include <mpv/client.h>
#include <stdio.h>
#include <unistd.h>

int main()
{
    for (int i = 0; i < 1000; i++) {
        printf("cycle %d\n", i);
        mpv_handle *mpv = mpv_create();
        mpv_initialize(mpv);
        usleep(10000);
        mpv_destroy(mpv);
    }

    pause();
}

The final memory usage is 43 MB, same as before the first test. Test mpv version was mpv 0.35+git.20221220.d5c3b9d9

wbtcpip2 commented 1 year ago

thank you for triyng this. what is your os version?

did you tried the 32 bits dll? or the 64bit?

wbtcpip2 commented 1 year ago

this is my minimalistic code to reproduce the problem:

Declare LIB "mpv-2.dll"
Declare Function mpv_create CDecl () As Int
Declare Function mpv_initialize CDecl (ByVal mpv_handle As Int) As Int
Declare Sub mpv_destroy CDecl (ByVal mpv_handle As Int)

Dim mpvhandle As Int, counter As Int

For counter = 1 To 1000
  mpvhandle = mpv_create()
  mpv_initialize(mpvhandle)
  Pause 1
  mpv_destroy(mpvhandle)
Next counter

this is the result after 1000 loop 2022-12-23_142738

Note that whith this same code if i only replace mpv-2.dll (32 bits) with mpv-1.dll (32 bits) from 5 april 2020 or previous the memory stay quiet no memory leak

i attach a compiled sample of the code above (i have digitally signed it so i I certify that it is not a malicious executable) can someone run it (put mpv-2.dll 32 bits near the exe) and tell me if the problem occurs? Thank you MPVTestLeak.zip

avih commented 1 year ago

without playing anything a loop like this:

mpvhandle = mpv_create() mpv_initialize(mpvhandle) Sleep 10 mpv_destroy(mpvhandle)

cause approx 1 MB of memory leak at every cycle on win32

Can you run your program after setting the env var MPV_LEAK_REPORT=1 and post any reported leaks? preferably reduce the program to one iteration of create/destroy.

wbtcpip2 commented 1 year ago

Can you run your program after setting the env var MPV_LEAK_REPORT=1 and post any reported leaks? preferably reduce the program to one iteration of create/destroy.

Unfortunately im unable to build libmpv binary by myself but if you give me a 32 bit debug version of libmpv that log memory allocations \ deallocations i will run it and show results.

avih commented 1 year ago

I don't think you need the debug version. Just set the env var, then run the program which you already compiled with the (normal) dll which you downloaded, and report the result please.

wbtcpip2 commented 1 year ago

pardon my ignorance, is MPV_LEAK_REPORT to be set as a an operating system environment variable? 2022-12-23_161731

and where do i expect to get the log? through the stdout? stderror?

avih commented 1 year ago

You can set it like in your picture (and then you need to open a new console).

Or, simpler, if you open a cmd.exe console window, then first run this line:

set MPV_LEAK_REPORT=1

And then, from the same console, run your program by typing its name and enter (e.g. MPVTestLeak.Exe), and let it exit normally (don't abort it with ctrl-c).

If leaks are detected, then the output should be at the console. I don't recall if it's stdout or stderr, but I'm guessing stderr.

wbtcpip2 commented 1 year ago

when i run my exe at the console it returns immeditely without waiting for the end execution (probably because my exe is not a consolle application but a standard desktop app). kindly you or haasn could you please compile for me a little exe that i will run here and post results? thank you.

avih commented 1 year ago

I compiled this program:

// build:
// (mingw) gcc:  gcc -o mpv-leak-test.gcc.exe mpv-leak-test.c -L. -lmpv
// (msvc 2015):  cl mpv-leak-test.c mpv.dll.a

#include <windows.h>
#include <stdio.h>
//#include <unistd.h>

#include "include/client.h"

int main(int argc, char **argv)
{
    int n = argc > 1 ? atoi(argv[1]) : 1;
    for (int i = 0; i < n; i++) {
        printf("%d ", i);
        mpv_handle *mpv = mpv_create();
        mpv_initialize(mpv);
        Sleep(100);
        mpv_destroy(mpv);
        Sleep(100);
    }

    return 0;
}

Using gcc first, then compiled using visual studio 2015 (64).

I tested it with mpv-2.dll (64) which I compiled myself.

The source, dll, and binaries (gcc 64, msvc 2015 64) are in this zip https://0x0.st/o5JR.zip .

EDIT: For reference, the dll at the zip is from 0.35.0-60-g874e28f4a4 (one commit behind current master 657fd280) plus some of my own patches on top, from this branch https://github.com/avih/mpv/commits/experiments-avih .

I let it iterate 500 times, and the program memory usage did not reach even 10M. Leaks were not reported either.

If you want to ensure that leaks are tested, delete the line mpv_destroy(mpv); and compile again, to see the leak report at the end. At the zip that's the file mpv-leak-test.msvc-2015.NO-DESTROY.exe.

I did not yet test with 32 bit dll downloaded from someone else. Maybe we can do that later.

wbtcpip2 commented 1 year ago

thank you i want to test your stuff but the link https://0x0.st/o5JR.zip say 2022-12-23_183459

could you please send me another link?

avih commented 1 year ago

thank you for triyng this. what is your os version?

did you tried the 32 bits dll? or the 64bit?

My earlier post has this info.

could you please send me another link?

No, I'm not going to try hosting sites until you can download. If you want, tell me where I can upload it so that you can download it.

this is my minimalistic code to reproduce the problem:

I'm not trying it. Let's stick to C.

Please use the program I tried (above, at the zip, and here below). Does it reproduce with it too?

// build:
// (mingw) gcc:  gcc -o mpv-leak-test.gcc.exe mpv-leak-test.c -L. -lmpv
// (msvc 2015):  cl mpv-leak-test.c mpv.dll.a

#include <windows.h>
#include <stdio.h>
//#include <unistd.h>

#include "include/client.h"

int main(int argc, char **argv)
{
    int n = argc > 1 ? atoi(argv[1]) : 1;
    for (int i = 0; i < n; i++) {
        printf("%d ", i);
        mpv_handle *mpv = mpv_create();
        mpv_initialize(mpv);
        Sleep(100);
        mpv_destroy(mpv);
        Sleep(100);
    }

    return 0;
}
mia-0 commented 1 year ago

thank you i want to test your stuff but the link https://0x0.st/o5JR.zip say 2022-12-23_183459

could you please send me another link?

Sorry about this, 0x0.st’s virus scanner had a false positive on the libmpv build in there and removed it. Should be fixed.

avih commented 1 year ago

could you please send me another link?

No, I'm not going to try hosting sites until you can download. If you want, tell me where I can upload it so that you can download it.

Sorry, the host thought it's a virus (because it has few executables compiled with two compilers?) and removed it, but now it's cleared as false-positive.

I've uploaded it again here https://0x0.st/o5JR.zip

wbtcpip2 commented 1 year ago

ok i downloaded. i'll test and report it

wbtcpip2 commented 1 year ago

i'm really confused now. all your stuuf works fine...no memory leak ...but i tried this: i replaced your mp-2.dll with the one downloaded from here: https://sourceforge.net/projects/mpv-player-windows/files/libmpv/mpv-dev-x86_64-20221218-git-f9d0b0c.7z/download and it happened the same meory leak that ccurs in my app. look: 2022-12-23_203418 how could be eplained? can you try the same to you?

avih commented 1 year ago

all your stuuf works fine...no memory leak ...but i tried this: i replaced your mp-2.dll with the one downloaded from here: https://sourceforge.net/projects/mpv-player-windows/files/libmpv/mpv-dev-x86_64-20221218-git-f9d0b0c.7z/download and it happened the same meory leak that ccurs in my app.

I can see the same if I run the same program and only replace the dll.

But I don't think this is a good test, because these are different DLLs, and you need to re-build the program again with the other DLL (and mpv.dll.a).

And when I did that, then the leaks go away, both when building with gcc, and when building with msvc.

So again, I can't reproduce your issue, also not with the DLL from sourceforge.

Can you please build my program, like the comment says, and check for leaks?

wbtcpip2 commented 1 year ago

unfortunately i don't have a c compiler. Normally i expect to be able to use a library directly like let's say libvlc ... i have a binary and an an include with all the functions that i can call from my app. i'm using client.h and mp-2.dll. as far as i know this should be enough but it produce memory leak. The last non leaking libmpv for me was mpv 0.32.0-328-gc5f8ec76b1. You can say "libmpv is only for c compilers" but at least until that date was fine for me and currently i'm using libvlc without problems. i'm not using mpv.dll.a, coudl be this the cause of my problems?

avih commented 1 year ago

unfortunately i don't have a c compiler.

Right, I just noticed it wasn't you who opened this issue.

Anyway, Using the C code/program I posted, I could not create a leak.

I don't know how to help you anymore.

wbtcpip2 commented 1 year ago

maybe a last help: are you able to compile a mp-2.dll 32 bits for me?

avih commented 1 year ago

are you able to compile a mp-2.dll 32 bits for me?

No, sorry, I don't have the required setup for this.

avih commented 1 year ago

Declare Function mpv_create CDecl () As Int Declare Function mpv_initialize CDecl (ByVal mpv_handle As Int) As Int Declare Sub mpv_destroy CDecl (ByVal mpv_handle As Int)

This looks like visual basic, and i'm completely unfamiliar with VB, so this is a shot in the dark.

How did you come up with these declarations?

mpv_handle is a struct, and the return value of mpv_create (and the argument of mpv_initialize and mpv_destroy) is a pointer to this struct.

You declare it as int, and I'm guessing that it can't hold the pointer, and therefore you don't pass the correct pointer to mpv_destroy (and also mpv_initialize), which maybe make it not destroy anything, hence a leak.

Also, why are there both Function and Sub declarations? shouldn't all of these be of the same call-type?

wbtcpip2 commented 1 year ago

mpv_handle it's a pointer to a struct so it's a 32 bit address that can be stored without problem in a 32 bit integer. Me i don't have to access directly that structure, I only have to pass that address not the structure istelf to the lib functions, and indeed all libpmv functions works perfectly. An address could be right or wrong, if it's wrong you only get an access violation not a leak. Sub are calls without return value ...if you look at client.h there are calls that not return any value

wbtcpip2 commented 1 year ago

I finally figured out where the problem is. i managed to compile mpv-2.dll i686 in my windows computer using msys2 and there is no memory leak with my builded version. i followed this page: https://github.com/mpv-player/mpv/blob/master/DOCS/compile-windows.md#native-compilation-with-msys2

The problem only occurs with shinchiro versions (when used as dynamic dll). I hope shinchiro will investigate the problem that occurs with his i686 dlls starting from 2020-04-05. Before that date shinchiro i686 mpv-1.dll has no leaks

so we can conclude that there is no memory leak in the destroy function of mpv

unfortunately I still have one last problem ... msys2 does not compile a single mpv-2.dll with all linked dependencies but I have to use dozens of other dlls. I hope to find a solution to this too.

avih commented 1 year ago

The problem only occurs with shinchiro versions

It might be related to different runtime used, maybe ucrt vs msvcrt. Usually you want to build the dll and your own program with similar enough environments. But anyway, we don't really have enough evidence to suggest that create+destroy on its own is leaking.

unfortunately I still have one last problem ... msys2 does not compile a single mpv-2.dll with all linked dependencies

This is completely off topic. It's because msys2 does not provide static mingw libraries. The main method to link statically with all the libs is to first compile all the libs statically yourslf - like shinchiro does, or using MXE (which is similarly lengthy).

Anyway, your issue of create->destroy->leak is most probably off topic for this issue, where the leak is after playing a file - https://github.com/mpv-player/mpv/issues/9860#issue-1133616853 .

Please don't post about your issue here (create/destroy -> leak). If you're convinced that create+destroy leaks, please open another issue (or request to re-open an existing issue on this subject).

wbtcpip2 commented 1 year ago

no i'm not off topic. I have the same issue of cipher1985. He do not post anymore here but i'm sure that if he remove the loadfile call it has the same 768 kb memory leak caused by the i686 version of shinchiro. (if you read the post he also is using that) I've already opened another issue at shinchiro but it was immediately closed because nobody believes me but it doesn't matter. However, know that it is not possible to embed the schichiro i686 mpv-2dll into a non-stop application due to this leak. Now I will study alternative methods to build mpv-2.dll i686

avih commented 1 year ago

i'm sure that if he remove the loadfile call it has the same 768 kb memory leak caused by the i686 version of shinchiro

I'm not sure because I can't know. If they confirm then you have the same issue, but until it's confirmed, to me it looks like different issues.

Dudemanguy commented 9 months ago

Old one but from skimming the discussion it looks like whatever leak was occurring here didn't come from mpv so closing.