Frame improvements (#572)

* Fix #544

* Improve Frames behavior and error messages

* Improve Frames documentation

* Fix tests

* Fix video crashes

cameraview/src/androidTest/java/com/otaliastudios/cameraview/engine/CameraIntegration2Test.java

@@ -27,4 +27,9 @@ public class CameraIntegration2Test extends CameraIntegrationTest {
     protected Engine getEngine() {
         return Engine.CAMERA2;
     }
+
+    @Override
+    public void testFrameProcessing_afterVideo() throws Exception {
+        super.testFrameProcessing_afterVideo();
+    }
 }

cameraview/src/androidTest/java/com/otaliastudios/cameraview/engine/CameraIntegrationTest.java

@@ -136,17 +136,19 @@ private void waitForUiException() throws Throwable {
         }
     }
 
+    @SuppressWarnings("StatementWithEmptyBody")
     private CameraOptions openSync(boolean expectSuccess) {
         camera.open();
         final Op<CameraOptions> open = new Op<>(true);
         doEndOp(open, 0).when(listener).onCameraOpened(any(CameraOptions.class));
         CameraOptions result = open.await(DELAY);
         if (expectSuccess) {
             assertNotNull("Can open", result);
-            // Extra wait for the bind state.
-            // TODO fix this and other while {} in this class in a more elegant way.
-            //noinspection StatementWithEmptyBody
+            // Extra wait for the bind and preview state, so we run tests in a fully operational
+            // state. If we didn't do so, we could have null values, for example, in getPictureSize
+            // or in getSnapshotSize.
             while (controller.getBindState() != CameraEngine.STATE_STARTED) {}
+            while (controller.getPreviewState() != CameraEngine.STATE_STARTED) {}
         } else {
             assertNull("Should not open", result);
         }
@@ -194,6 +196,9 @@ public boolean matches(CameraException argument) {
                 video.listen();
                 result = video.await(DELAY);
             }
+            // Sleep another 1000, because camera.isTakingVideo() might return false even
+            // if the result still has to be dispatched. Rare but could happen.
+            try { Thread.sleep(1000); } catch (InterruptedException ignore) {}
         }
 
         // Now we should be OK.
@@ -684,14 +689,11 @@ public void testCapturePicture_concurrentCalls() throws Exception {
         assertEquals(1, latch.getCount());
     }
 
