From b9620b70e60ab805a9cdc9fea7fa10c0ca0f854f Mon Sep 17 00:00:00 2001 From: Mattia Iavarone Date: Fri, 5 Apr 2019 21:19:46 -0300 Subject: [PATCH] Fix processor OOM bug (#431) * Fix processor bug * Add test * Try to fix CI * Fix Frame tests * Fix hidden tests --- .travis.yml | 20 ++--- .../cameraview/IntegrationTest.java | 25 +++++- .../com/otaliastudios/cameraview/Frame.java | 21 ++++- .../cameraview/FrameManager.java | 8 +- .../cameraview/FrameManagerTest.java | 19 +++-- .../otaliastudios/cameraview/FrameTest.java | 36 ++++----- .../cameraview/demo/CameraActivity.java | 77 ++++++++++--------- 7 files changed, 132 insertions(+), 74 deletions(-) diff --git a/.travis.yml b/.travis.yml index 0b7ddbec..db728959 100644 --- a/.travis.yml +++ b/.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_TAG=default + - EMULATOR_ABI=x86_64 # seems to work with emulator v29. + - 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 "$EMULATOR" # Install system image + - echo yes | sdkmanager "platforms;android-$EMULATOR_API" # Install sdk for the emulator + - 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 - - which emulator # ensure we are using the right emulator (home/emulator/) - - emulator -avd test -no-window -camera-back emulated -camera-front emulated -memory 2048 -writable-system & # Launch + - echo no | avdmanager create avd -n test -k "$EMULATOR" -f # Create emulator virtual device + - which emulator # ensure we are using the right emulator ($ANDROID_HOME/emulator/emulator) + - 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 diff --git a/cameraview/src/androidTest/java/com/otaliastudios/cameraview/IntegrationTest.java b/cameraview/src/androidTest/java/com/otaliastudios/cameraview/IntegrationTest.java index d7d47eb4..227d7833 100644 --- a/cameraview/src/androidTest/java/com/otaliastudios/cameraview/IntegrationTest.java +++ b/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 } diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/Frame.java b/cameraview/src/main/java/com/otaliastudios/cameraview/Frame.java index 3b584839..44a83928 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/Frame.java +++ b/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; } } diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/FrameManager.java b/cameraview/src/main/java/com/otaliastudios/cameraview/FrameManager.java index e05f607f..f08eaf1c 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/FrameManager.java +++ b/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(); - } - if (buffer != null && mCallback != null) { + } else { + // 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); } } + } /** diff --git a/cameraview/src/test/java/com/otaliastudios/cameraview/FrameManagerTest.java b/cameraview/src/test/java/com/otaliastudios/cameraview/FrameManagerTest.java index 98959d3a..6a9dd72d 100644 --- a/cameraview/src/test/java/com/otaliastudios/cameraview/FrameManagerTest.java +++ b/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)); - reset(callback); + int length = manager.allocate(4, new Size(50, 50)); - Frame frame = manager.getFrame(null, 0, 0, null, 0); - manager.onFrameReleased(frame); - verify(callback, never()).onBufferAvailable(frame.getData()); + Frame frame1 = manager.getFrame(new byte[length], 0, 0, null, 0); + // Since frame1 is already taken and poolSize = 1, a new Frame is created. + 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); } } diff --git a/cameraview/src/test/java/com/otaliastudios/cameraview/FrameTest.java b/cameraview/src/test/java/com/otaliastudios/cameraview/FrameTest.java index 13892402..22f27942 100644 --- a/cameraview/src/test/java/com/otaliastudios/cameraview/FrameTest.java +++ b/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() { - Frame frame = new Frame(manager); + public void testReleaseThrows() { + 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 diff --git a/demo/src/main/java/com/otaliastudios/cameraview/demo/CameraActivity.java b/demo/src/main/java/com/otaliastudios/cameraview/demo/CameraActivity.java index 0b0d348a..ea80ab64 100644 --- a/demo/src/main/java/com/otaliastudios/cameraview/demo/CameraActivity.java +++ b/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() { - 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); - } - }); + camera.addCameraListener(new Listener()); findViewById(R.id.edit).setOnClickListener(this); findViewById(R.id.capturePicture).setOnClickListener(this); @@ -80,38 +77,48 @@ public class CameraActivity extends AppCompatActivity implements View.OnClickLis Toast.makeText(this, content, length).show(); } - private void onOpened(CameraOptions options) { - ViewGroup group = (ViewGroup) controlPanel.getChildAt(0); - for (int i = 0; i < group.getChildCount(); i++) { - ControlView view = (ControlView) group.getChildAt(i); - view.onCameraOpened(camera, options); - } - } + private class Listener extends CameraListener { - private void onError(@NonNull CameraException exception) { - message("Got CameraException #" + exception.getReason(), true); - } + @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); + view.onCameraOpened(camera, options); + } + } - private void onPicture(PictureResult result) { - if (camera.isTakingVideo()) { - message("Captured while taking video. Size=" + result.getSize(), false); - return; + @Override + public void onCameraError(@NonNull CameraException exception) { + super.onCameraError(exception); + message("Got CameraException #" + exception.getReason(), true); } - // This can happen if picture was taken with a gesture. - long callbackTime = System.currentTimeMillis(); - if (mCaptureTime == 0) mCaptureTime = callbackTime - 300; - PicturePreviewActivity.setPictureResult(result); - Intent intent = new Intent(CameraActivity.this, PicturePreviewActivity.class); - intent.putExtra("delay", callbackTime - mCaptureTime); - startActivity(intent); - mCaptureTime = 0; - } + @Override + public void onPictureTaken(@NonNull PictureResult result) { + super.onPictureTaken(result); + if (camera.isTakingVideo()) { + message("Captured while taking video. Size=" + result.getSize(), false); + return; + } + + // This can happen if picture was taken with a gesture. + long callbackTime = System.currentTimeMillis(); + if (mCaptureTime == 0) mCaptureTime = callbackTime - 300; + PicturePreviewActivity.setPictureResult(result); + Intent intent = new Intent(CameraActivity.this, PicturePreviewActivity.class); + intent.putExtra("delay", callbackTime - mCaptureTime); + startActivity(intent); + mCaptureTime = 0; + } - private void onVideo(VideoResult video) { - VideoPreviewActivity.setVideoResult(video); - Intent intent = new Intent(CameraActivity.this, VideoPreviewActivity.class); - startActivity(intent); + @Override + public void onVideoTaken(@NonNull VideoResult result) { + super.onVideoTaken(result); + VideoPreviewActivity.setVideoResult(result); + Intent intent = new Intent(CameraActivity.this, VideoPreviewActivity.class); + startActivity(intent); + } } @Override