Moved ownership of camera thread to user image screen.

Patch of http://codereview.chromium.org/4106015/show

BUG=chromium-os:8440
TEST=Ensure that there is no crash after taking picture. Try several times, as this is timing related.

Review URL: http://codereview.chromium.org/4228002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@64800 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 44 insertions, 66 deletions

diff --git a/chrome/browser/chromeos/login/camera.cc b/chrome/browser/chromeos/login/camera.cc
index 1fb85f8e..59e77d3 100644
--- a/chrome/browser/chromeos/login/camera.cc
+++ b/chrome/browser/chromeos/login/camera.cc
@@ -22,6 +22,7 @@
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
+#include "base/thread.h"
 #include "base/time.h"
 #include "chrome/browser/browser_thread.h"
 #include "gfx/size.h"
@@ -113,8 +114,6 @@ gfx::Size get_best_frame_size(int fd,
 
 // Default camera device name.
 const char kDeviceName[] = "/dev/video0";
-// Name for camera thread.
-const char kCameraThreadName[] = "Chrome_CameraThread";
 // Default width of each frame received from the camera.
 const int kFrameWidth = 640;
 // Default height of each frame received from the camera.
@@ -126,18 +125,12 @@ const long kSelectTimeout = 1 * base::Time::kMicrosecondsPerSecond;
 
 }  // namespace
 
-// static
-Lock Camera::image_lock_;
-
-// static
-Lock Camera::thread_lock_;
-
 ///////////////////////////////////////////////////////////////////////////////
 // Camera, public members:
 
-Camera::Camera(Delegate* delegate, bool mirrored)
+Camera::Camera(Delegate* delegate, base::Thread* thread, bool mirrored)
     : delegate_(delegate),
-      camera_thread_(kCameraThreadName),
+      thread_(thread),
       device_name_(kDeviceName),
       device_descriptor_(-1),
       is_capturing_(false),
@@ -152,7 +145,6 @@ Camera::~Camera() {
   DCHECK_EQ(-1, device_descriptor_) << "Don't forget to uninitialize camera.";
 }
 