-    @SuppressWarnings("StatementWithEmptyBody")
     @Test
     public void testCapturePicture_size() {
         openSync(true);
-        // PictureSize can still be null after opened.
-        // TODO be more elegant
-        while (camera.getPictureSize() == null) {}
         Size size = camera.getPictureSize();
+        assertNotNull(size);
         camera.takePicture();
         PictureResult result = waitForPictureResult(true);
         assertNotNull(result);
@@ -734,14 +736,11 @@ public void testCaptureSnapshot_concurrentCalls() throws Exception {
         assertEquals(1, latch.getCount());
     }
 
-    @SuppressWarnings("StatementWithEmptyBody")
     @Test
     public void testCaptureSnapshot_size() {
         openSync(true);
-        // SnapshotSize can still be null after opened.
-        // TODO be more elegant
-        while (camera.getSnapshotSize() == null) {}
         Size size = camera.getSnapshotSize();
+        assertNotNull(size);
         camera.takePictureSnapshot();
 
         PictureResult result = waitForPictureResult(true);
@@ -764,8 +763,8 @@ private void assert30Frames(FrameProcessor mock) throws Exception {
         // Expect 30 frames
         CountDownLatch latch = new CountDownLatch(30);
         doCountDown(latch).when(mock).process(any(Frame.class));
-        boolean did = latch.await(60, TimeUnit.SECONDS);
-        assertTrue(did);
+        boolean did = latch.await(15, TimeUnit.SECONDS);
+        assertTrue("Latch count should be 0: " + latch.getCount(), did);
     }
 
     @Test
@@ -811,6 +810,7 @@ public void testFrameProcessing_afterVideo() throws Exception {
         openSync(true);
         takeVideoSync(true,4000);
         waitForVideoResult(true);
+
         assert30Frames(processor);
     }
 

cameraview/src/test/java/com/otaliastudios/cameraview/frame/FrameManagerTest.java → cameraview/src/androidTest/java/com/otaliastudios/cameraview/frame/FrameManagerTest.java

@@ -1,22 +1,30 @@
 package com.otaliastudios.cameraview.frame;
 
 
+import android.graphics.ImageFormat;
+
+import androidx.test.ext.junit.runners.AndroidJUnit4;
+import androidx.test.filters.SmallTest;
+
+import com.otaliastudios.cameraview.BaseTest;
 import com.otaliastudios.cameraview.size.Size;
 
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
-public class FrameManagerTest {
+@RunWith(AndroidJUnit4.class)
+@SmallTest
+public class FrameManagerTest extends BaseTest {
 
     private FrameManager.BufferCallback callback;
 
@@ -33,25 +41,25 @@ public void tearDown() {
     @Test
     public void testAllocate() {
         FrameManager manager = new FrameManager(1, callback);
-        manager.setUp(4, new Size(50, 50));
+        manager.setUp(ImageFormat.NV21, new Size(50, 50));
         verify(callback, times(1)).onBufferAvailable(any(byte[].class));
         reset(callback);
 
         manager = new FrameManager(5, callback);
-        manager.setUp(4, new Size(50, 50));
+        manager.setUp(ImageFormat.NV21, new Size(50, 50));
         verify(callback, times(5)).onBufferAvailable(any(byte[].class));
     }
 
     @Test
     public void testFrameRecycling() {
         // A 1-pool manager will always recycle the same frame.
         FrameManager manager = new FrameManager(1, callback);
-        manager.setUp(4, new Size(50, 50));
+        manager.setUp(ImageFormat.NV21, new Size(50, 50));
 
-        Frame first = manager.getFrame(null, 0, 0, null, 0);
+        Frame first = manager.getFrame(null, 0, 0);
         first.release();
 
-        Frame second = manager.getFrame(null, 0, 0, null, 0);
+        Frame second = manager.getFrame(null, 0, 0);
         second.release();
 
         assertEquals(first, second);
@@ -60,63 +68,51 @@ public void testFrameRecycling() {
     @Test
     public void testOnFrameReleased_alreadyFull() {
         FrameManager manager = new FrameManager(1, callback);
-        int length = manager.setUp(4, new Size(50, 50));
+        int length = manager.setUp(ImageFormat.NV21, new Size(50, 50));
 
-        Frame frame1 = manager.getFrame(new byte[length], 0, 0, null, 0);
+        Frame frame1 = manager.getFrame(new byte[length], 0, 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);
+        Frame frame2 = manager.getFrame(new byte[length], 0, 0);
         // Release the first frame so it goes back into the pool.
-        manager.onFrameReleased(frame1);
+        manager.onFrameReleased(frame1, frame1.getData());
         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);
+        manager.onFrameReleased(frame2, frame2.getData());
         verify(callback, never()).onBufferAvailable(frame2.getData());
     }
 
     @Test
     public void testOnFrameReleased_sameLength() {
         FrameManager manager = new FrameManager(1, callback);
-        int length = manager.setUp(4, new Size(50, 50));
+        int length = manager.setUp(ImageFormat.NV21, new Size(50, 50));
 
         // A camera preview frame comes. Request a frame.
         byte[] picture = new byte[length];
-        Frame frame = manager.getFrame(picture, 0, 0, null, 0);
+        Frame frame = manager.getFrame(picture, 0, 0);
 
         // Release the frame and ensure that onBufferAvailable is called.
         reset(callback);
-        manager.onFrameReleased(frame);
+        manager.onFrameReleased(frame, frame.getData());
         verify(callback, times(1)).onBufferAvailable(picture);
     }
 
     @Test
     public void testOnFrameReleased_differentLength() {
         FrameManager manager = new FrameManager(1, callback);
-        int length = manager.setUp(4, new Size(50, 50));
+        int length = manager.setUp(ImageFormat.NV21, new Size(50, 50));
 
         // A camera preview frame comes. Request a frame.
         byte[] picture = new byte[length];
-        Frame frame = manager.getFrame(picture, 0, 0, null, 0);
+        Frame frame = manager.getFrame(picture, 0, 0);
 
         // Don't release the frame. Change the allocation size.
-        manager.setUp(2, new Size(15, 15));
+        manager.setUp(ImageFormat.NV16, new Size(15, 15));
 
         // Now release the old frame and ensure that onBufferAvailable is NOT called,
         // because the released data has wrong length.
-        manager.onFrameReleased(frame);
+        manager.onFrameReleased(frame, frame.getData());
         reset(callback);
         verify(callback, never()).onBufferAvailable(picture);
     }
-
-    @Test
-    public void testRelease() {
-        FrameManager manager = new FrameManager(1, callback);
-        int length = manager.setUp(4, new Size(50, 50));
-        Frame first = manager.getFrame(new byte[length], 0, 0, null, 0);
-        first.release(); // Store this frame in the queue.
-
-        // Release the whole manager and ensure it clears the frame.
-        manager.release();
-        assertNull(first.mManager);
-    }
 }

cameraview/src/androidTest/java/com/otaliastudios/cameraview/gesture/ScrollGestureFinderTest.java

@@ -21,6 +21,8 @@
 @SmallTest
 public class ScrollGestureFinderTest extends GestureFinderTest<ScrollGestureFinder> {
 
+    private final static long WAIT = 2000; // 500 was too short
+
     @Override
     protected ScrollGestureFinder createFinder(@NonNull GestureFinder.Controller controller) {
         return new ScrollGestureFinder(controller);
@@ -42,15 +44,15 @@ public void testScrollDisabled() {
         touchOp.listen();
         touchOp.start();
         onLayout().perform(swipeUp());
-        Gesture found = touchOp.await(500);
+        Gesture found = touchOp.await(WAIT);
         assertNull(found);
     }
 
     private void testScroll(ViewAction scroll, Gesture expected, boolean increasing) {
         touchOp.listen();
         touchOp.start();
         onLayout().perform(scroll);
-        Gesture found = touchOp.await(500);
+        Gesture found = touchOp.await(WAIT);
         assertEquals(found, expected);
 
         // How will this move our parameter?

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

@@ -2097,28 +2097,30 @@ public void run() {
         }
 
         @Override
-        public void dispatchFrame(final Frame frame) {
+        public void dispatchFrame(@NonNull final Frame frame) {
+            // The getTime() below might crash if developers incorrectly release frames asynchronously.
             mLogger.v("dispatchFrame:", frame.getTime(), "processors:", mFrameProcessors.size());
             if (mFrameProcessors.isEmpty()) {
                 // Mark as released. This instance will be reused.
                 frame.release();
-                return;
-            }
-            mFrameProcessorsHandler.run(new Runnable() {
-                @Override
-                public void run() {
-                    for (FrameProcessor processor : mFrameProcessors) {
-                        try {
-                            processor.process(frame);
-                        } catch (Exception e) {
-                            mLogger.w("dispatchFrame:",
-                                    "Error during processor implementation.",
-                                    "Can happen when camera is closed while processors are running.", e);
+            } else {
+                // Dispatch this frame to frame processors.
+                mFrameProcessorsHandler.run(new Runnable() {
+                    @Override
+                    public void run() {
+                        mLogger.v("dispatchFrame: dispatching", frame.getTime(), "to processors.");
+                        for (FrameProcessor processor : mFrameProcessors) {
+                            try {
+                                processor.process(frame);
+                            } catch (Exception e) {
+                                // Don't let a single processor crash the processor thread.
+                                mLogger.w("Frame processor crashed:", e);
+                            }
                         }
+                        frame.release();
                     }
-                    frame.release();
-                }
-            });
+                });
+            }
         }
 
         @Override

cameraview/src/main/java/com/otaliastudios/cameraview/engine/Camera1Engine.java

@@ -210,7 +210,7 @@ protected Task<Void> onStartPreview() {
 
         mCamera.setPreviewCallbackWithBuffer(null); // Release anything left
         mCamera.setPreviewCallbackWithBuffer(this); // Add ourselves
-        getFrameManager().setUp(ImageFormat.getBitsPerPixel(PREVIEW_FORMAT), mPreviewStreamSize);
+        getFrameManager().setUp(PREVIEW_FORMAT, mPreviewStreamSize);
 
         LOG.i("onStartPreview", "Starting preview with startPreview().");
         try {
@@ -654,12 +654,15 @@ public void onBufferAvailable(@NonNull byte[] buffer) {
     }
 
     @Override
-    public void onPreviewFrame(@NonNull byte[] data, Camera camera) {
+    public void onPreviewFrame(byte[] data, Camera camera) {
+        if (data == null) {
+            // Let's test this with an exception.
+            throw new RuntimeException("Camera1 returns null data from onPreviewFrame! " +
+                    "This would make the frame processors crash later.");
+        }
         Frame frame = getFrameManager().getFrame(data,
                 System.currentTimeMillis(),
-                getAngles().offset(Reference.SENSOR, Reference.OUTPUT, Axis.RELATIVE_TO_SENSOR),
-                mPreviewStreamSize,
-                PREVIEW_FORMAT);
+                getAngles().offset(Reference.SENSOR, Reference.OUTPUT, Axis.RELATIVE_TO_SENSOR));
         mCallback.dispatchFrame(frame);
     }