Refactor video capturing code in the render process

content/renderer/media/video_capture_impl.cc

Index: content/renderer/media/video_capture_impl.cc
diff --git a/content/renderer/media/video_capture_impl.cc b/content/renderer/media/video_capture_impl.cc
index b9a61f3d05b4db4a0b24afebb01adde58acd056a..51df96998f742674f353f87e1f7ec30dd0f2ef3c 100644
--- a/content/renderer/media/video_capture_impl.cc
+++ b/content/renderer/media/video_capture_impl.cc
@@ -32,9 +32,8 @@ class VideoCaptureImpl::ClientBuffer
   DISALLOW_COPY_AND_ASSIGN(ClientBuffer);
 };
 
-bool VideoCaptureImpl::CaptureStarted() {
-  return state_ == VIDEO_CAPTURE_STATE_STARTED;
-}
+VideoCaptureImpl::ClientInfo::ClientInfo() {}
+VideoCaptureImpl::ClientInfo::~ClientInfo() {}
 
 int VideoCaptureImpl::CaptureFrameRate() {
   return last_frame_format_.frame_rate;
@@ -43,118 +42,75 @@ int VideoCaptureImpl::CaptureFrameRate() {
 VideoCaptureImpl::VideoCaptureImpl(
     const media::VideoCaptureSessionId session_id,
     VideoCaptureMessageFilter* filter)
-    : VideoCapture(),
-      message_filter_(filter),
-      io_message_loop_proxy_(ChildProcess::current()->io_message_loop_proxy()),
+    : message_filter_(filter),
       device_id_(0),
       session_id_(session_id),
       suspended_(false),
       state_(VIDEO_CAPTURE_STATE_STOPPED),
       weak_factory_(this) {
   DCHECK(filter);
+  thread_checker_.DetachFromThread();
 }
 
-VideoCaptureImpl::~VideoCaptureImpl() {}
-
-void VideoCaptureImpl::Init() {
-  io_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::InitOnIOThread,
-                 base::Unretained(this)));
-}
-
-void VideoCaptureImpl::DeInit(base::Closure done_cb) {
-  io_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DeInitOnIOThread,
-                 base::Unretained(this),
-                 done_cb));
-}
-
-void VideoCaptureImpl::SuspendCapture(bool suspend) {
-  io_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::SuspendCaptureOnIOThread,
-                 base::Unretained(this),
-                 suspend));
-}
-
-void VideoCaptureImpl::StartCapture(
-    media::VideoCapture::EventHandler* handler,
-    const media::VideoCaptureParams& params) {
-  io_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::StartCaptureOnIOThread,
-                 base::Unretained(this), handler, params));
+VideoCaptureImpl::~VideoCaptureImpl() {
+  DCHECK(thread_checker_.CalledOnValidThread());
 }
 
-void VideoCaptureImpl::StopCapture(
-    media::VideoCapture::EventHandler* handler) {
-  io_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::StopCaptureOnIOThread,
-                 base::Unretained(this), handler));
-}
-
-void VideoCaptureImpl::GetDeviceSupportedFormats(
-    const DeviceFormatsCallback& callback) {
-  DCHECK(!callback.is_null());
-  io_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::GetDeviceSupportedFormatsOnIOThread,
-                 base::Unretained(this), media::BindToCurrentLoop(callback)));
-}
-
-void VideoCaptureImpl::GetDeviceFormatsInUse(
-    const DeviceFormatsInUseCallback& callback) {
-  DCHECK(!callback.is_null());
-  io_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::GetDeviceFormatsInUseOnIOThread,
-                 base::Unretained(this), media::BindToCurrentLoop(callback)));
-}
-
-void VideoCaptureImpl::InitOnIOThread() {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+void VideoCaptureImpl::Init() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   message_filter_->AddDelegate(this);
 }
 
-void VideoCaptureImpl::DeInitOnIOThread(base::Closure done_cb) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+void VideoCaptureImpl::DeInit() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (state_ == VIDEO_CAPTURE_STATE_STARTED)
     Send(new VideoCaptureHostMsg_Stop(device_id_));
   message_filter_->RemoveDelegate(this);
-  done_cb.Run();
 }
 
