cropsly / ffmpeg-android-java

Android java library for FFmpeg binary compiled using https://github.com/writingminds/ffmpeg-android
http://writingminds.github.io/ffmpeg-android-java
GNU General Public License v3.0
3.32k stars 828 forks source link

Memory leak #107

Open Gelassen opened 8 years ago

Gelassen commented 8 years ago

Could you please fix the memory leak for this wrapper?

The issue is the FFMPEG class is the singleton and contains the Context as member field. Application context is used as this context and makes app live till lifecycle of the application. Once initialized ffmpeg lib is keep in memory forever.

I would like to have an option to release the memory allocated for it. The good option from my point of view is to add a public method, e.g. onDispose() where context will be set in null.

vxhviet commented 8 years ago

+1, I'm scratching my head trying to fix this issue and would love to be able to use .AAR as it's the simplest way to add ffmpeg to the project.

Another issue, killRunningProcesses() sometimes doesn't seem to properly kill the process. This is mostly the way how the AsyncTask is handled. I presume this is one of the cause for memory leak, isn't it?

vxhviet commented 8 years ago

The easiest way to do this is to edit the source code and build a new .AAR file, you will have to build one anyway because of #79.

So for me I add this function in FFmpeg and call it every time I need to kill a process:

public void releaseContext(){
        context = null;
}

Or better yet, just remove the singleton.

Dak0r commented 7 years ago

+1, Hi, did someone already create a fork where killRunningProcesses and the memory leak was fixed? This repro seems to be dead :(

diegoperini commented 7 years ago

I managed to fix the issue on my fork. Shameful self promotion here. :)

Steps to install:

  1. Edit your project's build.gradle (not app) like this. (important line is jitpack)
allprojects {
    repositories {
        jcenter()
        mavenCentral()

        ...

        maven {
            url 'https://jitpack.io'
        }
    }
}
  1. Add below line to your app's build.gradle dependencies.

compile 'com.github.diegoperini:ffmpeg-android-java:v0.4.6'

Link for the fork: https://github.com/diegoperini/ffmpeg-android-java

Changelog:

  1. Added whenFFmpegIsReady() to properly wait for ffmpeg state.
  2. Fixed killRunningProcesses() to properly kill the execution.
  3. Added a FFmpeg.getInstance() overload to work with a ContextProvider instead of a context. It is a fix for a common memory leak caused by storing the context internally. Old factory method is still supported but marked as deprecated.
pawaom commented 7 years ago

@diegoperini , are you planning to maintain the code, it is really useful , no need to build another AAR of our own, it would be useful for others , there is a preblem with isFFmpegCommandRunning() alos they need to change it from

return ffmpegExecuteAsyncTask != null && !ffmpegExecuteAsyncTask.isProcessCompleted();

to

return ffmpegExecuteAsyncTask != null || !ffmpegExecuteAsyncTask.isProcessCompleted();

diegoperini commented 7 years ago

@pawaom , I am okay to commit bug fixes every now and then but to be honest, there are technical things I don't think I'm proficient enough such as,

Now these are mentioned, your suggestion is easy to apply. You can try and test it with my fork. I also updated the README on my fork.

Steps to install from scratch

Edit your project's build.gradle (not app) like this. (important line is jitpack)

allprojects {
    repositories {
        jcenter()
        mavenCentral()

        ...

        maven {
            url 'https://jitpack.io'
        }
    }
}

Add below line to your app's build.gradle dependencies.

compile 'com.github.diegoperini:ffmpeg-android-java:v0.4.7'

pawaom commented 7 years ago

@diegoperini , thanks for the reply, can you ask others to collaborate with you, can any one else help @diegoperini , to maintain the code, this code (writing minds) seems to be abandoned, no updates in the gradle, no one replays any thing

@diegoperini , have you solved the memory leak issue, as well , please keep this code on Github, i really dont know how to build an .AAR , can some one explain that process for us

diegoperini commented 7 years ago

The leak is solved @pawaom . Construct FFmpeg with newly implemented getInstance(ContextProvider) in a way that in the body of overridden ContextProvider.provide(), you return context by calling your fragment's getActivity() method.

Example:

FFmpeg f = FFmpeg.getInstance(new FFmpegContextProvider() {
            @Override
            public Context provide() {
                return myFragment.getActivity();
            }
}));