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 fixes and proper killing of processes #214

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
@SuppressWarnings("unused")
public class FFmpeg implements FFmpegInterface {

private final Context context;
private final FFmpegContextProvider context;
private FFmpegExecuteAsyncTask ffmpegExecuteAsyncTask;
private FFmpegLoadLibraryAsyncTask ffmpegLoadLibraryAsyncTask;

Expand All @@ -21,14 +21,27 @@ public class FFmpeg implements FFmpegInterface {

private static FFmpeg instance = null;

private FFmpeg(Context context) {
this.context = context.getApplicationContext();
Log.setDEBUG(Util.isDebug(this.context));
private FFmpeg(FFmpegContextProvider contextProvider) {
this.context = contextProvider;
Log.setDEBUG(Util.isDebug(this.context.provide()));
}

public static FFmpeg getInstance(Context context) {
public static FFmpeg getInstance(FFmpegContextProvider contextProvider) {
if (instance == null) {
instance = new FFmpeg(context);
instance = new FFmpeg(contextProvider);
}
return instance;
}

@Deprecated
public static FFmpeg getInstance(final Context context) {
if (instance == null) {
instance = new FFmpeg(new FFmpegContextProvider() {
@Override
public Context provide() {
return context;
}
});
}
return instance;
}
Expand Down Expand Up @@ -63,7 +76,7 @@ public void execute(Map<String, String> environvenmentVars, String[] cmd, FFmpeg
throw new FFmpegCommandAlreadyRunningException("FFmpeg command is already running, you are only allowed to run single command at a time");
}
if (cmd.length != 0) {
String[] ffmpegBinary = new String[] { FileUtils.getFFmpeg(context, environvenmentVars) };
String[] ffmpegBinary = new String[] { FileUtils.getFFmpeg(context.provide(), environvenmentVars) };
String[] command = concatenate(ffmpegBinary, cmd);
ffmpegExecuteAsyncTask = new FFmpegExecuteAsyncTask(command , timeout, ffmpegExecuteResponseHandler);
ffmpegExecuteAsyncTask.execute();
Expand All @@ -72,7 +85,7 @@ public void execute(Map<String, String> environvenmentVars, String[] cmd, FFmpeg
}
}

public <T> T[] concatenate (T[] a, T[] b) {
private static <T> T[] concatenate (T[] a, T[] b) {
int aLen = a.length;
int bLen = b.length;

Expand All @@ -92,7 +105,7 @@ public void execute(String[] cmd, FFmpegExecuteResponseHandler ffmpegExecuteResp
@Override
public String getDeviceFFmpegVersion() throws FFmpegCommandAlreadyRunningException {
ShellCommand shellCommand = new ShellCommand();
CommandResult commandResult = shellCommand.runWaitFor(new String[] { FileUtils.getFFmpeg(context), "-version" });
CommandResult commandResult = shellCommand.runWaitFor(new String[] { FileUtils.getFFmpeg(context.provide()), "-version" });
if (commandResult.success) {
return commandResult.output.split(" ")[2];
}
Expand All @@ -102,17 +115,22 @@ public String getDeviceFFmpegVersion() throws FFmpegCommandAlreadyRunningExcepti

@Override
public String getLibraryFFmpegVersion() {
return context.getString(R.string.shipped_ffmpeg_version);
return context.provide().getString(R.string.shipped_ffmpeg_version);
}

@Override
public boolean isFFmpegCommandRunning() {
return ffmpegExecuteAsyncTask != null && !ffmpegExecuteAsyncTask.isProcessCompleted();
if (ffmpegExecuteAsyncTask == null)
return false;
else
return !ffmpegExecuteAsyncTask.isProcessCompleted();
}

@Override
public boolean killRunningProcesses() {
return Util.killAsync(ffmpegLoadLibraryAsyncTask) || Util.killAsync(ffmpegExecuteAsyncTask);
boolean status = Util.killAsync(ffmpegLoadLibraryAsyncTask) || Util.killAsync(ffmpegExecuteAsyncTask);
ffmpegExecuteAsyncTask = null;
return status;
}

@Override
Expand All @@ -121,4 +139,14 @@ public void setTimeout(long timeout) {
this.timeout = timeout;
}
}

@Override
public FFmpegObserver whenFFmpegIsReady(Runnable onReady, int timeout) {
return Util.observeOnce(new Util.ObservePredicate() {
@Override
public Boolean isReadyToProceed() {
return !isFFmpegCommandRunning();
}
}, onReady, timeout);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.github.hiteshsondhi88.libffmpeg;

import android.content.Context;

public interface FFmpegContextProvider {
Context provide();
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ private void checkAndUpdateProcess() throws TimeoutException, InterruptedExcepti
BufferedReader reader = new BufferedReader(new InputStreamReader(process.getErrorStream()));
while ((line = reader.readLine()) != null) {
if (isCancelled()) {
process.destroy();
process.waitFor();
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,11 @@ interface FFmpegInterface {
*/
public void setTimeout(long timeout);

/**
* Wait for ffmpeg to get ready asynchronously
* @param onReady code to run when FFmpeg is ready
* @param timeout when to give up in miliseconds
*/
public FFmpegObserver whenFFmpegIsReady(Runnable onReady, int timeout);

}
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,22 @@ class FFmpegLoadLibraryAsyncTask extends AsyncTask<Void, Void, Boolean> {

private final String cpuArchNameFromAssets;
private final FFmpegLoadBinaryResponseHandler ffmpegLoadBinaryResponseHandler;
private final Context context;
private final FFmpegContextProvider context;

FFmpegLoadLibraryAsyncTask(Context context, String cpuArchNameFromAssets, FFmpegLoadBinaryResponseHandler ffmpegLoadBinaryResponseHandler) {
FFmpegLoadLibraryAsyncTask(FFmpegContextProvider context, String cpuArchNameFromAssets, FFmpegLoadBinaryResponseHandler ffmpegLoadBinaryResponseHandler) {
this.context = context;
this.cpuArchNameFromAssets = cpuArchNameFromAssets;
this.ffmpegLoadBinaryResponseHandler = ffmpegLoadBinaryResponseHandler;
}

@Override
protected Boolean doInBackground(Void... params) {
File ffmpegFile = new File(FileUtils.getFFmpeg(context));
File ffmpegFile = new File(FileUtils.getFFmpeg(context.provide()));
if (ffmpegFile.exists() && isDeviceFFmpegVersionOld() && !ffmpegFile.delete()) {
return false;
}
if (!ffmpegFile.exists()) {
boolean isFileCopied = FileUtils.copyBinaryFromAssetsToData(context,
boolean isFileCopied = FileUtils.copyBinaryFromAssetsToData(context.provide(),
cpuArchNameFromAssets + File.separator + FileUtils.ffmpegFileName,
FileUtils.ffmpegFileName);

Expand Down Expand Up @@ -58,6 +58,6 @@ protected void onPostExecute(Boolean isSuccess) {
}

private boolean isDeviceFFmpegVersionOld() {
return CpuArch.fromString(FileUtils.SHA1(FileUtils.getFFmpeg(context))).equals(CpuArch.NONE);
return CpuArch.fromString(FileUtils.SHA1(FileUtils.getFFmpeg(context.provide()))).equals(CpuArch.NONE);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package com.github.hiteshsondhi88.libffmpeg;

public interface FFmpegObserver extends Runnable {
void cancel();
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,13 @@ static String convertInputStreamToString(InputStream inputStream) {
}

static void destroyProcess(Process process) {
if (process != null)
process.destroy();
if (process != null) {
try {
process.destroy();
} catch (Exception e) {
Log.e("process destroy error", e);
}
}
}

static boolean killAsync(AsyncTask asyncTask) {
Expand All @@ -71,4 +76,56 @@ static boolean isProcessCompleted(Process process) {
}
return false;
}

public interface ObservePredicate {
Boolean isReadyToProceed();
}

public static FFmpegObserver observeOnce(final ObservePredicate predicate, final Runnable run, final int timeout) {
final android.os.Handler observer = new android.os.Handler();

// Enable this to detect neverending observers
// final Exception e = new RuntimeException("WTF");

final FFmpegObserver observeAction = new FFmpegObserver() {
private boolean canceled = false;
private int timeElapsed = 0;

@Override
public void run() {
if (timeElapsed + 40 > timeout) cancel();
timeElapsed += 40;

if (canceled) return;

Boolean readyToProceed = null;
try {
readyToProceed = predicate.isReadyToProceed();
} catch (Exception e) {
// Log.v("Observing " + e.getMessage());
observer.postDelayed(this, 40);
return;
}

if (readyToProceed != null && readyToProceed) {
// Log.v("Observed");
run.run();
} else {
// Enable this to detect neverending observers
// Log.v("Util", "Observing", e);
// Log.v("Observing");
observer.postDelayed(this, 40);
}
}

@Override
public void cancel() {
canceled = true;
}
};

observer.post(observeAction);

return observeAction;
}
}
50 changes: 50 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,53 @@
!!! Deprecated !!!
===================

This project is discontinued. There is a much better [alternative by bravobit](https://github.com/bravobit/FFmpeg-Android) which incorporates good parts of this fork and also attempts to solve some existing issues. I'll be keeping a close eye to its issue tracker and contribute there instead.

About the fork
===================
### Installation
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.9'`


### Applied fixes

* Added `whenFFmpegIsReady()` to properly wait for ffmpeg state.
* Fixed `killRunningProcesses()` to properly kill the execution.
* 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.
* Fixed `isFFmpegCommandRunning()` to properly return running state status. (thanks to @pawaom)


### Help needed

* to update ffmpeg binary versions for all architectures
* to test the fixes
* to build and publish the fork somewhere more common

### License

GPLv3

-----------------------------------

[FFmpeg-Android-Java](http://writingminds.github.io/ffmpeg-android-java/) [![Build Status](https://travis-ci.org/hiteshsondhi88/ffmpeg-android-java.svg?branch=master)](https://travis-ci.org/hiteshsondhi88/ffmpeg-android-java) [![Android Arsenal](https://img.shields.io/badge/Android%20Arsenal-FFmpeg--Android--Java-brightgreen.svg?style=flat)](https://android-arsenal.com/details/1/931)
==============

Expand Down
8 changes: 4 additions & 4 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ allprojects {
}

ext {
compileSdkVersion = 22
buildToolsVersion = '22.0.1'
targetSdkVersion = 22
compileSdkVersion = 23
buildToolsVersion = '23.0.3'
targetSdkVersion = 23
minSdkVersion = 16
versionCode = 28
versionName = "0.3.2"
versionName = "0.4.8"
}