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

media/capture/video/android/video_capture_device_android.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/capture/video/android/video_capture_device_android.h"
 
 #include <stdint.h>
 #include <utility>
 
 #include "base/android/jni_android.h"
@@ skipping 25 matching lines @@
   // only used for USB model identifiers, so this implementation just indicates
   // an unknown device model.
   return "";
 }
 
 VideoCaptureDeviceAndroid::VideoCaptureDeviceAndroid(const Name& device_name)
     : state_(kIdle), got_first_frame_(false), device_name_(device_name) {
 }
 
 VideoCaptureDeviceAndroid::~VideoCaptureDeviceAndroid() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   StopAndDeAllocate();
 }
 
 bool VideoCaptureDeviceAndroid::Init() {
   int id;
   if (!base::StringToInt(device_name_.id(), &id))
     return false;
 
   j_capture_.Reset(VideoCaptureDeviceFactoryAndroid::createVideoCaptureAndroid(
       id, reinterpret_cast<intptr_t>(this)));
   return true;
 }
 
 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)
       return;
     client_ = std::move(client);
     got_first_frame_ = false;
   }
 
   JNIEnv* env = AttachCurrentThread();
 
   jboolean ret = Java_VideoCapture_allocate(
       env, j_capture_.obj(), params.requested_format.frame_size.width(),
       params.requested_format.frame_size.height(),
       params.requested_format.frame_rate);
   if (!ret) {
     SetErrorState(FROM_HERE, "failed to allocate");
     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()));
   capture_format_.frame_rate =
       Java_VideoCapture_queryFrameRate(env, j_capture_.obj());
   capture_format_.pixel_format = GetColorspace();
   DCHECK_NE(capture_format_.pixel_format, media::PIXEL_FORMAT_UNKNOWN);
   CHECK(capture_format_.frame_size.GetArea() > 0);
   CHECK(!(capture_format_.frame_size.width() % 2));
   CHECK(!(capture_format_.frame_size.height() % 2));
 
   if (capture_format_.frame_rate > 0) {
     frame_interval_ = base::TimeDelta::FromMicroseconds(
         (base::Time::kMicrosecondsPerSecond + capture_format_.frame_rate - 1) /
         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) {
     SetErrorState(FROM_HERE, "failed to start capture");
     return;
   }
 
   {
     base::AutoLock lock(lock_);
     state_ = kCapturing;
   }
 }
 
 void VideoCaptureDeviceAndroid::StopAndDeAllocate() {
-  DVLOG(1) << __FUNCTION__;
+  DCHECK(thread_checker_.CalledOnValidThread());
   {
     base::AutoLock lock(lock_);
     if (state_ != kCapturing && state_ != kError)
       return;
   }
 
   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;
   }
 
   {
     base::AutoLock lock(lock_);
     state_ = kIdle;
     client_.reset();
   }
 
   Java_VideoCapture_deallocate(env, j_capture_.obj());
 }
 
 void VideoCaptureDeviceAndroid::TakePhoto(TakePhotoCallback callback) {
-  DVLOG(1) << __FUNCTION__;
+  DCHECK(thread_checker_.CalledOnValidThread());
   {
     base::AutoLock lock(lock_);
     if (state_ != kCapturing)
       return;
   }
 
   JNIEnv* env = AttachCurrentThread();
 
   // Make copy on the heap so we can pass the pointer through JNI.
   std::unique_ptr<TakePhotoCallback> heap_callback(
       new TakePhotoCallback(std::move(callback)));
   const intptr_t callback_id = reinterpret_cast<intptr_t>(heap_callback.get());
   if (!Java_VideoCapture_takePhoto(env, j_capture_.obj(), callback_id,
                                    next_photo_resolution_.width(),
                                    next_photo_resolution_.height()))
     return;
 
   {
     base::AutoLock lock(photo_callbacks_lock_);
     photo_callbacks_.push_back(std::move(heap_callback));
   }
 }
 
 void VideoCaptureDeviceAndroid::GetPhotoCapabilities(
     GetPhotoCapabilitiesCallback callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   JNIEnv* env = AttachCurrentThread();
 
   PhotoCapabilities caps(
       Java_VideoCapture_getPhotoCapabilities(env, j_capture_.obj()));
 
   // TODO(mcasas): Manual member copying sucks, consider adding typemapping from
   // PhotoCapabilities to mojom::PhotoCapabilitiesPtr, https://crbug.com/622002.
   mojom::PhotoCapabilitiesPtr photo_capabilities =
       mojom::PhotoCapabilities::New();
   photo_capabilities->iso = mojom::Range::New();
@@ skipping 14 matching lines @@
   photo_capabilities->zoom->min = caps.getMinZoom();
   photo_capabilities->focus_mode = caps.getAutoFocusInUse()
                                        ? mojom::FocusMode::AUTO
                                        : mojom::FocusMode::MANUAL;
   callback.Run(std::move(photo_capabilities));
 }
 
 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)
     next_photo_resolution_.SetSize(0, 0);
   if (settings->has_width) {
     next_photo_resolution_.set_width(
         base::saturated_cast<int>(settings->width));
   }
   if (settings->has_height) {
     next_photo_resolution_.set_height(
         base::saturated_cast<int>(settings->height));
   }
 
   if (settings->has_zoom)
     Java_VideoCapture_setZoom(env, j_capture_.obj(), settings->zoom);
   callback.Run(true);
 }
 
 void VideoCaptureDeviceAndroid::OnFrameAvailable(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jbyteArray>& data,
     jint length,
     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) {
     LOG(ERROR) << "VideoCaptureDeviceAndroid::OnFrameAvailable: "
                   "failed to GetByteArrayElements";
     return;
   }
 
   const base::TimeTicks current_time = base::TimeTicks::Now();
   if (!got_first_frame_) {
@@ skipping 127 matching lines @@
     const tracked_objects::Location& from_here,
     const std::string& reason) {
   {
     base::AutoLock lock(lock_);
     state_ = kError;
   }
   client_->OnError(from_here, reason);
 }
 
 }  // namespace media