-// If this method is called there's no need to call PostCameraThreadAck().
 void Camera::ReportFailure() {
   DCHECK(IsOnCameraThread());
   if (device_descriptor_ == -1) {
@@ -179,6 +171,7 @@ void Camera::ReportFailure() {
 void Camera::Initialize(int desired_width, int desired_height) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   PostCameraTask(
+      FROM_HERE,
       NewRunnableMethod(this,
                         &Camera::DoInitialize,
                         desired_width,
@@ -187,11 +180,9 @@ void Camera::Initialize(int desired_width, int desired_height) {
 
 void Camera::DoInitialize(int desired_width, int desired_height) {
   DCHECK(IsOnCameraThread());
-  DCHECK(delegate_);
 
   if (device_descriptor_ != -1) {
     LOG(WARNING) << "Camera is initialized already.";
-    PostCameraThreadAck();
     return;
   }
 
@@ -247,34 +238,21 @@ void Camera::DoInitialize(int desired_width, int desired_height) {
   frame_height_ = frame_size.height();
   desired_width_ = desired_width;
   desired_height_ = desired_height;
-  // No need to call PostCameraThreadAck() back as this method
-  // is being posted instead.
   BrowserThread::PostTask(
       BrowserThread::UI,
       FROM_HERE,
       NewRunnableMethod(this, &Camera::OnInitializeSuccess));
 }
 
-void Camera::CameraThreadAck() {
-}
-
-void Camera::PostCameraThreadAck() {
-  BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      NewRunnableMethod(this, &Camera::CameraThreadAck));
-}
-
 void Camera::Uninitialize() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  PostCameraTask(NewRunnableMethod(this, &Camera::DoUninitialize));
+  PostCameraTask(FROM_HERE, NewRunnableMethod(this, &Camera::DoUninitialize));
 }
 
 void Camera::DoUninitialize() {
   DCHECK(IsOnCameraThread());
   if (device_descriptor_ == -1) {
     LOG(WARNING) << "Calling uninitialize for uninitialized camera.";
-    PostCameraThreadAck();
     return;
   }
   DoStopCapturing();
@@ -282,20 +260,18 @@ void Camera::DoUninitialize() {
   if (close(device_descriptor_) == -1)
     log_errno("Closing the device failed.");
   device_descriptor_ = -1;
-  // Maintain a reference so that camera object isn't deleted on wrong thread.
-  PostCameraThreadAck();
 }
 
 void Camera::StartCapturing() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  PostCameraTask(NewRunnableMethod(this, &Camera::DoStartCapturing));
+  PostCameraTask(FROM_HERE,
+                 NewRunnableMethod(this, &Camera::DoStartCapturing));
 }
 
 void Camera::DoStartCapturing() {
   DCHECK(IsOnCameraThread());
   if (is_capturing_) {
     LOG(WARNING) << "Capturing is already started.";
-    PostCameraThreadAck();
     return;
   }
 
@@ -324,18 +300,19 @@ void Camera::DoStartCapturing() {
       NewRunnableMethod(this,
                         &Camera::OnStartCapturingSuccess));
   is_capturing_ = true;
-  PostCameraTask(NewRunnableMethod(this, &Camera::OnCapture));
+  PostCameraTask(FROM_HERE,
+                 NewRunnableMethod(this, &Camera::OnCapture));
 }
 
 void Camera::StopCapturing() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  PostCameraTask(NewRunnableMethod(this, &Camera::DoStopCapturing));
+  PostCameraTask(FROM_HERE,
+                 NewRunnableMethod(this, &Camera::DoStopCapturing));
 }
 
 void Camera::DoStopCapturing() {
   DCHECK(IsOnCameraThread());
   if (!is_capturing_) {
-    PostCameraThreadAck();
     LOG(WARNING) << "Calling StopCapturing when capturing is not started.";
     return;
   }
@@ -344,8 +321,6 @@ void Camera::DoStopCapturing() {
   v4l2_buf_type type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
   if (xioctl(device_descriptor_, VIDIOC_STREAMOFF, &type) == -1)
     log_errno("VIDIOC_STREAMOFF failed.");
-  // Maintain a reference so that camera object isn't deleted on wrong thread.
-  PostCameraThreadAck();
 }
 
 void Camera::GetFrame(SkBitmap* frame) {
@@ -430,11 +405,8 @@ void Camera::UnmapVideoBuffers() {
 
 void Camera::OnCapture() {
   DCHECK(IsOnCameraThread());
-  if (!is_capturing_) {
-    // Maintain a reference so that camera object isn't deleted on wrong thread.
-    PostCameraThreadAck();
+  if (!is_capturing_)
     return;
-  }
 
   do {
     fd_set fds;
@@ -461,7 +433,8 @@ void Camera::OnCapture() {
     // EAGAIN - continue select loop.
   } while (!ReadFrame());
 
-  PostCameraTask(NewRunnableMethod(this, &Camera::OnCapture));
+  PostCameraTask(FROM_HERE,
+                 NewRunnableMethod(this, &Camera::OnCapture));
 }
 
 bool Camera::ReadFrame() {
@@ -601,14 +574,16 @@ void Camera::OnCaptureFailure() {
 
 bool Camera::IsOnCameraThread() const {
   AutoLock lock(thread_lock_);
-  return MessageLoop::current() == camera_thread_.message_loop();
+  return thread_ && MessageLoop::current() == thread_->message_loop();
 }
 
-void Camera::PostCameraTask(Task* task) {
+void Camera::PostCameraTask(const tracked_objects::Location& from_here,
+                            Task* task) {
   AutoLock lock(thread_lock_);
-  if (!camera_thread_.IsRunning())
-    camera_thread_.Start();
-  camera_thread_.message_loop()->PostTask(FROM_HERE, task);
+  if (!thread_)
+    return;
+  DCHECK(thread_->IsRunning());
+  thread_->message_loop()->PostTask(from_here, task);
 }
 
 }  // namespace chromeos
diff --git a/chrome/browser/chromeos/login/camera.h b/chrome/browser/chromeos/login/camera.h
index af70e62..a82f197 100644
--- a/chrome/browser/chromeos/login/camera.h
+++ b/chrome/browser/chromeos/login/camera.h
@@ -16,7 +16,7 @@
 
 class Task;
 namespace base {
-class TimeDelta;
+class Thread;
 }  // namespace base
 
 namespace chromeos {
@@ -49,9 +49,10 @@ class Camera : public base::RefCountedThreadSafe<Camera> {
   };
 
   // Initializes object members. |delegate| is object that will receive
-  // notifications about success of async method calls. |mirrored|
-  // determines if the returned video image is mirrored horizontally.
-  Camera(Delegate* delegate, bool mirrored);
+  // notifications about success of async method calls. |thread| is a thread
+  // to post blocking tasks to. |mirrored| determines if the returned video
+  // image is mirrored horizontally.
+  Camera(Delegate* delegate, base::Thread* thread, bool mirrored);
 
   // Initializes camera device on camera thread. Corresponding delegate's
   // callback is called on UI thread to notify about success or failure. Does
@@ -111,6 +112,8 @@ class Camera : public base::RefCountedThreadSafe<Camera> {
   // See the corresponding methods without Do prefix for details.
   void DoInitialize(int desired_width, int desired_height);
   void DoStartCapturing();
+  void DoUninitialize();
+  void DoStopCapturing();
 
   // Helper method that reports failure to the delegate via method
   // corresponding to the current state of the object.
@@ -124,22 +127,13 @@ class Camera : public base::RefCountedThreadSafe<Camera> {
   void OnCaptureSuccess();
   void OnCaptureFailure();
 
-  // Camera thread routines that implement the corresponding public methods.
-  void DoUninitialize();
-  void DoStopCapturing();
-
-  // All methods on the camera thread need to call this to post back to the UI
-  // thread. Otherwise the camera object could be deleted on the camera thread
-  // which is not allowed.
-  void PostCameraThreadAck();
-  void CameraThreadAck();
-
   // Returns true if the code is executed on camera thread right now, false
   // otherwise.
   bool IsOnCameraThread() const;
 
   // Posts task to camera thread.
-  void PostCameraTask(Task* task);
+  void PostCameraTask(const tracked_objects::Location& from_here,
+                      Task* task);
 
   // Defines a buffer in memory where one frame from the camera is stored.
   struct VideoBuffer {
@@ -152,7 +146,7 @@ class Camera : public base::RefCountedThreadSafe<Camera> {
   Delegate* delegate_;
 
   // Thread where all work with the device is going on.
-  base::Thread camera_thread_;
+  base::Thread* thread_;
 
   // All the members below are accessed only on camera thread.
   // Name of the device file, i.e. "/dev/video0".
@@ -187,10 +181,10 @@ class Camera : public base::RefCountedThreadSafe<Camera> {
   SkBitmap frame_image_;
 
   // Lock that guards references to |frame_image_|.
-  static Lock image_lock_;
+  mutable Lock image_lock_;
 
   // Lock that guards references to |camera_thread_|.
-  static Lock thread_lock_;
+  mutable Lock thread_lock_;
 
   DISALLOW_COPY_AND_ASSIGN(Camera);
 };
diff --git a/chrome/browser/chromeos/login/user_image_screen.cc b/chrome/browser/chromeos/login/user_image_screen.cc
index 3aa68b0..9016652 100644
--- a/chrome/browser/chromeos/login/user_image_screen.cc
+++ b/chrome/browser/chromeos/login/user_image_screen.cc
@@ -31,17 +31,22 @@ const int kMaxCaptureFailureCounter = 5;
 // Maximum number of camera initialization retries.
 const int kMaxCameraInitFailureCounter = 3;
 
+// Name for camera thread.
+const char kCameraThreadName[] = "Chrome_CameraThread";
+
 }  // namespace
 
 UserImageScreen::UserImageScreen(WizardScreenDelegate* delegate)
     : ViewScreen<UserImageView>(delegate),
-      ALLOW_THIS_IN_INITIALIZER_LIST(camera_(new Camera(this, true))),
       capture_failure_counter_(0),
-      camera_init_failure_counter_(0) {
+      camera_init_failure_counter_(0),
+      camera_thread_(kCameraThreadName) {
   registrar_.Add(
       this,
       NotificationType::SCREEN_LOCK_STATE_CHANGED,
       NotificationService::AllSources());
+  camera_thread_.Start();
+  camera_ = new Camera(this, &camera_thread_, true);
   camera_->Initialize(kFrameWidth, kFrameHeight);
 }
 
diff --git a/chrome/browser/chromeos/login/user_image_screen.h b/chrome/browser/chromeos/login/user_image_screen.h
index bc4f984..ce02047 100644
--- a/chrome/browser/chromeos/login/user_image_screen.h
+++ b/chrome/browser/chromeos/login/user_image_screen.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_CHROMEOS_LOGIN_USER_IMAGE_SCREEN_H_
 #pragma once
 
+#include "base/thread.h"
 #include "chrome/browser/chromeos/login/camera.h"
 #include "chrome/browser/chromeos/login/user_image_view.h"
 #include "chrome/browser/chromeos/login/view_screen.h"
@@ -58,6 +59,9 @@ class UserImageScreen: public ViewScreen<UserImageView>,
   // Counts how many times camera initialization failed.
   int camera_init_failure_counter_;
 
+  // Thread for camera to work on.
+  base::Thread camera_thread_;
+
   NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(UserImageScreen);