feat: Synchronize Frame properly (#2501)

* feat: Synchronize `Frame` properly * Update CameraError.ts * Image is not valid if `refCount` < 0
mrousavy · Feb 5, 2024 · d8c95c9 · d8c95c9
1 parent 97168c6
commit d8c95c9
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 56 deletions.
diff --git a/package/android/src/main/java/com/mrousavy/camera/core/CameraError.kt b/package/android/src/main/java/com/mrousavy/camera/core/CameraError.kt
@@ -113,6 +113,16 @@ class RecordingInProgressError :
     "recording-in-progress",
     "There is already an active video recording in progress! Did you call startRecording() twice?"
   )
+class FrameInvalidError :
+  CameraError(
+    "capture",
+    "frame-invalid",
+    "Trying to access an already closed Frame! " +
+      "Are you trying to access the Image data outside of a Frame Processor's lifetime?\n" +
+      "- If you want to use `console.log(frame)`, use `console.log(frame.toString())` instead.\n" +
+      "- If you want to do async processing, use `runAsync(...)` instead.\n" +
+      "- If you want to use runOnJS, increment it's ref-count: `frame.incrementRefCount()`"
+  )
 
 class CodeTypeNotSupportedError(codeType: String) :
   CameraError(

diff --git a/package/android/src/main/java/com/mrousavy/camera/frameprocessor/Frame.java b/package/android/src/main/java/com/mrousavy/camera/frameprocessor/Frame.java
@@ -4,6 +4,7 @@
 import android.media.Image;
 import android.os.Build;
 import com.facebook.proguard.annotations.DoNotStrip;
+import com.mrousavy.camera.core.FrameInvalidError;
 import com.mrousavy.camera.core.HardwareBuffersNotAvailableError;
 import com.mrousavy.camera.types.PixelFormat;
 import com.mrousavy.camera.types.Orientation;
@@ -23,122 +24,123 @@ public Frame(Image image, long timestamp, Orientation orientation, boolean isMir
         this.isMirrored = isMirrored;
     }
 
-    public Image getImage() {
-        synchronized (this) {
-            Image img = image;
-            if (!getIsImageValid(img)) {
-                throw new RuntimeException("Frame is already closed! " +
-                    "Are you trying to access the Image data outside of a Frame Processor's lifetime?\n" +
-                    "- If you want to use `console.log(frame)`, use `console.log(frame.toString())` instead.\n" +
-                    "- If you want to do async processing, use `runAsync(...)` instead.\n" +
-                    "- If you want to use runOnJS, increment it's ref-count: `frame.incrementRefCount()`");
-            }
-            return img;
+    private void assertIsValid() throws FrameInvalidError {
+        if (!getIsImageValid(image)) {
+            throw new FrameInvalidError();
         }
     }
 
-    @SuppressWarnings("unused")
-    @DoNotStrip
-    public int getWidth() {
-        return getImage().getWidth();
+    private synchronized boolean getIsImageValid(Image image) {
+        if (refCount <= 0) return false;
+        try {
+            // will throw an exception if the image is already closed
+            image.getFormat();
+            // no exception thrown, image must still be valid.
+            return true;
+        } catch (IllegalStateException e) {
+            // exception thrown, image has already been closed.
+            return false;
+        }
+    }
+
+    public synchronized Image getImage() {
+        return image;
     }
 
     @SuppressWarnings("unused")
     @DoNotStrip
-    public int getHeight() {
-        return getImage().getHeight();
+    public synchronized int getWidth() throws FrameInvalidError {
+        assertIsValid();
+        return image.getWidth();
     }
 
     @SuppressWarnings("unused")
     @DoNotStrip
-    public boolean getIsValid() {
-        return getIsImageValid(getImage());
+    public synchronized int getHeight() throws FrameInvalidError {
+        assertIsValid();
+        return image.getHeight();
     }
 
-    private boolean getIsImageValid(Image image) {
-        try {
-            // will throw an exception if the image is already closed
-            synchronized (this) { image.getFormat(); }
-            // no exception thrown, image must still be valid.
-            return true;
-        } catch (IllegalStateException e) {
-            // exception thrown, image has already been closed.
-            return false;
-        }
+    @SuppressWarnings("unused")
+    @DoNotStrip
+    public synchronized boolean getIsValid() throws FrameInvalidError {
+        assertIsValid();
+        return getIsImageValid(image);
     }
 
     @SuppressWarnings("unused")
     @DoNotStrip
-    public boolean getIsMirrored() {
+    public synchronized boolean getIsMirrored() throws FrameInvalidError {
+        assertIsValid();
         return isMirrored;
     }
 
     @SuppressWarnings("unused")
     @DoNotStrip
-    public long getTimestamp() {
+    public synchronized long getTimestamp() throws FrameInvalidError {
+        assertIsValid();
         return timestamp;
     }
 
     @SuppressWarnings("unused")
     @DoNotStrip
-    public Orientation getOrientation() {
+    public synchronized Orientation getOrientation() throws FrameInvalidError {
+        assertIsValid();
         return orientation;
     }
 
     @SuppressWarnings("unused")
     @DoNotStrip
-    public PixelFormat getPixelFormat() {
-        return PixelFormat.Companion.fromImageFormat(getImage().getFormat());
+    public synchronized PixelFormat getPixelFormat() throws FrameInvalidError {
+        assertIsValid();
+        return PixelFormat.Companion.fromImageFormat(image.getFormat());
     }
 
     @SuppressWarnings("unused")
     @DoNotStrip
-    public int getPlanesCount() {
-        return getImage().getPlanes().length;
+    public synchronized int getPlanesCount() throws FrameInvalidError {
+        assertIsValid();
+        return image.getPlanes().length;
     }
 
     @SuppressWarnings("unused")
     @DoNotStrip
-    public int getBytesPerRow() {
-        return getImage().getPlanes()[0].getRowStride();
+    public synchronized int getBytesPerRow() throws FrameInvalidError {
+        assertIsValid();
+        return image.getPlanes()[0].getRowStride();
     }
 
     @SuppressWarnings("unused")
     @DoNotStrip
-    public Object getHardwareBufferBoxed() throws HardwareBuffersNotAvailableError {
+    private Object getHardwareBufferBoxed() throws HardwareBuffersNotAvailableError, FrameInvalidError {
         return getHardwareBuffer();
     }
 
-    public HardwareBuffer getHardwareBuffer() throws HardwareBuffersNotAvailableError {
+    public synchronized HardwareBuffer getHardwareBuffer() throws HardwareBuffersNotAvailableError, FrameInvalidError {
         if (Build.VERSION.SDK_INT < Build.VERSION_CODES.P) {
             throw new HardwareBuffersNotAvailableError();
         }
-        return getImage().getHardwareBuffer();
+        assertIsValid();
+        return image.getHardwareBuffer();
     }
 
     @SuppressWarnings("unused")
     @DoNotStrip
-    public void incrementRefCount() {
-        synchronized (this) {
-            refCount++;
-        }
+    public synchronized void incrementRefCount() {
+        refCount++;
     }
 
     @SuppressWarnings("unused")
     @DoNotStrip
-    public void decrementRefCount() {
-        synchronized (this) {
-            refCount--;
-            if (refCount <= 0) {
-                // If no reference is held on this Image, close it.
-                close();
-            }
+    public synchronized void decrementRefCount() {
+        refCount--;
+        if (refCount <= 0) {
+            // If no reference is held on this Image, close it.
+            close();
         }
     }
 
-    private void close() {
-        synchronized (this) {
-            image.close();
-        }
+    private synchronized void close() {
+        image.close();
     }
 }
diff --git a/package/src/CameraError.ts b/package/src/CameraError.ts
@@ -40,6 +40,7 @@ export type CaptureError =
   | 'capture/recorder-error'
   | 'capture/video-not-enabled'
   | 'capture/photo-not-enabled'
+  | 'capture/frame-invalid'
   | 'capture/aborted'
   | 'capture/unknown'
 export type SystemError =