Add VideoCaptureImpl

Add VideoCaptureImpl and VideoCaptureImplManager
BUG=none
TEST=try bots
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86758

[scherkus (not reviewing), 2011-05-06 06:27:34]

Is this one ready for review?

[wjia(left Chromium), 2011-05-07 01:01:24]

Andrew,

It's ready for review now. This one depends on
http://codereview.chromium.org/6951013/ for MessageLoopProxy.

On 2011/05/06 06:27:34, scherkus wrote:
> Is this one ready for review?

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

mostly concerned about threading

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:64: if (vc) {
not sure how valuable this check is

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

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:84: if (it ==
manager->devices_.end())
is it valid to call RemoveDevice w/ invalid id?

it it worth DCHECKing this case?

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:90: if (size ==
it->second.clients.size() || size > 1)
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

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:94:
&VideoCaptureImplManager::FreeDevice, manager->devices_[id].vc));
are you transferring ownership of vc to this task in order to get deleted?

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:105: delete vc;
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?

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:120:
video_type_(media::VideoFrame::I420),
not initializing new_width_ / new_height_

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:125:
VideoCaptureImpl::~VideoCaptureImpl() {
nit: collapse {} onto one line if it's <80chars

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:133: StopDevice();
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.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:277: buffer = new VideoFrameBuffer;
add ()

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:352: if
(!ml_proxy_->BelongsToCurrentThread()) {
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

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:388: for (int i = 0; i < try_times;
i++) {
why do we have to try 100 times?

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:395:
base::PlatformThread::Sleep(1000);
???

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:420: if
(!message_loop_proxy->BelongsToCurrentThread()) {
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

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:426: device_id_ =
filter_->AddDelegate(this);
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

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:435: if
(!message_loop_proxy->BelongsToCurrentThread()) {
ditto

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.h (right):

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:19: class VideoCaptureImpl
class level comment

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:22: private:
nit: public section goes first

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:31: DIBBuffer() {}
do you need a default ctor?

otherwise it seems we'll have uninitialized memory (dib) and the dtor will
delete it

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:32: DIBBuffer(TransportDIB* d,
media::VideoCapture::VideoFrameBuffer* ptr)
please de-inline the ctor/dtor since this class contains non-trivial data
members (i.e., scoped_refptr)

this results in inlining template instantiation code at every new/delete site

see:
http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:43: typedef std::list<DIBBuffer*>
CachedDIB;
nit: I'd move this typedef down to where you declare the variable

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:45: typedef
std::map<media::VideoCapture::EventHandler*, VideoCaptureCapability>
ditto

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:49: // media::VideoCapture interface
add period

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:56: virtual int CaptureFrameRate() {
return frame_rate_; }
please de-inline all of these methods

virtual methods shouldn't be inlined, see:
http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:58: //
VideoCaptureMessageFilter::Delegate interface
add period

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:69: protected:
do you intend to further extend this class?

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:111: class VideoCaptureImplManager {
please move to separate file + class-level comment

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:126: Device() : vc(NULL) {}
please de-inline ctor/dtor since struct contains non-trivial data members

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:132: VideoCaptureImpl* vc;
sanity check: should Device own this pointer?

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:135: typedef
std::map<media::VideoCaptureSessionId, Device> Devices;
nit: move to variable declaration

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:143: };
DISALLOW_ etc....

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

Andrew,

Could you take another look?

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:64: if (vc) {
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:
> not sure how valuable this check is
> 
> chrome registers out of memory exception handlers that kill the tab :)

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:84: if (it ==
manager->devices_.end())
On 2011/05/14 23:33:42, scherkus wrote:
> is it valid to call RemoveDevice w/ invalid id?
> 
> it it worth DCHECKing this case?

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.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:90: if (size ==
it->second.clients.size() || size > 1)
On 2011/05/14 23:33:42, scherkus wrote:
> 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

same explanation as above.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:94:
&VideoCaptureImplManager::FreeDevice, manager->devices_[id].vc));
On 2011/05/14 23:33:42, scherkus wrote:
> are you transferring ownership of vc to this task in order to get deleted?

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.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:105: delete vc;
On 2011/05/14 23:33:42, scherkus wrote:
> 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?

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.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:120:
video_type_(media::VideoFrame::I420),
On 2011/05/14 23:33:42, scherkus wrote:
> not initializing new_width_ / new_height_

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:125:
VideoCaptureImpl::~VideoCaptureImpl() {
On 2011/05/14 23:33:42, scherkus wrote:
> nit: collapse {} onto one line if it's <80chars

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:133: StopDevice();
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:
> 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.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:277: buffer = new VideoFrameBuffer;
On 2011/05/14 23:33:42, scherkus wrote:
> add ()

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:388: for (int i = 0; i < try_times;
i++) {
On 2011/05/14 23:33:42, scherkus wrote:
> why do we have to try 100 times?

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.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:395:
base::PlatformThread::Sleep(1000);
On 2011/05/14 23:33:42, scherkus wrote:
> ???

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

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:420: if
(!message_loop_proxy->BelongsToCurrentThread()) {
On 2011/05/14 23:33:42, scherkus wrote:
> 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

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:426: device_id_ =
filter_->AddDelegate(this);
On 2011/05/14 23:33:42, scherkus wrote:
> 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

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.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:435: if
(!message_loop_proxy->BelongsToCurrentThread()) {
On 2011/05/14 23:33:42, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.h (right):

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:19: class VideoCaptureImpl
On 2011/05/14 23:33:42, scherkus wrote:
> class level comment

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:22: private:
On 2011/05/14 23:33:42, scherkus wrote:
> nit: public section goes first

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:31: DIBBuffer() {}
default ctor is removed.

On 2011/05/14 23:33:42, scherkus wrote:
> do you need a default ctor?
> 
> otherwise it seems we'll have uninitialized memory (dib) and the dtor will
> delete it

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:32: DIBBuffer(TransportDIB* d,
media::VideoCapture::VideoFrameBuffer* ptr)
On 2011/05/14 23:33:42, scherkus wrote:
> please de-inline the ctor/dtor since this class contains non-trivial data
> members (i.e., scoped_refptr)
> 
> this results in inlining template instantiation code at every new/delete site
> 
> see:
> http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:43: typedef std::list<DIBBuffer*>
CachedDIB;
On 2011/05/14 23:33:42, scherkus wrote:
> nit: I'd move this typedef down to where you declare the variable

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:45: typedef
std::map<media::VideoCapture::EventHandler*, VideoCaptureCapability>
On 2011/05/14 23:33:42, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:49: // media::VideoCapture interface
On 2011/05/14 23:33:42, scherkus wrote:
> add period

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:56: virtual int CaptureFrameRate() {
return frame_rate_; }
On 2011/05/14 23:33:42, scherkus wrote:
> please de-inline all of these methods
> 
> virtual methods shouldn't be inlined, see:
> http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:58: //
VideoCaptureMessageFilter::Delegate interface
On 2011/05/14 23:33:42, scherkus wrote:
> add period

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:69: protected:
On 2011/05/14 23:33:42, scherkus wrote:
> do you intend to further extend this class?

no. moved them to private.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:111: class VideoCaptureImplManager {
On 2011/05/14 23:33:42, scherkus wrote:
> please move to separate file + class-level comment

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:126: Device() : vc(NULL) {}
On 2011/05/14 23:33:42, scherkus wrote:
> please de-inline ctor/dtor since struct contains non-trivial data members

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:132: VideoCaptureImpl* vc;
On 2011/05/14 23:33:42, scherkus wrote:
> sanity check: should Device own this pointer?

yes, vc is owned by Device, and essentially managed by ImplManager.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:135: typedef
std::map<media::VideoCaptureSessionId, Device> Devices;
On 2011/05/14 23:33:42, scherkus wrote:
> nit: move to variable declaration

Done.

http://codereview.chromium.org/6902166/diff/11001/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:143: };
On 2011/05/14 23:33:42, scherkus wrote:
> DISALLOW_ etc....

Done.

[scherkus (not reviewing), 2011-05-23 03:45:54]

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:14: mapped_memory(ptr) {
indentation

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:112:
ml_proxy_->PostDelayedTask(FROM_HERE,
so now there is no time out?

I don't know which thread this is, but it's essentially a busy loop

would it help to create + register the filter during RenderView construction
(similar to AudioMessageFilter)?

if I understand this correctly this issue is because we lazy-instantiate the
VideoCaptureMessageFilter

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:271: it->first->OnBufferReady(this,
buffer);
over-indented

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.h (right):

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:82: typedef std::list<DIBBuffer*>
CachedDIB;
pedantic nit: move this below comment

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
File content/renderer/media/video_capture_impl_manager.h (right):

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl_manager.h:27: virtual
~VideoCaptureImplManager() {}
does this need to be virtual?

also don't inline

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl_manager.h:29: static
media::VideoCapture* AddDevice(
comments for these functiosn

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
File content/renderer/media/video_capture_impl_unittest.cc (right):

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl_unittest.cc:13: #define
DEFAULT_CAPABILITY {176, 144, 30, 0, media::VideoFrame::I420, \
you only use this once -- perhaps inline into test itself

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl_unittest.cc:80: VideoCaptureImpl*
video_capture_impl_;
scoped_ptr?

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl_unittest.cc:101: EXPECT_CALL(*client,
OnStopped(_))
I think it'd make more sense to move this call to after the call to
StartCapture()

[wjia(left Chromium), 2011-05-23 21:23:51]

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:14: mapped_memory(ptr) {
On 2011/05/23 03:45:54, scherkus wrote:
> indentation

Done.

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:112:
ml_proxy_->PostDelayedTask(FROM_HERE,
On 2011/05/23 03:45:54, scherkus wrote:
> so now there is no time out?
> 
> I don't know which thread this is, but it's essentially a busy loop
> 
> would it help to create + register the filter during RenderView construction
> (similar to AudioMessageFilter)?
> 
> if I understand this correctly this issue is because we lazy-instantiate the
> VideoCaptureMessageFilter

The "time out" can be added if desired (e.g., change second member of
PendingClient to a timeout count and every time when StartCapture is re-posted,
this count is decreased). The purpose of re-posting StartCapture is to wait for
filter and VCImpl to be ready, and hopefully it won't take long.

The loop is not that busy due to the delayed post.

The current design is to dynamically add filter. When a web page doesn't have
video capture needs, there is no need to add VideoCaptureMessageFilter. 

AudioMessageFilter has some limitation such as AudioDevice has to be created on
main thread in order to access RenderThread::current().

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:271: it->first->OnBufferReady(this,
buffer);
On 2011/05/23 03:45:54, scherkus wrote:
> over-indented

Done.

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.h (right):

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:82: typedef std::list<DIBBuffer*>
CachedDIB;
On 2011/05/23 03:45:54, scherkus wrote:
> pedantic nit: move this below comment

Done.

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
File content/renderer/media/video_capture_impl_manager.h (right):

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl_manager.h:27: virtual
~VideoCaptureImplManager() {}
On 2011/05/23 03:45:54, scherkus wrote:
> does this need to be virtual?
> 
> also don't inline
no, remove virtual. also remove inline.

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl_manager.h:29: static
media::VideoCapture* AddDevice(
On 2011/05/23 03:45:54, scherkus wrote:
> comments for these functiosn

Done.

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
File content/renderer/media/video_capture_impl_unittest.cc (right):

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl_unittest.cc:13: #define
DEFAULT_CAPABILITY {176, 144, 30, 0, media::VideoFrame::I420, \
On 2011/05/23 03:45:54, scherkus wrote:
> you only use this once -- perhaps inline into test itself

Right now, only one simple test has been added. More tests will be added later,
therefore this literal will be used multiple times.

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl_unittest.cc:80: VideoCaptureImpl*
video_capture_impl_;
On 2011/05/23 03:45:54, scherkus wrote:
> scoped_ptr?
VideoCaptureImpl has private ctor and dtor. Only VideoCaptureImplTest has been
added as friend class, not scoped_ptr<>.

http://codereview.chromium.org/6902166/diff/21004/content/renderer/media/vide...
content/renderer/media/video_capture_impl_unittest.cc:101: EXPECT_CALL(*client,
OnStopped(_))
On 2011/05/23 03:45:54, scherkus wrote:
> I think it'd make more sense to move this call to after the call to
> StartCapture() 

Done.

[scherkus (not reviewing), 2011-05-23 21:59:25]

LGTM and we'll iterate

[jam, 2011-05-24 19:15:41]

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:9: #include
"content/common/view_messages.h"
nit: why add this?

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:15: kTryTimes = 1000,
seems odd to put these two different constants in one enum...

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:68: DCHECK(ChildProcess::current())
<< "Must be in the renderer";
this will return true if you're in any childprocess, i.e. plugin/worker.  so not
sure what the dcheck gains us.

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:71: DCHECK(message_loop_proxy);
nit: this dcheck is unnecessary.  in release builds, it does nothing.  in debug,
we would crash a line below anyways, so it doesn't add any value

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.h (right):

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:15: #include
"base/message_loop_proxy.h"
nit: not needed, can just forward declare

http://codereview.chromium.org/6902166/diff/29003/content/renderer/video_capt...
File content/renderer/video_capture_message_filter.h (right):

http://codereview.chromium.org/6902166/diff/29003/content/renderer/video_capt...
content/renderer/video_capture_message_filter.h:51: virtual bool
Send(IPC::Message* message);
why virtual? it's not part of MessageFilter

http://codereview.chromium.org/6902166/diff/29003/content/renderer/video_capt...
content/renderer/video_capture_message_filter.h:55: virtual bool ReadyToSend();
documentation?

[wjia(left Chromium), 2011-05-24 20:47:15]

John,

Could you take another look?

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:9: #include
"content/common/view_messages.h"
On 2011/05/24 19:15:41, John Abd-El-Malek wrote:
> nit: why add this?

removed.

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:15: kTryTimes = 1000,
On 2011/05/24 19:15:41, John Abd-El-Malek wrote:
> seems odd to put these two different constants in one enum...

split.

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:68: DCHECK(ChildProcess::current())
<< "Must be in the renderer";
On 2011/05/24 19:15:41, John Abd-El-Malek wrote:
> this will return true if you're in any childprocess, i.e. plugin/worker.  so
not
> sure what the dcheck gains us.

removed.

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:71: DCHECK(message_loop_proxy);
On 2011/05/24 19:15:41, John Abd-El-Malek wrote:
> nit: this dcheck is unnecessary.  in release builds, it does nothing.  in
debug,
> we would crash a line below anyways, so it doesn't add any value

changed to condition checking.

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.h (right):

http://codereview.chromium.org/6902166/diff/29003/content/renderer/media/vide...
content/renderer/media/video_capture_impl.h:15: #include
"base/message_loop_proxy.h"
On 2011/05/24 19:15:41, John Abd-El-Malek wrote:
> nit: not needed, can just forward declare

Done.

http://codereview.chromium.org/6902166/diff/29003/content/renderer/video_capt...
File content/renderer/video_capture_message_filter.h (right):

http://codereview.chromium.org/6902166/diff/29003/content/renderer/video_capt...
content/renderer/video_capture_message_filter.h:51: virtual bool
Send(IPC::Message* message);
On 2011/05/24 19:15:41, John Abd-El-Malek wrote:
> why virtual? it's not part of MessageFilter

It's overridden in MockVideoCaptureMessageFilter which is used in
VideoCaptureImpl unit test.

http://codereview.chromium.org/6902166/diff/29003/content/renderer/video_capt...
content/renderer/video_capture_message_filter.h:55: virtual bool ReadyToSend();
On 2011/05/24 19:15:41, John Abd-El-Malek wrote:
> documentation?

Done.

[jam, 2011-05-25 17:21:51]

http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:13: enum { kTryTimes = 1000 };
enums are used when you have a list of related constants.  here you have two
unrelated constants.  you should just use

static const int kTryTimes = 1000;
static const int kDelayMs = 1;

http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:68: return;
why add this?  have you tracked a scenario where this is null?

http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:86: return;
ditto

[wjia(left Chromium), 2011-05-25 19:26:03]

Thanks, John! Those 3 places have been changed accordingly in Patch Set 13.


On 2011/05/25 17:21:51, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
> File content/renderer/media/video_capture_impl.cc (right):
> 
>
http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
> content/renderer/media/video_capture_impl.cc:13: enum { kTryTimes = 1000 };
> enums are used when you have a list of related constants.  here you have two
> unrelated constants.  you should just use
> 
> static const int kTryTimes = 1000;
> static const int kDelayMs = 1;
> 
>
http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
> content/renderer/media/video_capture_impl.cc:68: return;
> why add this?  have you tracked a scenario where this is null?
> 
>
http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
> content/renderer/media/video_capture_impl.cc:86: return;
> ditto

[wjia(left Chromium), 2011-05-25 19:26:23]

http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
File content/renderer/media/video_capture_impl.cc (right):

http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:13: enum { kTryTimes = 1000 };
On 2011/05/25 17:21:52, John Abd-El-Malek wrote:
> enums are used when you have a list of related constants.  here you have two
> unrelated constants.  you should just use
> 
> static const int kTryTimes = 1000;
> static const int kDelayMs = 1;

Done.

http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:68: return;
On 2011/05/25 17:21:52, John Abd-El-Malek wrote:
> why add this?  have you tracked a scenario where this is null?

removed. I added it because I am not sure if io thread will be created before
video capture client (maybe this one is not an issue), or outlive video capture
client.

http://codereview.chromium.org/6902166/diff/32004/content/renderer/media/vide...
content/renderer/media/video_capture_impl.cc:86: return;
On 2011/05/25 17:21:52, John Abd-El-Malek wrote:
> ditto

removed.

[jam, 2011-05-25 19:35:46]

lgtm

[jam, 2011-05-25 19:38:34]

On 2011/05/25 19:35:46, John Abd-El-Malek wrote:
> lgtm

sorry, actually hold off on checking this in yet.

You have a loop that keeps checking, up to 1000 times, until the channel is
connected.  That's not a pattern that we use in chrome.

Either you can have the code that you call call you back when OnChannelConnected
is called, or you can have the callee queue messages.

[jam, 2011-05-25 19:41:03]

On 2011/05/25 19:38:34, John Abd-El-Malek wrote:
> On 2011/05/25 19:35:46, John Abd-El-Malek wrote:
> > lgtm
> 
> sorry, actually hold off on checking this in yet.
> 
> You have a loop that keeps checking, up to 1000 times, until the channel is
> connected.  That's not a pattern that we use in chrome.
> 
> Either you can have the code that you call call you back when
OnChannelConnected
> is called, or you can have the callee queue messages.

looking at the code, it seems that that VideoCaptureMessageFilter::Delegate
should have an OnChannelConnected, that it calls when
VideoCaptureMessageFilter::OnChannelConnected is called (which is a method on
VideoCaptureMessageFilter's parent class IPC::ChannelProxy::MessageFilter)

[wjia(left Chromium), 2011-05-25 23:23:47]

On 2011/05/25 19:41:03, John Abd-El-Malek wrote:
> On 2011/05/25 19:38:34, John Abd-El-Malek wrote:
> > On 2011/05/25 19:35:46, John Abd-El-Malek wrote:
> > > lgtm
> > 
> > sorry, actually hold off on checking this in yet.
> > 
> > You have a loop that keeps checking, up to 1000 times, until the channel is
> > connected.  That's not a pattern that we use in chrome.
> > 
> > Either you can have the code that you call call you back when
> OnChannelConnected
> > is called, or you can have the callee queue messages.
> 
> looking at the code, it seems that that VideoCaptureMessageFilter::Delegate
> should have an OnChannelConnected, that it calls when
> VideoCaptureMessageFilter::OnChannelConnected is called (which is a method on
> VideoCaptureMessageFilter's parent class IPC::ChannelProxy::MessageFilter)

AddDelegate() is changed to be asynchronous, and timeout stuff has been removed.

John, Could you take a look?

[jam, 2011-05-26 00:08:08]

lgtm, thanks