VideoCaptureDeviceAndroid: add ThreadChecker, lock access to |client_|, mini cleanup

media/capture/video/android/video_capture_device_android.cc

Index: media/capture/video/android/video_capture_device_android.cc
diff --git a/media/capture/video/android/video_capture_device_android.cc b/media/capture/video/android/video_capture_device_android.cc
index 715f8f4c966eacb91e7d918024e9b8191f127d03..5c26b38e78dfe9b16e6d6fcf5be2104a0670ff86 100644
--- a/media/capture/video/android/video_capture_device_android.cc
+++ b/media/capture/video/android/video_capture_device_android.cc
@@ -43,6 +43,7 @@ VideoCaptureDeviceAndroid::VideoCaptureDeviceAndroid(const Name& device_name)
 }
 
 VideoCaptureDeviceAndroid::~VideoCaptureDeviceAndroid() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   StopAndDeAllocate();
 }
 
@@ -59,7 +60,7 @@ bool VideoCaptureDeviceAndroid::Init() {
 void VideoCaptureDeviceAndroid::AllocateAndStart(
     const VideoCaptureParams& params,
     std::unique_ptr<Client> client) {
-  DVLOG(1) << __FUNCTION__;
+  DCHECK(thread_checker_.CalledOnValidThread());
   {
     base::AutoLock lock(lock_);
     if (state_ != kIdle)
@@ -79,7 +80,6 @@ void VideoCaptureDeviceAndroid::AllocateAndStart(
     return;
   }
 
-  // Store current width and height.
   capture_format_.frame_size.SetSize(
       Java_VideoCapture_queryWidth(env, j_capture_.obj()),
       Java_VideoCapture_queryHeight(env, j_capture_.obj()));
@@ -97,9 +97,9 @@ void VideoCaptureDeviceAndroid::AllocateAndStart(
         capture_format_.frame_rate);
   }
 
-  DVLOG(1) << "VideoCaptureDeviceAndroid::Allocate: queried frame_size="
-           << capture_format_.frame_size.ToString()
-           << ", frame_rate=" << capture_format_.frame_rate;
+  DVLOG(1) << __FUNCTION__ << " requested ("
+           << capture_format_.frame_size.ToString() << ")@ "
+           << capture_format_.frame_rate << "fps";
 
   ret = Java_VideoCapture_startCapture(env, j_capture_.obj());
   if (!ret) {
@@ -114,7 +114,7 @@ void VideoCaptureDeviceAndroid::AllocateAndStart(
 }
 
 void VideoCaptureDeviceAndroid::StopAndDeAllocate() {
-  DVLOG(1) << __FUNCTION__;
+  DCHECK(thread_checker_.CalledOnValidThread());
   {
     base::AutoLock lock(lock_);
     if (state_ != kCapturing && state_ != kError)
@@ -123,7 +123,7 @@ void VideoCaptureDeviceAndroid::StopAndDeAllocate() {
 
   JNIEnv* env = AttachCurrentThread();
 
-  jboolean ret = Java_VideoCapture_stopCapture(env, j_capture_.obj());
+  const jboolean ret = Java_VideoCapture_stopCapture(env, j_capture_.obj());
   if (!ret) {
     SetErrorState(FROM_HERE, "failed to stop capture");
     return;
@@ -139,7 +139,7 @@ void VideoCaptureDeviceAndroid::StopAndDeAllocate() {
 }
 
 void VideoCaptureDeviceAndroid::TakePhoto(TakePhotoCallback callback) {
-  DVLOG(1) << __FUNCTION__;
+  DCHECK(thread_checker_.CalledOnValidThread());
   {
     base::AutoLock lock(lock_);
     if (state_ != kCapturing)
@@ -165,6 +165,7 @@ void VideoCaptureDeviceAndroid::TakePhoto(TakePhotoCallback callback) {
 
 void VideoCaptureDeviceAndroid::GetPhotoCapabilities(
     GetPhotoCapabilitiesCallback callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   JNIEnv* env = AttachCurrentThread();
 
   PhotoCapabilities caps(
@@ -199,6 +200,7 @@ void VideoCaptureDeviceAndroid::GetPhotoCapabilities(
 void VideoCaptureDeviceAndroid::SetPhotoOptions(
     mojom::PhotoSettingsPtr settings,
     SetPhotoOptionsCallback callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   JNIEnv* env = AttachCurrentThread();
   // |width| and/or |height| are kept for the next TakePhoto()s.
   if (settings->has_width || settings->has_height)
@@ -225,9 +227,11 @@ void VideoCaptureDeviceAndroid::OnFrameAvailable(
     jint rotation) {
   DVLOG(3) << __FUNCTION__ << " length =" << length;
 
-  base::AutoLock lock(lock_);
-  if (state_ != kCapturing || !client_.get())
-    return;
+  {
+    base::AutoLock lock(lock_);
+    if (state_ != kCapturing || !client_.get())
+      return;
+  }

[chfremer, 2016/07/27 17:07:59]

Not sure we really can reduce the scope of the lock like this.
Couldn't it happen that between this position and the usage of |client_| in line
257 a call to StopAndDeallocate() arrives that resets |client_| to nullptr? 

And if this were the case, I would also be concerned about the access to
|client_| at lines 311 and 378 that are not protected by a lock.

[mcasas, 2016/07/27 18:40:54]

You're right, done.

 
   jbyte* buffer = env->GetByteArrayElements(data, NULL);
   if (!buffer) {