Fix processor OOM bug (#431)

* Fix processor bug

* Add test

* Try to fix CI

* Fix Frame tests

* Fix hidden tests

.travis.yml

@@ -16,17 +16,16 @@ jdk:
 
 env:
   global:
-    # Where to run androidTests
     - EMULATOR_API=22 # 24 has some issues, probably some overlayed window
-    - EMULATOR_ABI=armeabi-v7a
+    - EMULATOR_ABI=x86_64 # seems to work with emulator v29.
-    - EMULATOR_TAG=default
+    - EMULATOR_TAG=default # can be google_apis
     - PATH=$ANDROID_HOME:$ANDROID_HOME/emulator:$ANDROID_HOME/platform-tools:$PATH
 
 android:
   components:
     - tools
     - platform-tools
-    - build-tools-28.0.2
+    - build-tools-28.0.3
     - android-28
     - doc-28
 
@@ -38,15 +37,18 @@ install:
   - echo yes | sdkmanager "emulator" # Ensure emulator is present
 
   # Install emulator
+  # The channel=4 line looks into canary which brings in v29.
+  # The previous version v28 was broken:
+  # https://travis-ci.community/t/android-emulators-not-starting-for-the-last-few-days-late-march-2019/2871/11?u=mikehardy
   - export EMULATOR="system-images;android-$EMULATOR_API;$EMULATOR_TAG;$EMULATOR_ABI"
-  - echo yes | sdkmanager "platforms;android-$EMULATOR_API" # Install sdk
+  - echo yes | sdkmanager "platforms;android-$EMULATOR_API" # Install sdk for the emulator
-  - echo yes | sdkmanager "$EMULATOR" # Install system image
+  - echo yes | sdkmanager --channel=4 "$EMULATOR" # Install system image
   - sdkmanager --list || true # Check everything is updated
 
   # Create adn start emulator
-  - echo no | avdmanager create avd -n test -k "$EMULATOR" -f # Create emulator
+  - echo no | avdmanager create avd -n test -k "$EMULATOR" -f # Create emulator virtual device
-  - which emulator # ensure we are using the right emulator (home/emulator/)
+  - which emulator # ensure we are using the right emulator ($ANDROID_HOME/emulator/emulator)
-  - emulator -avd test -no-window -camera-back emulated -camera-front emulated -memory 2048 -writable-system & # Launch
+  - emulator -avd test -no-window -no-accel -no-snapshot -camera-back emulated -camera-front emulated -memory 2048 -writable-system & # Launch
   - adb wait-for-device # Wait for adb process
   - adb remount # Mount as writable
 

cameraview/src/androidTest/java/com/otaliastudios/cameraview/IntegrationTest.java

@@ -6,6 +6,7 @@ import android.graphics.PointF;
 import android.hardware.Camera;
 import android.os.Build;
 
+import androidx.annotation.NonNull;
 import androidx.test.ext.junit.runners.AndroidJUnit4;
 import androidx.test.filters.MediumTest;
 import androidx.test.rule.ActivityTestRule;
@@ -29,6 +30,7 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
 
 
 /**
@@ -416,7 +418,7 @@ public class IntegrationTest extends BaseTest {
     @Test
     public void testEndVideo_withMaxSize() {
         camera.setMode(Mode.VIDEO);
-        camera.setVideoMaxSize(500*1000); // 0.5 mb
+        camera.setVideoMaxSize(3000*1000); // Less is risky
         waitForOpen(true);
         waitForVideoStart();
         waitForVideoEnd(true);
@@ -597,5 +599,26 @@ public class IntegrationTest extends BaseTest {
         assert30Frames(processor);
     }
 
+
+    @Test
+    public void testFrameProcessing_freezeRelease() throws Exception {
+        // Ensure that freeze/release cycles do not cause OOMs.
+        // There was a bug doing this and it might resurface for any improper
+        // disposal of the frames.
+        FrameProcessor source = new FreezeReleaseFrameProcessor();
+        FrameProcessor processor = spy(source);
+        camera.addFrameProcessor(processor);
+        waitForOpen(true);
+
+        assert30Frames(processor);
+    }
+
+    public class FreezeReleaseFrameProcessor implements FrameProcessor {
+        @Override
+        public void process(@NonNull Frame frame) {
+            frame.freeze().release();
+        }
+    }
+
     //endregion
 }

cameraview/src/main/java/com/otaliastudios/cameraview/Frame.java

@@ -20,6 +20,18 @@ public class Frame {
         mManager = manager;
     }
 
+    boolean isAlive() {
+        return mData != null;
+    }
+
+    private void ensureAlive() {
+        if (!isAlive()) {
+            throw new RuntimeException("You should not access a released frame. " +
+                    "If this frame was passed to a FrameProcessor, you can only use its contents synchronously," +
+                    "for the duration of the process() method.");
+        }
+    }
+
     void set(@NonNull byte[] data, long time, int rotation, @NonNull Size size, int format) {
         this.mData = data;
         this.mTime = time;
@@ -44,6 +56,7 @@ public class Frame {
     @SuppressWarnings("WeakerAccess")
     @NonNull
     public Frame freeze() {
+        ensureAlive();
         byte[] data = new byte[mData.length];
         System.arraycopy(mData, 0, data, 0, mData.length);
         Frame other = new Frame(mManager);
@@ -56,11 +69,12 @@ public class Frame {
      * that are not useful anymore.
      */
     public void release() {
+        if (!isAlive()) return;
+
         if (mManager != null) {
             // If needed, the manager will call releaseManager on us.
             mManager.onFrameReleased(this);
         }
-
         mData = null;
         mRotation = 0;
         mTime = -1;
@@ -79,6 +93,7 @@ public class Frame {
      */
     @NonNull
     public byte[] getData() {
+        ensureAlive();
         return mData;
     }
 
@@ -89,6 +104,7 @@ public class Frame {
      * @return time data
      */
     public long getTime() {
+        ensureAlive();
         return mTime;
     }
 
@@ -100,6 +116,7 @@ public class Frame {
      * @return clock-wise rotation
      */
     public int getRotation() {
+        ensureAlive();
         return mRotation;
     }
 
@@ -110,6 +127,7 @@ public class Frame {
      */
     @NonNull
     public Size getSize() {
+        ensureAlive();
         return mSize;
     }
 
@@ -122,6 +140,7 @@ public class Frame {
      * @see android.graphics.ImageFormat
      */
     public int getFormat() {
+        ensureAlive();
         return mFormat;
     }
 }

cameraview/src/main/java/com/otaliastudios/cameraview/FrameManager.java

@@ -52,15 +52,17 @@ class FrameManager {
         byte[] buffer = frame.getData();
         boolean willRecycle = mQueue.offer(frame);
         if (!willRecycle) {
+            // If frame queue is full, let's drop everything.
             frame.releaseManager();
-        }
+        } else {
-        if (buffer != null && mCallback != null) {
+            // If frame will be recycled, let's recycle the buffer as well.
             int currSize = buffer.length;
             int reqSize = mBufferSize;
-            if (currSize == reqSize) {
+            if (currSize == reqSize && mCallback != null) {
                 mCallback.onBufferAvailable(buffer);
             }
         }
+
     }
 
     /**

cameraview/src/test/java/com/otaliastudios/cameraview/FrameManagerTest.java

@@ -65,14 +65,20 @@ public class FrameManagerTest {
     }
 
     @Test
-    public void testOnFrameReleased_nullBuffer() {
+    public void testOnFrameReleased_alreadyFull() {
         FrameManager manager = new FrameManager(1, callback);
-        manager.allocate(4, new Size(50, 50));
+        int length = manager.allocate(4, new Size(50, 50));
-        reset(callback);
 
-        Frame frame = manager.getFrame(null, 0, 0, null, 0);
+        Frame frame1 = manager.getFrame(new byte[length], 0, 0, null, 0);
-        manager.onFrameReleased(frame);
+        // Since frame1 is already taken and poolSize = 1, a new Frame is created.
-        verify(callback, never()).onBufferAvailable(frame.getData());
+        Frame frame2 = manager.getFrame(new byte[length], 0, 0, null, 0);
+        // Release the first frame so it goes back into the pool.
+        manager.onFrameReleased(frame1);
+        reset(callback);
+        // Release the second. The pool is already full, so onBufferAvailable should not be called
+        // since this Frame instance will NOT be reused.
+        manager.onFrameReleased(frame2);
+        verify(callback, never()).onBufferAvailable(frame2.getData());
     }
 
     @Test
@@ -118,7 +124,6 @@ public class FrameManagerTest {
 
         // Release the whole manager and ensure it clears the frame.
         manager.release();
-        assertNull(first.getData());
         assertNull(first.mManager);
     }
 }

cameraview/src/test/java/com/otaliastudios/cameraview/FrameTest.java

@@ -6,6 +6,7 @@ import android.graphics.ImageFormat;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.mockito.Mockito;
 
 import static junit.framework.Assert.assertNotNull;
 import static org.junit.Assert.assertArrayEquals;
@@ -33,16 +34,6 @@ public class FrameTest {
         manager = null;
     }
 
-    @Test
-    public void testDefaults() {
-        Frame frame = new Frame(manager);
-        assertEquals(frame.getTime(), -1);
-        assertEquals(frame.getFormat(), -1);
-        assertEquals(frame.getRotation(), 0);
-        assertNull(frame.getData());
-        assertNull(frame.getSize());
-    }
-
     @Test
     public void testEquals() {
         Frame f1 = new Frame(manager);
@@ -57,17 +48,26 @@ public class FrameTest {
     }
 
     @Test
-    public void testRelease() {
+    public void testReleaseThrows() {
-        Frame frame = new Frame(manager);
+        final Frame frame = new Frame(manager);
         frame.set(new byte[2], 1000, 90, new Size(10, 10), ImageFormat.NV21);
         frame.release();
-
-        assertEquals(frame.getTime(), -1);
-        assertEquals(frame.getFormat(), -1);
-        assertEquals(frame.getRotation(), 0);
-        assertNull(frame.getData());
-        assertNull(frame.getSize());
         verify(manager, times(1)).onFrameReleased(frame);
+
+        assertThrows(new Runnable() { public void run() { frame.getTime(); }});
+        assertThrows(new Runnable() { public void run() { frame.getFormat(); }});
+        assertThrows(new Runnable() { public void run() { frame.getRotation(); }});
+        assertThrows(new Runnable() { public void run() { frame.getData(); }});
+        assertThrows(new Runnable() { public void run() { frame.getSize(); }});
+    }
+
+    private void assertThrows(Runnable runnable) {
+        try {
+            runnable.run();
+            throw new IllegalStateException("Expected an exception but found none.");
+        } catch (Exception e) {
+            // All good
+        }
     }
 
     @Test

demo/src/main/java/com/otaliastudios/cameraview/demo/CameraActivity.java

@@ -7,6 +7,8 @@ import android.os.Bundle;
 import androidx.annotation.NonNull;
 import com.google.android.material.bottomsheet.BottomSheetBehavior;
 import androidx.appcompat.app.AppCompatActivity;
+
+import android.util.Log;
 import android.view.View;
 import android.view.ViewGroup;
 import android.view.ViewTreeObserver;
@@ -17,6 +19,8 @@ import com.otaliastudios.cameraview.CameraListener;
 import com.otaliastudios.cameraview.CameraLogger;
 import com.otaliastudios.cameraview.CameraOptions;
 import com.otaliastudios.cameraview.CameraView;
+import com.otaliastudios.cameraview.Frame;
+import com.otaliastudios.cameraview.FrameProcessor;
 import com.otaliastudios.cameraview.PictureResult;
 import com.otaliastudios.cameraview.Mode;
 import com.otaliastudios.cameraview.VideoResult;
@@ -40,14 +44,7 @@ public class CameraActivity extends AppCompatActivity implements View.OnClickLis
 
         camera = findViewById(R.id.camera);
         camera.setLifecycleOwner(this);
-        camera.addCameraListener(new CameraListener() {
+        camera.addCameraListener(new Listener());
-            public void onCameraOpened(@NonNull CameraOptions options) { onOpened(options); }
-            public void onPictureTaken(@NonNull PictureResult result) { onPicture(result); }
-            public void onVideoTaken(@NonNull VideoResult result) { onVideo(result); }
-            public void onCameraError(@NonNull CameraException exception) {
-                onError(exception);
-            }
-        });
 
         findViewById(R.id.edit).setOnClickListener(this);
         findViewById(R.id.capturePicture).setOnClickListener(this);
@@ -80,7 +77,10 @@ public class CameraActivity extends AppCompatActivity implements View.OnClickLis
         Toast.makeText(this, content, length).show();
     }
 
-    private void onOpened(CameraOptions options) {
+    private class Listener extends CameraListener {
+
+        @Override
+        public void onCameraOpened(@NonNull CameraOptions options) {
             ViewGroup group = (ViewGroup) controlPanel.getChildAt(0);
             for (int i = 0; i < group.getChildCount(); i++) {
                 ControlView view = (ControlView) group.getChildAt(i);
@@ -88,11 +88,15 @@ public class CameraActivity extends AppCompatActivity implements View.OnClickLis
             }
         }
 
-    private void onError(@NonNull CameraException exception) {
+        @Override
+        public void onCameraError(@NonNull CameraException exception) {
+            super.onCameraError(exception);
             message("Got CameraException #" + exception.getReason(), true);
         }
 
-    private void onPicture(PictureResult result) {
+        @Override
+        public void onPictureTaken(@NonNull PictureResult result) {
+            super.onPictureTaken(result);
             if (camera.isTakingVideo()) {
                 message("Captured while taking video. Size=" + result.getSize(), false);
                 return;
@@ -108,11 +112,14 @@ public class CameraActivity extends AppCompatActivity implements View.OnClickLis
             mCaptureTime = 0;
         }
 
-    private void onVideo(VideoResult video) {
+        @Override
-        VideoPreviewActivity.setVideoResult(video);
+        public void onVideoTaken(@NonNull VideoResult result) {
+            super.onVideoTaken(result);
+            VideoPreviewActivity.setVideoResult(result);
             Intent intent = new Intent(CameraActivity.this, VideoPreviewActivity.class);
             startActivity(intent);
         }
+    }
 
     @Override
     public void onClick(View view) {