Fixed music player memory leak.

philliskiragu · Jun 9, 2014 · d69747c · d69747c
1 parent c21d5fc
commit d69747c
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 99 deletions.
diff --git a/AndroidManifest.xml b/AndroidManifest.xml
@@ -1,8 +1,8 @@
 <?xml version="1.0" encoding="utf-8"?>
 <manifest xmlns:android="http://schemas.android.com/apk/res/android"
   package="org.jfedor.frozenbubble"
-  android:versionCode="34"
-  android:versionName="3.1">
+  android:versionCode="35"
+  android:versionName="3.2">
 
   <supports-screens
     android:smallScreens="true"

diff --git a/jni/jni_stubs.cpp b/jni/jni_stubs.cpp
@@ -102,17 +102,16 @@ const ModPlug_Settings gSettings32000 =
 //
 // ADD FOLLOWING JNI INTERFACE FUNCTIONS after the header files
 //
-// ************************************************************ 
+// ************************************************************
 // Start of JNI stub code
-// ************************************************************ 
-ModPlugFile *currmodFile;
+// ************************************************************
+ModPlugFile* currmodFile;
 
 #define SAMPLEBUFFERSIZE 40000
 
 unsigned char samplebuffer[SAMPLEBUFFERSIZE];
 
 int currsample;
-void *Cbuffer;
 
 /*
  * DIAB hack to change tempo!!
@@ -205,8 +204,10 @@ JNIEXPORT jboolean JNICALL Java_com_peculiargames_andmodplug_PlayerThread_ModPlu
   /*
    * Convert from Java buffer into a C buffer.
    */
-  Cbuffer = (void *) env->GetByteArrayElements(buffer, 0);
-  currmodFile = ModPlug_Load(Cbuffer, csize);
+  jbyte* bytes = env->GetByteArrayElements(buffer, 0);
+  currmodFile = ModPlug_Load(bytes, csize);
+  env->ReleaseByteArrayElements(buffer, bytes, 0);
+  env->DeleteLocalRef(buffer);
 
   DIABpatternchanged = 0;
   ANDMODPLUGpatternfrom = 0;

diff --git a/src/com/efortin/frozenbubble/ModPlayer.java b/src/com/efortin/frozenbubble/ModPlayer.java
@@ -91,7 +91,7 @@ public void loadNewSong(int songId, boolean startPlaying) {
       // Pause the current song.
       resPlayer.pausePlay(true);
       // Load the current MOD into the player.
-      resPlayer.loadModuleResource(songId, true);
+      resPlayer.loadModuleResource(songId);
       if (startPlaying)
         resPlayer.unPausePlay();
     }
@@ -112,7 +112,7 @@ private void newMusicPlayer(Context context,
     // Create a new music player.
     resPlayer = new MODResourcePlayer(context);
     // Load the MOD file.
-    resPlayer.loadModuleResource(songId, false);
+    resPlayer.loadModuleResource(songId);
     // Loop the song forever.
     resPlayer.setLoopCount(PlayerThread.LOOP_SONG_FOREVER);
     // Turn the music on or off.

diff --git a/src/com/peculiargames/andmodplug/MODResourcePlayer.java b/src/com/peculiargames/andmodplug/MODResourcePlayer.java
@@ -116,12 +116,10 @@ public MODResourcePlayer(Context context) {
    * MOD/XM song file, e.g. R.raw.coolsong
    * @param modResource - Android resource ID for a MOD/XM/etc. (tracker
    * format) song file.
-   * @param gc - if <code>true</code>, perform garbage collection prior
-   * to loading the file.
    */
-  public boolean loadModuleResource(int modResource, boolean gc) {
-    byte[] modData = null;
-    int currfilesize = 0;
+  public boolean loadModuleResource(int modResource) {
+    byte[]      modData      = null;
+    int         currfilesize = 0;
     InputStream mModfileInStream;
 
     /*
@@ -135,35 +133,38 @@ public boolean loadModuleResource(int modResource, boolean gc) {
     mModfileInStream = mContext.getResources().openRawResource(modResource);
     try {
       currfilesize = mModfileInStream.available();
-    } catch (IOException e) {
+    } catch (IOException ioe1) {
+      try {
+        mModfileInStream.close();
+      } catch (IOException ioe2) {
+        /*
+         * Should never happen.
+         */
+      }
       return false;
     }
 
     /*
      * Allocate a buffer that can hold the current MOD file data.
      */
     try {
-      /*
-       * This is a very critical and often very large memory allocation.
-       * Force the system to perform garbage collection to free up
-       * memory for the allocation.
-       *
-       * This is not recommended practice, as it can affect system
-       * performance.  This should only be called when the user is not
-       * interacting with the system.
-       */
-      if (gc) {
-        System.gc();
-      }
       modData = new byte[currfilesize];
     } catch (OutOfMemoryError oome) {
       // Auto-generated catch block.
       oome.printStackTrace();
+      try {
+        mModfileInStream.close();
+      } catch (IOException e) {
+        /*
+         * Should never happen.
+         */
+      }
       return false;
     }
 
     try {
-      setModSize(mModfileInStream.read(modData,0, currfilesize));
+      setModSize(mModfileInStream.read(modData, 0, currfilesize));
+      mModfileInStream.close();
     } catch (IOException e) {
       // Auto-generated catch block.
       e.printStackTrace();
@@ -199,6 +200,5 @@ public void stopAndClose() {
       }
     }
     closeLibModPlug();
-    invalidatePlayer();
   }
 }
