From eddae18caa1679c8f73abbc6e749d569890aa888 Mon Sep 17 00:00:00 2001 From: Mattia Iavarone Date: Thu, 29 Aug 2019 12:24:05 +0200 Subject: [PATCH] Frame improvements (#572) * Fix #544 * Improve Frames behavior and error messages * Improve Frames documentation * Fix tests * Fix video crashes --- .../engine/CameraIntegration2Test.java | 5 + .../engine/CameraIntegrationTest.java | 26 ++-- .../cameraview/frame/FrameManagerTest.java | 58 ++++----- .../gesture/ScrollGestureFinderTest.java | 6 +- .../otaliastudios/cameraview/CameraView.java | 34 ++--- .../cameraview/engine/Camera1Engine.java | 13 +- .../cameraview/engine/Camera2Engine.java | 49 ++++++-- .../cameraview/engine/CameraEngine.java | 2 +- .../otaliastudios/cameraview/frame/Frame.java | 62 +++++----- .../cameraview/frame/FrameManager.java | 117 +++++++++++------- .../picture/Snapshot1PictureRecorder.java | 2 +- .../cameraview/frame/FrameTest.java | 25 ++-- docs/_posts/2018-12-20-frame-processing.md | 42 ++++++- 13 files changed, 264 insertions(+), 177 deletions(-) rename cameraview/src/{test => androidTest}/java/com/otaliastudios/cameraview/frame/FrameManagerTest.java (66%) diff --git a/cameraview/src/androidTest/java/com/otaliastudios/cameraview/engine/CameraIntegration2Test.java b/cameraview/src/androidTest/java/com/otaliastudios/cameraview/engine/CameraIntegration2Test.java index 32ff78a7..1995eeee 100644 --- a/cameraview/src/androidTest/java/com/otaliastudios/cameraview/engine/CameraIntegration2Test.java +++ b/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(); + } } diff --git a/cameraview/src/androidTest/java/com/otaliastudios/cameraview/engine/CameraIntegrationTest.java b/cameraview/src/androidTest/java/com/otaliastudios/cameraview/engine/CameraIntegrationTest.java index 2bfbdcfa..9f95dfe4 100644 --- a/cameraview/src/androidTest/java/com/otaliastudios/cameraview/engine/CameraIntegrationTest.java +++ b/cameraview/src/androidTest/java/com/otaliastudios/cameraview/engine/CameraIntegrationTest.java @@ -136,6 +136,7 @@ public abstract class CameraIntegrationTest extends BaseTest { } } + @SuppressWarnings("StatementWithEmptyBody") private CameraOptions openSync(boolean expectSuccess) { camera.open(); final Op open = new Op<>(true); @@ -143,10 +144,11 @@ public abstract class CameraIntegrationTest extends BaseTest { 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 abstract class CameraIntegrationTest extends BaseTest { 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 abstract class CameraIntegrationTest extends BaseTest { 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 abstract class CameraIntegrationTest extends BaseTest { 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 @@ public abstract class CameraIntegrationTest extends BaseTest { // 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 abstract class CameraIntegrationTest extends BaseTest { openSync(true); takeVideoSync(true,4000); waitForVideoResult(true); + assert30Frames(processor); } diff --git a/cameraview/src/test/java/com/otaliastudios/cameraview/frame/FrameManagerTest.java b/cameraview/src/androidTest/java/com/otaliastudios/cameraview/frame/FrameManagerTest.java similarity index 66% rename from cameraview/src/test/java/com/otaliastudios/cameraview/frame/FrameManagerTest.java rename to cameraview/src/androidTest/java/com/otaliastudios/cameraview/frame/FrameManagerTest.java index 513a05fb..77375047 100644 --- a/cameraview/src/test/java/com/otaliastudios/cameraview/frame/FrameManagerTest.java +++ b/cameraview/src/androidTest/java/com/otaliastudios/cameraview/frame/FrameManagerTest.java @@ -1,14 +1,20 @@ 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; @@ -16,7 +22,9 @@ 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,12 +41,12 @@ public class FrameManagerTest { @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)); } @@ -46,12 +54,12 @@ public class FrameManagerTest { 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 class FrameManagerTest { @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); - } } diff --git a/cameraview/src/androidTest/java/com/otaliastudios/cameraview/gesture/ScrollGestureFinderTest.java b/cameraview/src/androidTest/java/com/otaliastudios/cameraview/gesture/ScrollGestureFinderTest.java index a7d6bb17..4a886ac2 100644 --- a/cameraview/src/androidTest/java/com/otaliastudios/cameraview/gesture/ScrollGestureFinderTest.java +++ b/cameraview/src/androidTest/java/com/otaliastudios/cameraview/gesture/ScrollGestureFinderTest.java @@ -21,6 +21,8 @@ import static org.junit.Assert.assertTrue; @SmallTest public class ScrollGestureFinderTest extends GestureFinderTest { + private final static long WAIT = 2000; // 500 was too short + @Override protected ScrollGestureFinder createFinder(@NonNull GestureFinder.Controller controller) { return new ScrollGestureFinder(controller); @@ -42,7 +44,7 @@ public class ScrollGestureFinderTest extends GestureFinderTest mFrameQueue; private LinkedBlockingQueue mBufferQueue; private BufferCallback mBufferCallback; @@ -94,17 +98,22 @@ public class FrameManager { /** * Allocates a {@link #mPoolSize} number of buffers. Should be called once - * the preview size and the bitsPerPixel value are known. + * the preview size and the image format value are known. * * This method can be called again after {@link #release()} has been called. * - * @param bitsPerPixel bits per pixel, depends on image format - * @param previewSize the preview size + * @param format the image format + * @param size the frame size * @return the buffer size */ - public int setUp(int bitsPerPixel, @NonNull Size previewSize) { - // TODO throw if called twice without release? - long sizeInBits = previewSize.getHeight() * previewSize.getWidth() * bitsPerPixel; + public int setUp(int format, @NonNull Size size) { + if (isSetUp()) { + // TODO throw or just reconfigure? + } + mFrameSize = size; + mFrameFormat = format; + int bitsPerPixel = ImageFormat.getBitsPerPixel(format); + long sizeInBits = size.getHeight() * size.getWidth() * bitsPerPixel; mBufferSize = (int) Math.ceil(sizeInBits / 8.0d); for (int i = 0; i < mPoolSize; i++) { if (mBufferMode == BUFFER_MODE_DISPATCH) { @@ -116,13 +125,24 @@ public class FrameManager { return mBufferSize; } + /** + * Returns true after {@link #setUp(int, Size)} + * but before {@link #release()}. + * Returns false otherwise. + * + * @return true if set up + */ + private boolean isSetUp() { + return mFrameSize != null; + } + /** * Returns a new byte buffer than can be filled. * This can only be called in {@link #BUFFER_MODE_ENQUEUE} mode! Where the frame * manager also holds a queue of the byte buffers. * * If not null, the buffer returned by this method can be filled and used to get - * a new frame through {@link #getFrame(byte[], long, int, Size, int)}. + * a new frame through {@link #getFrame(byte[], long, int)}. * * @return a buffer, or null */ @@ -143,7 +163,12 @@ public class FrameManager { if (mBufferMode != BUFFER_MODE_ENQUEUE) { throw new IllegalStateException("Can't call onBufferUnused() when not in BUFFER_MODE_ENQUEUE."); } - mBufferQueue.offer(buffer); + + if (isSetUp()) { + mBufferQueue.offer(buffer); + } else { + LOG.w("onBufferUnused: buffer was returned but we're not set up anymore."); + } } /** @@ -158,53 +183,38 @@ public class FrameManager { * @param data data * @param time timestamp * @param rotation rotation - * @param previewSize preview size - * @param previewFormat format * @return a new frame */ @NonNull - public Frame getFrame(@NonNull byte[] data, long time, int rotation, @NonNull Size previewSize, int previewFormat) { + public Frame getFrame(@NonNull byte[] data, long time, int rotation) { + if (!isSetUp()) { + throw new IllegalStateException("Can't call getFrame() after releasing or before setUp."); + } + Frame frame = mFrameQueue.poll(); if (frame != null) { - LOG.v("getFrame for time:", time, "RECYCLING.", "Data:", data != null); + LOG.v("getFrame for time:", time, "RECYCLING."); } else { - LOG.v("getFrame for time:", time, "CREATING.", "Data:", data != null); + LOG.v("getFrame for time:", time, "CREATING."); frame = new Frame(this); } - frame.set(data, time, rotation, previewSize, previewFormat); + frame.setContent(data, time, rotation, mFrameSize, mFrameFormat); return frame; } - /** - * Releases all frames controlled by this manager and - * clears the pool. - * In BUFFER_MODE_ENQUEUE, releases also all the buffers. - */ - public void release() { - LOG.w("Releasing all frames!"); - for (Frame frame : mFrameQueue) { - frame.releaseManager(); - frame.release(); - } - mFrameQueue.clear(); - if (mBufferMode == BUFFER_MODE_ENQUEUE) { - mBufferQueue.clear(); - } - mBufferSize = -1; - } - /** * Called by child frames when they are released. + * This might be called from old Frames that belong to an old 'setUp' + * of this FrameManager instance. So the buffer size might be different, + * for instance. + * * @param frame the released frame */ - void onFrameReleased(@NonNull Frame frame) { - byte[] buffer = frame.getData(); - boolean willRecycle = mFrameQueue.offer(frame); - if (!willRecycle) { - // If frame queue is full, let's drop everything. - frame.releaseManager(); - } else { - // If frame will be recycled, let's recycle the buffer as well. + void onFrameReleased(@NonNull Frame frame, @NonNull byte[] buffer) { + if (!isSetUp()) return; + // If frame queue is full, let's drop everything. + // If frame queue accepts this frame, let's recycle the buffer as well. + if (mFrameQueue.offer(frame)) { int currSize = buffer.length; int reqSize = mBufferSize; if (currSize == reqSize) { @@ -216,4 +226,25 @@ public class FrameManager { } } } + + /** + * Releases all frames controlled by this manager and + * clears the pool. + * In BUFFER_MODE_ENQUEUE, releases also all the buffers. + */ + public void release() { + if (!isSetUp()) { + LOG.w("release called twice. Ignoring."); + return; + } + + LOG.i("release: Clearing the frame and buffer queue."); + mFrameQueue.clear(); + if (mBufferMode == BUFFER_MODE_ENQUEUE) { + mBufferQueue.clear(); + } + mBufferSize = -1; + mFrameSize = null; + mFrameFormat = -1; + } } diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/picture/Snapshot1PictureRecorder.java b/cameraview/src/main/java/com/otaliastudios/cameraview/picture/Snapshot1PictureRecorder.java index c0404821..606ecaaa 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/picture/Snapshot1PictureRecorder.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/picture/Snapshot1PictureRecorder.java @@ -87,7 +87,7 @@ public class Snapshot1PictureRecorder extends PictureRecorder { // It seems that the buffers are already cleared here, so we need to allocate again. camera.setPreviewCallbackWithBuffer(null); // Release anything left camera.setPreviewCallbackWithBuffer(mEngine1); // Add ourselves - mEngine1.getFrameManager().setUp(ImageFormat.getBitsPerPixel(mFormat), previewStreamSize); + mEngine1.getFrameManager().setUp(mFormat, previewStreamSize); } }); } diff --git a/cameraview/src/test/java/com/otaliastudios/cameraview/frame/FrameTest.java b/cameraview/src/test/java/com/otaliastudios/cameraview/frame/FrameTest.java index b9fea96f..3f31548b 100644 --- a/cameraview/src/test/java/com/otaliastudios/cameraview/frame/FrameTest.java +++ b/cameraview/src/test/java/com/otaliastudios/cameraview/frame/FrameTest.java @@ -15,6 +15,8 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNull; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -35,23 +37,24 @@ public class FrameTest { @Test public void testEquals() { + // Only time should count. Frame f1 = new Frame(manager); long time = 1000; - f1.set(null, time, 90, null, ImageFormat.NV21); + f1.setContent(new byte[3], time, 90, new Size(5, 5), ImageFormat.NV21); Frame f2 = new Frame(manager); - f2.set(new byte[2], time, 0, new Size(10, 10), ImageFormat.NV21); + f2.setContent(new byte[2], time, 0, new Size(10, 10), ImageFormat.NV21); assertEquals(f1, f2); - f2.set(new byte[2], time + 1, 0, new Size(10, 10), ImageFormat.NV21); + f2.setContent(new byte[2], time + 1, 0, new Size(10, 10), ImageFormat.NV21); assertNotEquals(f1, f2); } @Test public void testReleaseThrows() { final Frame frame = new Frame(manager); - frame.set(new byte[2], 1000, 90, new Size(10, 10), ImageFormat.NV21); + frame.setContent(new byte[2], 1000, 90, new Size(10, 10), ImageFormat.NV21); frame.release(); - verify(manager, times(1)).onFrameReleased(frame); + verify(manager, times(1)).onFrameReleased(eq(frame), any(byte[].class)); assertThrows(new Runnable() { public void run() { frame.getTime(); }}); assertThrows(new Runnable() { public void run() { frame.getFormat(); }}); @@ -69,14 +72,6 @@ public class FrameTest { } } - @Test - public void testReleaseManager() { - Frame frame = new Frame(manager); - assertNotNull(frame.mManager); - frame.releaseManager(); - assertNull(frame.mManager); - } - @Test public void testFreeze() { Frame frame = new Frame(manager); @@ -85,7 +80,7 @@ public class FrameTest { int rotation = 90; Size size = new Size(10, 10); int format = ImageFormat.NV21; - frame.set(data, time, rotation, size, format); + frame.setContent(data, time, rotation, size, format); Frame frozen = frame.freeze(); assertArrayEquals(data, frozen.getData()); @@ -94,7 +89,7 @@ public class FrameTest { assertEquals(size, frozen.getSize()); // Mutate the first, ensure that frozen is not affected - frame.set(new byte[]{3, 2, 1}, 50, 180, new Size(1, 1), ImageFormat.JPEG); + frame.setContent(new byte[]{3, 2, 1}, 50, 180, new Size(1, 1), ImageFormat.JPEG); assertArrayEquals(data, frozen.getData()); assertEquals(time, frozen.getTime()); assertEquals(rotation, frozen.getRotation()); diff --git a/docs/_posts/2018-12-20-frame-processing.md b/docs/_posts/2018-12-20-frame-processing.md index 08a7d02d..864f441e 100644 --- a/docs/_posts/2018-12-20-frame-processing.md +++ b/docs/_posts/2018-12-20-frame-processing.md @@ -18,7 +18,7 @@ a QR code detector, the cameraView.addFrameProcessor(new FrameProcessor() { @Override @WorkerThread - public void process(Frame frame) { + public void process(@NonNull Frame frame) { byte[] data = frame.getData(); int rotation = frame.getRotation(); long time = frame.getTime(); @@ -33,9 +33,45 @@ For your convenience, the `FrameProcessor` method is run in a background thread in a synchronous fashion. Once the process method returns, internally we will re-use the `Frame` instance and apply new data to it. So: -- you can do your job synchronously in the `process()` method +- you can do your job synchronously in the `process()` method. This is **recommended**. - if you must hold the `Frame` instance longer, use `frame = frame.freeze()` to get a frozen instance - that will not be affected + that will not be affected. This is **discouraged** because it requires copying the whole array. + +### Process synchronously + +Processing synchronously, for the duration of the `process()` method, is the recommended way of using +processors, because it solves different issues: + +- avoids the need of calling `frame = frame.freeze()` which is a very expensive operation +- the engine will **automatically drop frames** if the `process()` method is busy, so you'll only receive frames that you can handle +- we have already allocated a thread for you, so there's no need to create another + +Some frame consumers might have a built-in asynchronous behavior. +But you can still block the `process()` thread until the consumer has returned. + +```java +@Override +@WorkerThread +public void process(@NonNull Frame frame) { + + // EXAMPLE 1: + // Firebase and Google APIs will often return a Task. + // You can use Tasks.await() to complete the task on the current thread. + Tasks.await(firebaseDetector.detectInImage(firebaseImage)); + + // EXAMPLE 2: + // For other async consumers, you can use, for example, a CountDownLatch. + + // Step 1: create the latch. + final CountDownLatch latch = new CountDownLatch(1); + + // Step 2: launch async processing here... + // When processing completes or fails, call latch.countDown(); + + // Step 3: after launching, block the current thread. + latch.await(); +} +``` ### Related APIs