[Image Capture, Android] Fix various threading issues

This CL simplifies the threading of class VideoCaptureCamera2.java
and by doing so resolves potential issues caused by concurrent
access to member variables and Android video API calls.

See https://bugs.chromium.org/p/chromium/issues/detail?id=857530 for
details on issues.

The simplified model is to (still) have the constructor and public API
calls happen on a native thread, and to use a single dedicated thread
owned by the class instance to post to, do work on, and call back into
the native code.

This CL is part of a series, see Design Doc at
https://docs.google.com/document/d/1h1kva4VR1gaV3HVXaSYZFY41icfaB58j-WnHmJdyqc8/edit?usp=sharing

Bug: 857530
Change-Id: I75ffcc4a14f2395d833d80f300acef7b456676e8
Reviewed-on: https://chromium-review.googlesource.com/1117857
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576155}

media/capture/video/android/java/src/org/chromium/media/VideoCapture.java

@@ -62,14 +62,22 @@ public abstract class VideoCapture {
     @CalledByNative
     public abstract boolean allocate(int width, int height, int frameRate);
 
+    // Success is indicated by returning true and a callback to
+    // nativeOnStarted(), which may occur synchronously or asynchronously.
+    // Failure can be indicated by one of the following:
+    // * Returning false. In this case no callback to nativeOnStarted() is made.
+    // * Returning true, and asynchronously invoking nativeOnError. In this case
+    //   also no callback to nativeOnStarted() is made.
     @CalledByNative
-    public abstract boolean startCapture();
+    public abstract boolean startCaptureMaybeAsync();
 
+    // Blocks until it is guaranteed that no more frames are sent.
     @CalledByNative
-    public abstract boolean stopCapture();
+    public abstract boolean stopCaptureAndBlockUntilStopped();
 
+    // Replies by calling nativeOnGetPhotoCapabilitiesReply().
     @CalledByNative
-    public abstract PhotoCapabilities getPhotoCapabilities();
+    public abstract void getPhotoCapabilitiesAsync(long callbackId);
 
     /**
      * @param zoom Zoom level, should be ignored if 0.
@@ -95,8 +103,9 @@ public abstract class VideoCapture {
             boolean hasRedEyeReduction, boolean redEyeReduction, int fillLightMode,
             boolean hasTorch, boolean torch, double colorTemperature);
 
+    // Replies by calling nativeOnPhotoTaken().
     @CalledByNative
-    public abstract boolean takePhoto(final long callbackId);
+    public abstract void takePhotoAsync(long callbackId);
 
     @CalledByNative
     public abstract void deallocate();
@@ -164,6 +173,12 @@ public abstract class VideoCapture {
         return orientation;
     }
 
+    // {@link nativeOnPhotoTaken()} needs to be called back if there's any
+    // problem after {@link takePhotoAsync()} has returned true.
+    protected void notifyTakePhotoError(long callbackId) {
+        nativeOnPhotoTaken(mNativeVideoCaptureDeviceAndroid, callbackId, null);
+    }
+
     /**
      * Finds the framerate range matching |targetFramerate|. Tries to find a range with as low of a
      * minimum value as possible to allow the camera adjust based on the lighting conditions.
@@ -230,10 +245,17 @@ public abstract class VideoCapture {
     // Method for VideoCapture implementations to signal an asynchronous error.
     public native void nativeOnError(long nativeVideoCaptureDeviceAndroid, String message);
 
-    // Method for VideoCapture implementations to send Photos back to.
+    public native void nativeOnGetPhotoCapabilitiesReply(
+            long nativeVideoCaptureDeviceAndroid, long callbackId, PhotoCapabilities result);
+
+    // Callback for calls to takePhoto(). This can indicate both success and
+    // failure. Failure is indicated by |data| being null.
     public native void nativeOnPhotoTaken(
             long nativeVideoCaptureDeviceAndroid, long callbackId, byte[] data);
 
     // Method for VideoCapture implementations to report device started event.
     public native void nativeOnStarted(long nativeVideoCaptureDeviceAndroid);
+
+    public native void nativeDCheckCurrentlyOnIncomingTaskRunner(
+            long nativeVideoCaptureDeviceAndroid);
 }

media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java

@@ -146,8 +146,7 @@ public class VideoCaptureCamera
 
             synchronized (mPhotoTakenCallbackLock) {
                 if (mPhotoTakenCallbackId == 0) return;
-                nativeOnPhotoTaken(
-                        mNativeVideoCaptureDeviceAndroid, mPhotoTakenCallbackId, new byte[0]);
+                notifyTakePhotoError(mPhotoTakenCallbackId);
                 mPhotoTakenCallbackId = 0;
             }
         }
@@ -425,9 +424,9 @@ public class VideoCaptureCamera
     }
 
     @Override
-    public boolean startCapture() {
+    public boolean startCaptureMaybeAsync() {
         if (mCamera == null) {
-            Log.e(TAG, "startCapture: mCamera is null");
+            Log.e(TAG, "startCaptureAsync: mCamera is null");
             return false;
         }
 
@@ -444,7 +443,7 @@ public class VideoCaptureCamera
         try {
             mCamera.startPreview();
         } catch (RuntimeException ex) {
-            Log.e(TAG, "startCapture: Camera.startPreview: " + ex);
+            Log.e(TAG, "startCaptureAsync: Camera.startPreview: " + ex);
             return false;
         }
 
@@ -459,9 +458,9 @@ public class VideoCaptureCamera
     }
 
     @Override
-    public boolean stopCapture() {
+    public boolean stopCaptureAndBlockUntilStopped() {
         if (mCamera == null) {
-            Log.e(TAG, "stopCapture: mCamera is null");
+            Log.e(TAG, "stopCaptureAndBlockUntilStopped: mCamera is null");
             return true;
         }
 
@@ -481,7 +480,7 @@ public class VideoCaptureCamera
     }
 
     @Override
-    public PhotoCapabilities getPhotoCapabilities() {
+    public void getPhotoCapabilitiesAsync(long callbackId) {
         final android.hardware.Camera.Parameters parameters = getCameraParameters(mCamera);
         PhotoCapabilities.Builder builder = new PhotoCapabilities.Builder();
         Log.i(TAG, " CAM params: %s", parameters.flatten());
@@ -634,7 +633,8 @@ public class VideoCaptureCamera
             builder.setFillLightModes(integerArrayListToArray(modes));
         }
 
-        return builder.build();
+        nativeOnGetPhotoCapabilitiesReply(
+                mNativeVideoCaptureDeviceAndroid, callbackId, builder.build());
     }
 
     @Override
@@ -776,15 +776,19 @@ public class VideoCaptureCamera
     }
 
     @Override
-    public boolean takePhoto(final long callbackId) {
+    public void takePhotoAsync(final long callbackId) {
         if (mCamera == null || !mIsRunning) {
-            Log.e(TAG, "takePhoto: mCamera is null or is not running");
-            return false;
+            Log.e(TAG, "takePhotoAsync: mCamera is null or is not running");
+            notifyTakePhotoError(callbackId);
+            return;
         }
 
         // Only one picture can be taken at once.
         synchronized (mPhotoTakenCallbackLock) {
-            if (mPhotoTakenCallbackId != 0) return false;
+            if (mPhotoTakenCallbackId != 0) {
+                notifyTakePhotoError(callbackId);
+                return;
+            }
             mPhotoTakenCallbackId = callbackId;
         }
         mPreviewParameters = getCameraParameters(mCamera);
@@ -817,18 +821,18 @@ public class VideoCaptureCamera
             mCamera.setParameters(photoParameters);
         } catch (RuntimeException ex) {
             Log.e(TAG, "setParameters " + ex);
-            return false;
+            notifyTakePhotoError(callbackId);
+            return;
         }
 
         mCamera.takePicture(null, null, null, new CrPictureCallback());
-        return true;
     }
 
     @Override
     public void deallocate() {
         if (mCamera == null) return;
 
-        stopCapture();
+        stopCaptureAndBlockUntilStopped();
         try {
             mCamera.setPreviewTexture(null);
             if (mGlTextures != null) GLES20.glDeleteTextures(1, mGlTextures, 0);

media/capture/video/android/video_capture_device_android.h

@@ -21,7 +21,7 @@
 namespace base {
 class Location;
 class SingleThreadTaskRunner;
-}
+}  // namespace base
 
 namespace media {
 
@@ -91,6 +91,12 @@ class CAPTURE_EXPORT VideoCaptureDeviceAndroid : public VideoCaptureDevice {
                const base::android::JavaParamRef<jobject>& obj,
                const base::android::JavaParamRef<jstring>& message);
 
+  void OnGetPhotoCapabilitiesReply(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj,
+      jlong callback_id,
+      jobject photo_capabilities);
+
   // Implement org.chromium.media.VideoCapture.nativeOnPhotoTaken.
   void OnPhotoTaken(JNIEnv* env,
                     const base::android::JavaParamRef<jobject>& obj,
@@ -100,6 +106,12 @@ class CAPTURE_EXPORT VideoCaptureDeviceAndroid : public VideoCaptureDevice {
   // Implement org.chromium.media.VideoCapture.nativeOnStarted.
   void OnStarted(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
 
+  // Implement
+  // org.chromium.media.VideoCapture.nativeDCheckCurrentlyOnIncomingTaskRunner.
+  void DCheckCurrentlyOnIncomingTaskRunner(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj);
+
   void ConfigureForTesting();
 
  protected:
@@ -152,9 +164,10 @@ class CAPTURE_EXPORT VideoCaptureDeviceAndroid : public VideoCaptureDevice {
   base::TimeTicks expected_next_frame_time_;
   base::TimeDelta frame_interval_;
 
-  // List of |photo_callbacks_| in flight, being served in Java side.
+  // List of callbacks for photo API in flight, being served in Java side.
   base::Lock photo_callbacks_lock_;
-  std::list<std::unique_ptr<TakePhotoCallback>> photo_callbacks_;
+  std::list<std::unique_ptr<GetPhotoStateCallback>> get_photo_state_callbacks_;
+  std::list<std::unique_ptr<TakePhotoCallback>> take_photo_callbacks_;
 
   const VideoCaptureDeviceDescriptor device_descriptor_;
   VideoCaptureFormat capture_format_;