ImageCapture: Queue up requests while device not ready

media/capture/video/android/video_capture_device_android.h

Index: media/capture/video/android/video_capture_device_android.h
diff --git a/media/capture/video/android/video_capture_device_android.h b/media/capture/video/android/video_capture_device_android.h
index 9a637a39eb1af496995747d86fdca0e43586bb4c..9788d1f4e8771c242e9b253be986e30c70fcbac6 100644
--- a/media/capture/video/android/video_capture_device_android.h
+++ b/media/capture/video/android/video_capture_device_android.h
@@ -10,6 +10,7 @@
 
 #include "base/android/scoped_java_ref.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_checker.h"
@@ -17,9 +18,13 @@
 #include "media/capture/capture_export.h"
 #include "media/capture/video/video_capture_device.h"
 
+namespace base {
+class SingleThreadTaskRunner;
+}
+
 namespace tracked_objects {
 class Location;
-}  // namespace tracked_
+}  // namespace tracked_objects
 
 namespace media {
 
@@ -97,28 +102,37 @@ class CAPTURE_EXPORT VideoCaptureDeviceAndroid : public VideoCaptureDevice {
 
  private:
   enum InternalState {
-    kIdle,       // The device is opened but not in use.
-    kCapturing,  // Video is being captured.
-    kError       // Hit error. User needs to recover by destroying the object.
+    kIdle,        // The device is opened but not in use.
+    kConfigured,  // The device has been AllocateAndStart()ed.
+    kError        // Hit error. User needs to recover by destroying the object.
   };
 
   VideoPixelFormat GetColorspace();
   void SetErrorState(const tracked_objects::Location& from_here,
                      const std::string& reason);
 
-  base::ThreadChecker thread_checker_;
+  void DoGetPhotoCapabilities(GetPhotoCapabilitiesCallback callback);
+  void DoSetPhotoOptions(mojom::PhotoSettingsPtr settings,
+                         SetPhotoOptionsCallback callback);
+  void DoTakePhoto(TakePhotoCallback callback);
+
+  // Thread on which we are created.
+  const scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;

[emircan, 2016/08/03 21:55:54]

Can we use SequencedTaskRunner and SequencedTaskRunnerHandle instead? AFAIU,
there isn't a reason why this would have to be pinned on a single thread.

[mcasas, 2016/08/03 22:19:36]

SingleThreadTaskRunner is more pervasive, i.e. 167 [1] vs 27 for
SequencedTaskRunner [2] (in content/). Reading the comments 
in [3] I think it applies to our case: this variable captures the
construction thread (== owning thread), that should not change
during its lifetime.

[1] grep -rn "scoped_refptr<base::SingleThreadTaskRunner>" content/ | grep "_;"
|wc -l
[2] grep -rn "scoped_refptr<base::SequencedTaskRunner>" content/ | grep "_;" |
wc -l
[3]
https://cs.chromium.org/chromium/src/base/sequenced_task_runner.h?q=Sequenced...

[emircan, 2016/08/03 22:52:59]

Acknowledged.

 
-  // Prevent racing on accessing |state_| and |client_| since both could be
-  // accessed from different threads.
+  // |lock_| protects |state_|, |client_| and |got_first_frame_|,

[emircan, 2016/08/03 21:55:54]

also |photo_requests_queue_|

[mcasas, 2016/08/03 22:19:37]

Done.

   base::Lock lock_;
   InternalState state_;
   std::unique_ptr<VideoCaptureDevice::Client> client_;
-
   bool got_first_frame_;

[emircan, 2016/08/03 21:55:54]

Can this be an InternalState such as kCapturing?

[mcasas, 2016/08/03 22:19:37]

Hmm it could be it would mix two things: |state_| 
represents different possible transitions, whereas
this binary flag is a flip-flop: stays false until it becomes
true and then stays true forever. Mixing both might
make it less evident their purpose.

[emircan, 2016/08/03 22:52:59]

Acknowledged.

+
   base::TimeTicks expected_next_frame_time_;
   base::TimeTicks first_ref_time_;
   base::TimeDelta frame_interval_;
 
+  // Photo-related requests waiting for |got_first_frame_| to be served. Android
+  // APIs need the device capturing or nearly-capturing to be fully oeprational.
+  std::list<base::Closure> photo_requests_queue_;
+
   // List of |photo_callbacks_| in flight, being served in Java side.
   base::Lock photo_callbacks_lock_;
   std::list<std::unique_ptr<TakePhotoCallback>> photo_callbacks_;
@@ -131,6 +145,8 @@ class CAPTURE_EXPORT VideoCaptureDeviceAndroid : public VideoCaptureDevice {
   // Java VideoCaptureAndroid instance.
   base::android::ScopedJavaLocalRef<jobject> j_capture_;
 
+  base::WeakPtrFactory<VideoCaptureDeviceAndroid> weak_ptr_factory_;
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(VideoCaptureDeviceAndroid);
 };