diff --git a/src/com/peculiargames/andmodplug/PlayerThread.java b/src/com/peculiargames/andmodplug/PlayerThread.java
@@ -158,26 +158,11 @@ public class PlayerThread extends Thread {
   public final static float[] sVolume_floats = {0.0f, 0.125f, 0.25f, 0.375f,
                                                 0.5f, 0.625f, 0.75f, 1.0f};
 
-  /*
-   * Object for lock on PlayerValid check (mostly necessary for passing
-   * a single PlayerThread instance among Activities in an Android
-   * multi-activity application).
-   */
-  public static Object sPVlock;
-
   /*
    * Object for lock on ReadData call (to prevent UI thread messing
    * with player thread's GetSoundData() calls).
    */
-  public static Object sRDlock;
-
-  /*
-   * Mark the player as invalid for when an Activity shuts it down, but
-   * Android allows a reference to the player to persist.  A better
-   * solution is probably to just null out the reference to the
-   * PlayerThread object in whichever Activity shuts it down.
-   */
-  public  boolean mPlayerValid = false;
+  private Object sRDlock;
 
   /*
    * Private audio playback control flags.
@@ -236,13 +221,6 @@ public class PlayerThread extends Thread {
       Log.e(LOGPREFIX, "WARNING: Could not load libmodplug-"+VERS+".so");
       Log.e(LOGPREFIX, "------ older or differently named libmodplug???");
     }
-
-    /*
-     * Get lock objects for synchronizing access to playerValid flag and
-     * GetSoundData() call.
-     */
-    sPVlock = new Object();
-    sRDlock = new Object();
   }
 
   //********************************************************************
@@ -331,14 +309,17 @@ public PlayerThread(int desiredRate) {
     startPaused   = false;
     playerStarted = false;
 
+    /*
+     * Get lock object for synchronizing access to GetSoundData() call.
+     */
+    sRDlock = new Object();
+
     /*
      * Try to get the audio track.
      */
     if (!getAndroidAudioTrack(desiredRate)) {
       return;
     }
-
-    mPlayerValid = true;
   }
 
   /**
@@ -359,13 +340,9 @@ private void checkSongCompleted() {
     posWas = posNow;
   }
 
-  /**
-   * Close the native internal tracker library (libmodplug) and
-   * deallocate any resources.
-   */
-  public void closeLibModPlug() {
-    ModPlug_JUnload();
-    ModPlug_CloseDown();
+  private void cleanUp() {
+    mModname = null;
+    sRDlock  = null;
     /*
      * Release the audio track resources.
      */
@@ -376,6 +353,15 @@ public void closeLibModPlug() {
     }
   }
 
+  /**
+   * Close the native internal tracker library (libmodplug) and
+   * deallocate any resources.
+   */
+  public void closeLibModPlug() {
+    ModPlug_JUnload();
+    ModPlug_CloseDown();
+  }
+
   /**
    * Try to get an Android stereo audio track used by the various
    * constructors.
@@ -444,7 +430,6 @@ private boolean getAndroidAudioTrack(int desiredRate) {
     }
 
     if (mMyTrack == null) {
-      mPlayerValid = false;
       /*
        * Couldn't get an audio track so return false to caller.
        */
@@ -559,16 +544,6 @@ public int getRate() {
     return mRate;
   }
 
-  /**
-   * Mark this <code>PlayerThread</code> as invalid (typically when
-   * we're closing down the main Activity).
-   */
-  public void invalidatePlayer() {
-    synchronized(sPVlock) {
-      mPlayerValid = false;
-    }
-  }
-
   /**
    * Pauses playback of the current song.
    * @param immediate - if <code>true</code>, <code>pause()</code> then
@@ -614,24 +589,6 @@ public boolean pausePlay(boolean immediate) {
     return paused;
   }
 
-  /**
-   * This PlayerValid stuff is for multi-activity use, or also
-   * Android's Pause/Resume.
-   * <p>A better way to deal with it is probably to always stop and
-   * <code>join()</code> the PlayerThread in <code>onPause()</code> and
-   * allocate a new PlayerThread in <code>onResume()</code> (or
-   * <code>onCreate()</code>??).
-   * <p>Check if the player thread is still valid.
-   */
-  public boolean playerValid() {
-    /*
-     * Return whether this player is valid.
-     */
-    synchronized(sPVlock) {
-      return mPlayerValid;
-    }
-  }
-
   /**
    * The thread's run() call, where the modules are played.
    * <p>Start playing the MOD/XM song (hopefully it's been previously
@@ -788,14 +745,7 @@ public void run() {
       }
     }
 
-    /*
-     * Release the audio track resources.
-     */
-    if (mMyTrack != null)
-    {
-      mMyTrack.release();
-      mMyTrack = null;
-    }
+    cleanUp();
   }
 
   /**