-void VideoCaptureImpl::SuspendCaptureOnIOThread(bool suspend) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+void VideoCaptureImpl::SuspendCapture(bool suspend) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   suspended_ = suspend;
 }
 
-void VideoCaptureImpl::StartCaptureOnIOThread(
-    media::VideoCapture::EventHandler* handler,
-    const media::VideoCaptureParams& params) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+void VideoCaptureImpl::StartCapture(
+    int client_id,
+    const media::VideoCaptureParams& params,
+    const VideoCaptureStateUpdateCB& state_update_cb,
+    const VideoCaptureDeliverFrameCB& deliver_frame_cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ClientInfo client_info;
+  client_info.params = params;
+  client_info.state_update_cb = state_update_cb;
+  client_info.deliver_frame_cb = deliver_frame_cb;
+
   if (state_ == VIDEO_CAPTURE_STATE_ERROR) {
-    handler->OnError(this, 1);
-    handler->OnRemoved(this);
-  } else if ((clients_pending_on_filter_.find(handler) !=
+    state_update_cb.Run(VIDEO_CAPTURE_STATE_ERROR);
+  } else if ((clients_pending_on_filter_.find(client_id) !=

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

map_.find(foo) != map_.end()
is shorter to write as
map_.count(foo)
(and avoids the pitfall of accidentally specifying a different map_ the second
time)

[Alpha Left Google, 2014/04/23 18:48:33]

Done.

               clients_pending_on_filter_.end()) ||
-             (clients_pending_on_restart_.find(handler) !=
+             (clients_pending_on_restart_.find(client_id) !=
               clients_pending_on_restart_.end()) ||
-             clients_.find(handler) != clients_.end() ) {
+             clients_.find(client_id) != clients_.end() ) {
     // This client has started.

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

what about the callbacks?  This seems like it should be a DFATAL.

[Alpha Left Google, 2014/04/23 18:48:33]

Done.

   } else if (!device_id_) {
-    clients_pending_on_filter_[handler] = params;
+    clients_pending_on_filter_[client_id] = client_info;
   } else {
-    handler->OnStarted(this);
+    // Note: |state_| might not be started at this point. But we tell
+    // client that we have started.
+    state_update_cb.Run(VIDEO_CAPTURE_STATE_STARTED);
     if (state_ == VIDEO_CAPTURE_STATE_STARTED) {
-      clients_[handler] = params;
+      clients_[client_id] = client_info;
       // TODO(sheu): Allowing resolution change will require that all
       // outstanding clients of a capture session support resolution change.
       DCHECK_EQ(params_.allow_resolution_change,
                 params.allow_resolution_change);
     } else if (state_ == VIDEO_CAPTURE_STATE_STOPPING) {
-      clients_pending_on_restart_[handler] = params;
+      clients_pending_on_restart_[client_id] = client_info;
       DVLOG(1) << "StartCapture: Got new resolution "
                << params.requested_format.frame_size.ToString()
                << " during stopping.";
     } else {
-      clients_[handler] = params;
+      clients_[client_id] = client_info;
       DCHECK_EQ(1ul, clients_.size());

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

'l' should not be necessary here.

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

what prevents two clients from registering almost-simultaneously?

[Alpha Left Google, 2014/04/23 18:48:33]

Done.

[Alpha Left Google, 2014/04/23 18:48:33]

I *think* the intention in this code is to verify that it doesn't start twice.
Added a check for state. Also a test for this.

       params_ = params;
       if (params_.requested_format.frame_rate >
@@ -170,16 +126,15 @@ void VideoCaptureImpl::StartCaptureOnIOThread(
   }
 }
 
-void VideoCaptureImpl::StopCaptureOnIOThread(
-    media::VideoCapture::EventHandler* handler) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+void VideoCaptureImpl::StopCapture(int client_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
 
-  // A handler can be in only one client list.
-  // If this handler is in any client list, we can just remove it from
+  // A client ID can be in only one client list.
+  // If this ID is in any client list, we can just remove it from
   // that client list and don't have to run the other following RemoveClient().
-  RemoveClient(handler, &clients_pending_on_filter_) ||
-  RemoveClient(handler, &clients_pending_on_restart_) ||
-  RemoveClient(handler, &clients_);
+  RemoveClient(client_id, &clients_pending_on_filter_) ||
+  RemoveClient(client_id, &clients_pending_on_restart_) ||
+  RemoveClient(client_id, &clients_);

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

ick to using boolean short-cutting like this (in a context where the
expression's value is dropped)

[Alpha Left Google, 2014/04/23 18:48:33]

RemoveClient actually calls the client and give it a stopped state. I have made
this cleaner a bit.

 
   if (clients_.empty()) {
     DVLOG(1) << "StopCapture: No more client, stopping ...";
@@ -189,20 +144,20 @@ void VideoCaptureImpl::StopCaptureOnIOThread(
   }
 }
 
-void VideoCaptureImpl::GetDeviceSupportedFormatsOnIOThread(
-    const DeviceFormatsCallback& callback) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
-  device_formats_callback_queue_.push_back(callback);
-  if (device_formats_callback_queue_.size() == 1)
+void VideoCaptureImpl::GetDeviceSupportedFormats(
+    const VideoCaptureDeviceFormatsCB& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  device_formats_cb_queue_.push_back(callback);
+  if (device_formats_cb_queue_.size() == 1)
     Send(new VideoCaptureHostMsg_GetDeviceSupportedFormats(device_id_,
                                                            session_id_));
 }
 
-void VideoCaptureImpl::GetDeviceFormatsInUseOnIOThread(
-    const DeviceFormatsInUseCallback& callback) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
-  device_formats_in_use_callback_queue_.push_back(callback);
-  if (device_formats_in_use_callback_queue_.size() == 1)
+void VideoCaptureImpl::GetDeviceFormatsInUse(
+    const VideoCaptureDeviceFormatsCB& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  device_formats_in_use_cb_queue_.push_back(callback);
+  if (device_formats_in_use_cb_queue_.size() == 1)
     Send(
         new VideoCaptureHostMsg_GetDeviceFormatsInUse(device_id_, session_id_));
 }
@@ -210,7 +165,7 @@ void VideoCaptureImpl::GetDeviceFormatsInUseOnIOThread(
 void VideoCaptureImpl::OnBufferCreated(
     base::SharedMemoryHandle handle,
     int length, int buffer_id) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   // In case client calls StopCapture before the arrival of created buffer,
   // just close this buffer and return.
@@ -234,7 +189,7 @@ void VideoCaptureImpl::OnBufferCreated(
 }
 
 void VideoCaptureImpl::OnBufferDestroyed(int buffer_id) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   ClientBufferMap::iterator iter = client_buffers_.find(buffer_id);
   if (iter == client_buffers_.end())
@@ -248,7 +203,7 @@ void VideoCaptureImpl::OnBufferDestroyed(int buffer_id) {
 void VideoCaptureImpl::OnBufferReceived(int buffer_id,
                                         const media::VideoCaptureFormat& format,
                                         base::TimeTicks timestamp) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   // The capture pipeline supports only I420 for now.
   DCHECK_EQ(format.pixel_format, media::PIXEL_FORMAT_I420);
@@ -289,8 +244,10 @@ void VideoCaptureImpl::OnBufferReceived(int buffer_id,
               buffer,
               base::Passed(scoped_ptr<gpu::MailboxHolder>().Pass()))));
 
-  for (ClientInfo::iterator it = clients_.begin(); it != clients_.end(); ++it)
-    it->first->OnFrameReady(this, frame);
+  for (ClientInfoMap::iterator it = clients_.begin(); it != clients_.end();
+       ++it) {
+    it->second.deliver_frame_cb.Run(frame, format, timestamp);
+  }
 }
 
 static void NullReadPixelsCB(const SkBitmap& bitmap) { NOTIMPLEMENTED(); }
@@ -300,7 +257,7 @@ void VideoCaptureImpl::OnMailboxBufferReceived(
     const gpu::MailboxHolder& mailbox_holder,
     const media::VideoCaptureFormat& format,
     base::TimeTicks timestamp) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   if (state_ != VIDEO_CAPTURE_STATE_STARTED || suspended_) {
     Send(new VideoCaptureHostMsg_BufferReady(
@@ -325,21 +282,23 @@ void VideoCaptureImpl::OnMailboxBufferReceived(
       timestamp - first_frame_timestamp_,
       base::Bind(&NullReadPixelsCB));
 
-  for (ClientInfo::iterator it = clients_.begin(); it != clients_.end(); ++it)
-    it->first->OnFrameReady(this, frame);
+  for (ClientInfoMap::iterator it = clients_.begin(); it != clients_.end();
+       ++it) {
+    it->second.deliver_frame_cb.Run(frame, format, timestamp);
+  }
 }
 
 void VideoCaptureImpl::OnClientBufferFinished(
     int buffer_id,
     const scoped_refptr<ClientBuffer>& /* ignored_buffer */,
     scoped_ptr<gpu::MailboxHolder> mailbox_holder) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   const uint32 sync_point = (mailbox_holder ? mailbox_holder->sync_point : 0);
   Send(new VideoCaptureHostMsg_BufferReady(device_id_, buffer_id, sync_point));
 }
 
 void VideoCaptureImpl::OnStateChanged(VideoCaptureState state) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   switch (state) {
     case VIDEO_CAPTURE_STATE_STARTED:

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

seems strange that this is a no-op.

[Alpha Left Google, 2014/04/23 18:48:33]

Added a comment.

@@ -353,27 +312,26 @@ void VideoCaptureImpl::OnStateChanged(VideoCaptureState state) {
         RestartCapture();
       break;
     case VIDEO_CAPTURE_STATE_PAUSED:
-      for (ClientInfo::iterator it = clients_.begin();
+      for (ClientInfoMap::iterator it = clients_.begin();
            it != clients_.end(); ++it) {
-        it->first->OnPaused(this);
+        it->second.state_update_cb.Run(VIDEO_CAPTURE_STATE_PAUSED);
       }
       break;
     case VIDEO_CAPTURE_STATE_ERROR:
       DVLOG(1) << "OnStateChanged: error!, device_id = " << device_id_;
-      for (ClientInfo::iterator it = clients_.begin();
+      for (ClientInfoMap::iterator it = clients_.begin();
            it != clients_.end(); ++it) {
-        // TODO(wjia): browser process would send error code.
-        it->first->OnError(this, 1);
-        it->first->OnRemoved(this);
+        it->second.state_update_cb.Run(VIDEO_CAPTURE_STATE_ERROR);
       }
       clients_.clear();
       state_ = VIDEO_CAPTURE_STATE_ERROR;
       break;
     case VIDEO_CAPTURE_STATE_ENDED:
       DVLOG(1) << "OnStateChanged: ended!, device_id = " << device_id_;
-      for (ClientInfo::iterator it = clients_.begin();
+      for (ClientInfoMap::iterator it = clients_.begin();
           it != clients_.end(); ++it) {
-        it->first->OnRemoved(this);
+        // We'll only notify the client that the stream has stopped.
+        it->second.state_update_cb.Run(VIDEO_CAPTURE_STATE_STOPPED);
       }
       clients_.clear();
       state_ = VIDEO_CAPTURE_STATE_ENDED;
@@ -385,36 +343,41 @@ void VideoCaptureImpl::OnStateChanged(VideoCaptureState state) {
 
 void VideoCaptureImpl::OnDeviceSupportedFormatsEnumerated(
     const media::VideoCaptureFormats& supported_formats) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
-  for (size_t i = 0; i < device_formats_callback_queue_.size(); ++i)
-    device_formats_callback_queue_[i].Run(supported_formats);
-  device_formats_callback_queue_.clear();
+  DCHECK(thread_checker_.CalledOnValidThread());
+  for (size_t i = 0; i < device_formats_cb_queue_.size(); ++i)
+    device_formats_cb_queue_[i].Run(supported_formats);
+  device_formats_cb_queue_.clear();
 }
 
 void VideoCaptureImpl::OnDeviceFormatsInUseReceived(
     const media::VideoCaptureFormats& formats_in_use) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
-  for (size_t i = 0; i < device_formats_in_use_callback_queue_.size(); ++i)
-    device_formats_in_use_callback_queue_[i].Run(formats_in_use);
-  device_formats_in_use_callback_queue_.clear();
+  DCHECK(thread_checker_.CalledOnValidThread());
+  for (size_t i = 0; i < device_formats_in_use_cb_queue_.size(); ++i)
+    device_formats_in_use_cb_queue_[i].Run(formats_in_use);
+  device_formats_in_use_cb_queue_.clear();
 }
 
 void VideoCaptureImpl::OnDelegateAdded(int32 device_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(1) << "OnDelegateAdded: device_id " << device_id;
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
 
   device_id_ = device_id;
-  for (ClientInfo::iterator it = clients_pending_on_filter_.begin();
+  for (ClientInfoMap::iterator it = clients_pending_on_filter_.begin();
        it != clients_pending_on_filter_.end(); ) {
-    media::VideoCapture::EventHandler* handler = it->first;
-    const media::VideoCaptureParams params = it->second;
+    int client_id = it->first;
+    VideoCaptureStateUpdateCB state_update_cb =
+        it->second.state_update_cb;
+    VideoCaptureDeliverFrameCB deliver_frame_cb =
+        it->second.deliver_frame_cb;
+    const media::VideoCaptureParams params = it->second.params;
     clients_pending_on_filter_.erase(it++);
-    StartCapture(handler, params);
+    StartCapture(client_id, params, state_update_cb,
+                 deliver_frame_cb);
   }
 }
 
 void VideoCaptureImpl::StopDevice() {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   if (state_ == VIDEO_CAPTURE_STATE_STARTED) {
     state_ = VIDEO_CAPTURE_STATE_STOPPING;
@@ -424,20 +387,24 @@ void VideoCaptureImpl::StopDevice() {
 }
 
 void VideoCaptureImpl::RestartCapture() {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_EQ(state_, VIDEO_CAPTURE_STATE_STOPPED);
 
   int width = 0;
   int height = 0;
-  for (ClientInfo::iterator it = clients_.begin();
+  for (ClientInfoMap::iterator it = clients_.begin();
        it != clients_.end(); ++it) {
-    width = std::max(width, it->second.requested_format.frame_size.width());
-    height = std::max(height, it->second.requested_format.frame_size.height());
+    width = std::max(width,
+                     it->second.params.requested_format.frame_size.width());
+    height = std::max(height,
+                      it->second.params.requested_format.frame_size.height());
   }
-  for (ClientInfo::iterator it = clients_pending_on_restart_.begin();
+  for (ClientInfoMap::iterator it = clients_pending_on_restart_.begin();
        it != clients_pending_on_restart_.end(); ) {
-    width = std::max(width, it->second.requested_format.frame_size.width());
-    height = std::max(height, it->second.requested_format.frame_size.height());
+    width = std::max(width,
+                     it->second.params.requested_format.frame_size.width());
+    height = std::max(height,
+                      it->second.params.requested_format.frame_size.height());
     clients_[it->first] = it->second;
     clients_pending_on_restart_.erase(it++);
   }

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

if you did 
clients_.insert(clients_pending_on_restart_.begin(),
clients_pending_on_restart_.end());
at l.395 then you could delete this loop.

[Alpha Left Google, 2014/04/23 18:48:33]

Done.

@@ -448,7 +415,7 @@ void VideoCaptureImpl::RestartCapture() {
 }
 
 void VideoCaptureImpl::StartCaptureInternal() {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(device_id_);
 
   Send(new VideoCaptureHostMsg_Start(device_id_, session_id_, params_));
@@ -456,21 +423,17 @@ void VideoCaptureImpl::StartCaptureInternal() {
 }
 
 void VideoCaptureImpl::Send(IPC::Message* message) {
-  io_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(base::IgnoreResult(&VideoCaptureMessageFilter::Send),
-                 message_filter_.get(), message));
+  DCHECK(thread_checker_.CalledOnValidThread());
+  message_filter_->Send(message);
 }
 
-bool VideoCaptureImpl::RemoveClient(
-    media::VideoCapture::EventHandler* handler,
-    ClientInfo* clients) {
-  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+bool VideoCaptureImpl::RemoveClient(int client_id, ClientInfoMap* clients) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   bool found = false;
 
-  ClientInfo::iterator it = clients->find(handler);
+  ClientInfoMap::iterator it = clients->find(client_id);
   if (it != clients->end()) {
-    handler->OnStopped(this);
-    handler->OnRemoved(this);
+    it->second.state_update_cb.Run(VIDEO_CAPTURE_STATE_STOPPED);
     clients->erase(it);
     found = true;
   }