Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak #107

Open
Gelassen opened this issue Jun 6, 2016 · 8 comments
Open

Memory leak #107

Gelassen opened this issue Jun 6, 2016 · 8 comments

Comments

@Gelassen
Copy link

Gelassen commented Jun 6, 2016

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
Copy link

vxhviet commented Jun 8, 2016

+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
Copy link

vxhviet commented Jun 8, 2016

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
Copy link

Dak0r commented Jan 13, 2017

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

@diegoperini
Copy link

diegoperini commented Jul 19, 2017

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
Copy link

pawaom commented Aug 10, 2017

@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
Copy link

@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,

  • I don't know how to publish the library to maven or any other repository.
  • I don't know how to compile new ffmpeg binaries for android in any architecture.

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
Copy link

pawaom commented Aug 23, 2017

@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
Copy link

diegoperini commented Aug 24, 2017

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();
            }
}));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants