Add VideoCaptureImpl

content/renderer/media/video_capture_impl.cc

Index: content/renderer/media/video_capture_impl.cc
===================================================================
--- content/renderer/media/video_capture_impl.cc	(revision 0)
+++ content/renderer/media/video_capture_impl.cc	(revision 0)
@@ -0,0 +1,444 @@
+// Copyright (c) 2011 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 "content/renderer/media/video_capture_impl.h"
+
+#include "base/memory/singleton.h"
+#include "content/common/child_process.h"
+#include "content/common/child_thread.h"
+#include "content/common/video_capture_messages.h"
+#include "content/common/view_messages.h"
+#include "media/base/message_loop_factory_impl.h"
+
+namespace {
+
+// VideoCaptureMessageFilterCreator is to be used as a singleton so we can get
+// access to a shared VideoCaptureMessageFilter.
+// Example usage:
+//   VideoCaptureMessageFilter* filter =
+//       VideoCaptureMessageFilterCreator::SharedFilter();
+
+class VideoCaptureMessageFilterCreator {
+ public:
+  VideoCaptureMessageFilterCreator() {
+    int routing_id;
+    ChildThread::current()->Send(
+        new ViewHostMsg_GenerateRoutingID(&routing_id));
+    filter_ = new VideoCaptureMessageFilter(routing_id);
+    filter_->AddFilter();
+  }
+
+  static VideoCaptureMessageFilter* SharedFilter() {
+    return GetInstance()->filter_.get();
+  }
+
+  static VideoCaptureMessageFilterCreator* GetInstance() {
+    return Singleton<VideoCaptureMessageFilterCreator>::get();
+  }
+
+ private:
+  scoped_refptr<VideoCaptureMessageFilter> filter_;
+};
+
+}  // namespace
+
+VideoCaptureImplManager::VideoCaptureImplManager() {
+  ml_factory_.reset(new media::MessageLoopFactoryImpl());
+  ml_proxy_ = ml_factory_->GetMessageLoopProxy("VC manager");
+}
+
+// static
+media::VideoCapture* VideoCaptureImplManager::AddDevice(
+    media::VideoCaptureSessionId id,
+    media::VideoCapture::EventHandler* handler) {
+  DCHECK(handler);
+  VideoCaptureImplManager* manager = GetInstance();
+
+  base::AutoLock auto_lock(manager->lock_);
+  Devices::iterator it = manager->devices_.find(id);
+  if (it == manager->devices_.end()) {
+    VideoCaptureImpl* vc =
+        new VideoCaptureImpl(id, manager->ml_proxy_,
+                             VideoCaptureMessageFilterCreator::SharedFilter());
+    if (vc) {

[scherkus (not reviewing), 2011/05/14 23:33:42]

not sure how valuable this check is

chrome registers out of memory exception handlers that kill the tab :)

[wjia(left Chromium), 2011/05/17 04:00:25]

removed the checking. What if some module has a backup method (without
allocating memory, but slow) to handle out of memory case?

On 2011/05/14 23:33:42, scherkus wrote:

+      manager->devices_[id] = Device(vc, handler);
+      vc->Init();
+    }
+    return vc;
+  }
+
+  manager->devices_[id].clients.push_front(handler);
+  return it->second.vc;
+}
+
+// static
+void VideoCaptureImplManager::RemoveDevice(
+    media::VideoCaptureSessionId id,
+    media::VideoCapture::EventHandler* handler) {
+  DCHECK(handler);
+  VideoCaptureImplManager* manager = GetInstance();
+
+  base::AutoLock auto_lock(manager->lock_);
+  Devices::iterator it = manager->devices_.find(id);
+  if (it == manager->devices_.end())

[scherkus (not reviewing), 2011/05/14 23:33:42]

is it valid to call RemoveDevice w/ invalid id?

it it worth DCHECKing this case?

[wjia(left Chromium), 2011/05/17 04:00:25]

The idea is to allow anyone to call RemoveDevice with any id and pointer. Only
valid ones will go through. This can prevent clients from calling RemoveDevice
more than once. I might remove the first argument |id| since it's directly
mapped with |handler|.

I can add return value for RemoveDevice once invalid input is detected. Not sure
how useful that would be.

+    return;
+
+  size_t size = it->second.clients.size();
+  it->second.clients.remove(handler);
+
+  if (size == it->second.clients.size() || size > 1)

[scherkus (not reviewing), 2011/05/14 23:33:42]

is it valid to remove pass in an invalid handler pointer?

do you want to DCHECK that case?

also if I'm following this code correctly, it seems simpler to check for
(!it->second.clients.empty()) instead

[wjia(left Chromium), 2011/05/17 04:00:25]

same explanation as above. 

+    return;
+
+  manager->devices_[id].vc->DeInit(NewRunnableMethod(manager,
+      &VideoCaptureImplManager::FreeDevice, manager->devices_[id].vc));

[scherkus (not reviewing), 2011/05/14 23:33:42]

are you transferring ownership of vc to this task in order to get deleted?

[wjia(left Chromium), 2011/05/17 04:00:25]

yes, because |vc| has to be removed from MessageFilter's delegate list before it
can be deleted. If MessageFilter has a lock to allow Add/Remove delegate
synchronously, there would be no such issue.

+  manager->devices_.erase(id);
+  return;
+}
+
+// static
+VideoCaptureImplManager* VideoCaptureImplManager::GetInstance() {
+  return Singleton<VideoCaptureImplManager>::get();
+}
+
+void VideoCaptureImplManager::FreeDevice(VideoCaptureImpl* vc) {
+  delete vc;

[scherkus (not reviewing), 2011/05/14 23:33:42]

looks like this method only gets called on the IO thread

it's not clear to me which thread is calling
VideoCaptureImplManager::AddDevice() but typically it's good practice to
create+delete objects on the same threads

so as a sanity check... is it safe to delete vc from the IO thread?

[wjia(left Chromium), 2011/05/17 04:00:25]

VideoCaptureImplManager::AddDevice() can be called from any thread since there
is no guarantee on which thread capture client would run. Due to this, there is
no guarantee the creator thread of |vc| is still alive when |vc| is about to be
deleted.

When VideoCaptureImplManager tries to delete |vc|, it's guarantee that no one is
using |vc| since the reference count of Device is zero and |vc| has been removed
from MessageFilter's delegate list.

+}
+
+VideoCaptureImpl::VideoCaptureImpl(
+    const media::VideoCaptureSessionId id,
+    scoped_refptr<base::MessageLoopProxy> ml_proxy,
+    VideoCaptureMessageFilter* filter)
+    : VideoCapture(),
+      filter_(filter),
+      session_id_(id),
+      ml_proxy_(ml_proxy),
+      device_id_(0),
+      width_(0),
+      height_(0),
+      frame_rate_(0),
+      video_type_(media::VideoFrame::I420),

[scherkus (not reviewing), 2011/05/14 23:33:42]

not initializing new_width_ / new_height_

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+      state_(kStopped) {
+  DCHECK(filter);
+}
+
+VideoCaptureImpl::~VideoCaptureImpl() {

[scherkus (not reviewing), 2011/05/14 23:33:42]

nit: collapse {} onto one line if it's <80chars

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+}
+
+void VideoCaptureImpl::Init() {
+  AddDelegateOnIOThread();
+}
+
+void VideoCaptureImpl::DeInit(Task* task) {
+  StopDevice();

[scherkus (not reviewing), 2011/05/14 23:33:42]

hrmmmm... so again I'm not 100% on which thread is calling what here, but I
think in any case we have a nasty race between this pointer + FreeDevice()
getting executed:

If RemoveDevice() called on IO thread:
1) StopDevice() will re-post method to ml_proxy_
2) RemoveDelegateOnIOThread() is executed immediately, which calls FreeDevice()
and deletes the object
3) StopDevice() gets executed on deleted object

If RemoveDevice() called on some random thread:
1) StopDevice() will re-post method to ml_proxy_
2) RemoveDelegateOnIOThread() will re-post method to IO thread
3) Race between StopDevice() and FreeDevice() on two different threads

If RemoveDevice() called on ml_proxy_ thread:
1) StopDevice() will execute
2) RemoveDelegateOnIOThread() will re-post method to IO thread
3) FreeDevice() can get executed w/o race

Long story short, this all goes back to having clearer expectations on threading
semantics for VideoCaptureImplManager and ideally creating+deleting objects on
the same thread. See my comments in StopDevice() as well.

How about this:
  1) DCHECK() that this must be called on ml_proxy_
  2) Don't pass in a Task here as a callback
  3) Call StopDevice() and it will execute normally
  4) Post a task to IO thread proxy to remove delegate
  5) After removing, post a task to ml_proxy_ to self-delete

If there's never a case to remove yourself as a delegate but keep the object
living, I think it'd be good to reduce the complexity and simply combine the
steps for removing + deleting the object as part of tear down.

[wjia(left Chromium), 2011/05/17 04:00:25]

You are right. Thanks for pointing this out. I need to send out the message
synchronously, instead of using PostTask. Since in DeInit, we don't care about
state_ and width_/height_ any longer. So don't have to get on ml_proxy_ to send
message.

On 2011/05/14 23:33:42, scherkus wrote:

+  RemoveDelegateOnIOThread(task);
+}
+
+void VideoCaptureImpl::StartCapture(
+    media::VideoCapture::EventHandler* handler,
+    const VideoCaptureCapability& capability) {
+  DCHECK_EQ(capability.raw_type, media::VideoFrame::I420);
+
+  if (!ml_proxy_->BelongsToCurrentThread()) {
+    ml_proxy_->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &VideoCaptureImpl::StartCapture, handler,
+                          capability));
+    return;
+  }
+
+  if (capability.resolution_fixed && master_clients_.size() &&
+      (capability.width != width_ || capability.height != height_)) {
+    // Can't have 2 master clients with different resolutions.
+    handler->OnError(this, 1);
+    return;
+  }
+
+  clients_[handler] = capability;
+  if (capability.resolution_fixed) {
+    master_clients_.push_back(handler);
+    if (master_clients_.size() > 1)
+      return;
+  }
+
+  if (state_ == kStarted) {
+    // Take the resolution of master client.
+    if (capability.resolution_fixed &&
+        (capability.width != width_ || capability.height != height_)) {
+      new_width_ = capability.width;
+      new_height_ = capability.height;
+      DLOG(INFO) << "StartCapture: Got master client with new resolution "
+          "during started, try to restart.";
+      StopDevice();
+    }
+    handler->OnStarted(this);
+    return;
+  }
+
+  if (state_ == kStopping) {
+    if (capability.resolution_fixed || !pending_start()) {
+      new_width_ = capability.width;
+      new_height_ = capability.height;
+      DLOG(INFO) << "StartCapture: Got new resolution, already in stopping.";
+    }
+    handler->OnStarted(this);
+    return;
+  }
+
+  DCHECK_EQ(clients_.size(), 1ul);
+  video_type_ = capability.raw_type;
+  new_width_ = 0;
+  new_height_ = 0;
+  width_ = capability.width;
+  height_ = capability.height;
+
+  StartCaptureInternal();
+}
+
+void VideoCaptureImpl::StopCapture(media::VideoCapture::EventHandler* handler) {
+  if (!ml_proxy_->BelongsToCurrentThread()) {
+    ml_proxy_->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &VideoCaptureImpl::StopCapture, handler));
+    return;
+  }
+
+  if (clients_.find(handler) == clients_.end())
+    return;
+
+  handler->OnStopped(this);
+  clients_.erase(handler);
+  master_clients_.remove(handler);
+
+  // Still have at least one master client.
+  if (master_clients_.size() > 0)
+    return;
+
+  // TODO(wjia): Is it really needed to handle resolution change for non-master
+  // clients, except no client case?
+  if (clients_.size() > 0) {
+    DLOG(INFO) << "StopCapture: No master client.";
+    int maxw = 0;
+    int maxh = 0;
+    for (ClientInfo::iterator it = clients_.begin();
+         it != clients_.end(); it++) {
+      if (it->second.width > maxw && it->second.height > maxh) {
+        maxw = it->second.width;
+        maxh = it->second.height;
+      }
+    }
+
+    if (state_ == kStarted) {
+      // Only handle resolution reduction.
+      if (maxw < width_ && maxh < height_) {
+        new_width_ = maxw;
+        new_height_ = maxh;
+        DLOG(INFO) << "StopCapture: New smaller resolution, stopping ...";
+        StopDevice();
+      }
+      return;
+    }
+
+    if (state_ == kStopping) {
+      new_width_ = maxw;
+      new_height_ = maxh;
+      DLOG(INFO) << "StopCapture: New resolution, during stopping.";
+      return;
+    }
+  } else {
+    new_width_ = width_ = 0;
+    new_height_ = height_ = 0;
+    DLOG(INFO) << "StopCapture: No more client, stopping ...";
+    StopDevice();
+  }
+}
+
+void VideoCaptureImpl::OnBufferReceived(TransportDIB::Handle handle,
+                                        base::Time timestamp) {
+  if (!ml_proxy_->BelongsToCurrentThread()) {
+    ml_proxy_->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &VideoCaptureImpl::OnBufferReceived,
+                          handle, timestamp));
+    return;
+  }
+
+  if (state_ != kStarted) {
+    filter_->Send(
+        new VideoCaptureHostMsg_BufferReady(0, device_id_, handle));
+    return;
+  }
+
+  media::VideoCapture::VideoFrameBuffer* buffer;
+  CachedDIB::iterator it;
+  for (it = cached_dibs_.begin(); it != cached_dibs_.end(); it++) {
+    if ((*it)->dib->handle() == handle)
+      break;
+  }
+  if (it == cached_dibs_.end()) {
+    TransportDIB* dib = TransportDIB::Map(handle);
+    buffer = new VideoFrameBuffer;

[scherkus (not reviewing), 2011/05/14 23:33:42]

add ()

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+    buffer->memory_pointer = dib->memory();
+    buffer->buffer_size = dib->size();
+    buffer->width = width_;
+    buffer->height = height_;
+
+    DIBBuffer* dib_buffer = new DIBBuffer(dib, buffer);
+    cached_dibs_.push_back(dib_buffer);
+  } else {
+    buffer = (*it)->mapped_memory;
+  }
+
+  // TODO(wjia): handle buffer sharing with downstream modules.
+  for (ClientInfo::iterator it = clients_.begin(); it != clients_.end(); it++) {
+      it->first->OnBufferReady(this, buffer);
+  }
+
+  filter_->Send(
+      new VideoCaptureHostMsg_BufferReady(0, device_id_, handle));
+}
+
+void VideoCaptureImpl::OnStateChanged(
+    const media::VideoCapture::State& state) {
+  if (!ml_proxy_->BelongsToCurrentThread()) {
+    ml_proxy_->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &VideoCaptureImpl::OnStateChanged, state));
+    return;
+  }
+
+  switch (state) {
+    case media::VideoCapture::kStarted:
+      for (ClientInfo::iterator it = clients_.begin();
+           it != clients_.end(); it++) {
+        it->first->OnStarted(this);
+      }
+      break;
+    case media::VideoCapture::kStopped:
+      state_ = kStopped;
+      DLOG(INFO) << "OnStateChanged: stopped!, device_id = " << device_id_;
+      if (pending_start())
+        RestartCapture();
+      break;
+    case media::VideoCapture::kPaused:
+      for (ClientInfo::iterator it = clients_.begin();
+           it != clients_.end(); it++) {
+        it->first->OnPaused(this);
+      }
+      break;
+    case media::VideoCapture::kError:
+      for (ClientInfo::iterator it = clients_.begin();
+           it != clients_.end(); it++) {
+        // TODO(wjia): browser process would send error code.
+        it->first->OnError(this, 1);
+      }
+      break;
+    default:
+      break;
+  }
+}
+
+void VideoCaptureImpl::OnDeviceInfoReceived(
+    const media::VideoCaptureParams& device_info) {
+  if (!ml_proxy_->BelongsToCurrentThread()) {
+    ml_proxy_->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &VideoCaptureImpl::OnDeviceInfoReceived,
+                          device_info));
+    return;
+  }
+
+  for (ClientInfo::iterator it = clients_.begin(); it != clients_.end(); it++) {
+    it->first->OnDeviceInfoReceived(this, device_info);
+  }
+}
+
+void VideoCaptureImpl::StopDevice() {
+  if (!ml_proxy_->BelongsToCurrentThread()) {

[scherkus (not reviewing), 2011/05/14 23:33:42]

the idea behind using the proxy is that you don't need these checks unless
you're calling this method from the IO thread, which for this method (and all
other non-VideoCaptureMessageFilter::Delegate methods) you shouldn't be

I think you could simplify your code + make clarify the threading semantics if:
  1) VideoCaptureMessageFilter::Delegate methods are the only ones that re-post
to ml_proxy_
  2) VideoCapture interfaces DCHECK for BelongsOnCurrentThread

+    ml_proxy_->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &VideoCaptureImpl::StopDevice));
+    return;
+  }
+
+  if (state_ == kStarted) {
+    state_ = kStopping;
+    filter_->Send(new VideoCaptureHostMsg_Stop(0, device_id_));
+    width_ = height_ = 0;
+  }
+}
+
+void VideoCaptureImpl::RestartCapture() {
+  DCHECK(ml_proxy_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kStopped);
+
+  width_ = new_width_;
+  height_ = new_height_;
+  new_width_ = 0;
+  new_height_ = 0;
+
+  DLOG(INFO) << "RestartCapture, " << width_ << ", " << height_;
+  StartCaptureInternal();
+}
+
+void VideoCaptureImpl::StartCaptureInternal() {
+  DCHECK(ml_proxy_->BelongsToCurrentThread());
+
+  media::VideoCaptureParams params;
+  params.width = width_;
+  params.height = height_;
+  params.session_id = session_id_;
+
+  int try_times = 100;
+  bool result = false;
+  for (int i = 0; i < try_times; i++) {

[scherkus (not reviewing), 2011/05/14 23:33:42]

why do we have to try 100 times?

[wjia(left Chromium), 2011/05/17 04:00:25]

I just pick up this number for timeout. The reason here to try many times is
that |channel_| or |device_id_| might not be ready when client send StartCapture
message because both are set asychronously. If MessageFilter do synchronous
AddDelegate() (with a lock in MessageFilter), there is no need to worry about
|device_id_|. To get rid of dependency on |channel_| in
VideoCaptureMessageFilter, in VideoCaptureMessageFilter::Send(), I might use
ChildThread::current()->Send(). But I am not sure that's a clean solution. I
think ChildThread::current()->Send() will use the same |channel_|. It still has
issue of availability of |channel_|. Hopefully main thread is ready much earlier
than other threads.

+    if (device_id_ &&
+        filter_->Send(new VideoCaptureHostMsg_Start(0, device_id_, params))) {
+      result = true;
+      break;
+    }
+
+    base::PlatformThread::Sleep(1000);

[scherkus (not reviewing), 2011/05/14 23:33:42]

???

[wjia(left Chromium), 2011/05/17 04:00:25]

When either |channel_| or |device_id_| is not ready, give other threads some
time to set them up, then try to send message again.

+  }
+
+  if (!result) {
+    state_ = kStopped;
+    width_ = height_ = 0;
+    for (ClientInfo::iterator it = clients_.begin(); it != clients_.end();
+         it++) {
+      it->first->OnError(this, 2);
+    }
+  } else {
+    state_ = kStarted;
+    for (ClientInfo::iterator it = clients_.begin(); it != clients_.end();
+         it++) {
+      it->first->OnStarted(this);
+    }
+  }
+}
+
+void VideoCaptureImpl::AddDelegateOnIOThread() {
+  DCHECK(ChildProcess::current()) << "Must be in the renderer";
+  base::MessageLoopProxy* message_loop_proxy =
+      ChildProcess::current()->io_message_loop_proxy();
+  DCHECK(message_loop_proxy);
+
+  if (!message_loop_proxy->BelongsToCurrentThread()) {

[scherkus (not reviewing), 2011/05/14 23:33:42]

considering you only have one caller of AddDelegateOnIOThread I think it'd be
clearer if you changed that call site to post directly to the IO thead so you
don't have these re-entrant methods

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+    message_loop_proxy->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &VideoCaptureImpl::AddDelegateOnIOThread));
+    return;
+  }
+
+  device_id_ = filter_->AddDelegate(this);

[scherkus (not reviewing), 2011/05/14 23:33:42]

I think there's a race between StartCaptureInternal() and this line of code
executing on the IO thread

also this is written+read from different threads and I'm not seeing any locking

it's possible that a compiler will optimize the device_id_ read in your loop in
StartCaptureInternal() and never re-read it from memory

[wjia(left Chromium), 2011/05/17 04:00:25]

The |device_id_| can be only modified on IO thread. A volatile modifier is added
to ensure |device_id_| won't be optimized to a register variable in the loop.
There is a race between StartCaptureInternal() and AddDelegate(). That's why
StartCaptureInternal() is doing the for loop. 

This is kind of mess, I think. Another way is to change API between VCImpl and
its clients. When StartCapture message can't be sent out, VCImpl notifies its
client of an error. It's up to client to retry.

+}
+
+void VideoCaptureImpl::RemoveDelegateOnIOThread(Task* task) {
+  DCHECK(ChildProcess::current()) << "Must be in the renderer";
+  base::MessageLoopProxy* message_loop_proxy =
+      ChildProcess::current()->io_message_loop_proxy();
+  DCHECK(message_loop_proxy);
+
+  if (!message_loop_proxy->BelongsToCurrentThread()) {

[scherkus (not reviewing), 2011/05/14 23:33:42]

ditto

[wjia(left Chromium), 2011/05/17 04:00:25]

Done.

+    message_loop_proxy->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &VideoCaptureImpl::RemoveDelegateOnIOThread,
+                          task));
+    return;
+  }
+
+  filter_->RemoveDelegate(this);
+  media::AutoTaskRunner auto_runner(task);
+}
Property changes on: content/renderer/media/video_capture_impl.cc
___________________________________________________________________
Added: svn:eol-style
   + LF