Eliminate video capture thread in renderer

Eliminate video capture thread in renderer

The main motivation of this change is to remove the video capture thread
in the renderer. All users of a video capture device already handles the
video frame on their thread. There is no need to call the clients with
an additional thread.

Summary of this change:
* Video capture thread eliminated
  VideoCaptureImpl now runs on the IO thread. Clients are called on the
  IO thread.
* Simplified VideoCaptureImplManager
  We still need to keep this object for the purpose of sharing a
  VideoCaptureImpl object with multiple clients. It should own these
  objects and maintain the usage count. A couple clean up items are done
  on this class:
  * It doesn't own the video capture thread now.
  * It is now a render thread only object.
  * It maintains refcount of a VideoCaptureImpl explicitly.
  * It is no longer refcounted.
  * Clients access it through RenderThreadImpl. Which ensures usage is
    on the render thread.
* New VideoCaptureHandle class
  Object of this class is returned by VideoCaptureImplManager to give
  access to a media::VideoCapture object. It is purely a wrapper and
  helps to do refcounting on the render thread.

Testing:
Added unit tests for VideoCaptureImplManager to test refcounting.
Also updated unit test for VideoCaptureImpl due to the threading
changes.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244074

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244687

[Jói, 2013-12-27 13:34:34]

This change looks really good to me, it simplifies things quite a bit. However,
I don't know this code very well, so you should look for a review from somebody
who does before landing.

Several nits below, nothing big.

Cheers,
Jói

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:191: DLOG(ERROR) <<
"SharedMemory::Map() failed.";
nit: "OnBufferCreated: Map failed." might give more context.

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
content/renderer/media/video_capture_impl.h:22: // VideoCaptureImpl's by calling
DeInit which detours through the capture & I/O
This line references the capture thread, I think you want to remove that bit.

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:15: // destruction handles
on the render thread.
destruction handles on -> destruction is handled on (?)

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager_unittest.cc (right):

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
content/renderer/media/video_capture_impl_manager_unittest.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
2013

[Alpha Left Google, 2013-12-27 21:30:00]

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:191: DLOG(ERROR) <<
"SharedMemory::Map() failed.";
On 2013/12/27 13:34:35, Jói (AWAY UNTIL Jan 6th) wrote:
> nit: "OnBufferCreated: Map failed." might give more context.

Done.

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
content/renderer/media/video_capture_impl.h:22: // VideoCaptureImpl's by calling
DeInit which detours through the capture & I/O
On 2013/12/27 13:34:35, Jói (AWAY UNTIL Jan 6th) wrote:
> This line references the capture thread, I think you want to remove that bit.

Done.

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/120893002/diff/310004/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:15: // destruction handles
on the render thread.
On 2013/12/27 13:34:35, Jói (AWAY UNTIL Jan 6th) wrote:
> destruction handles on -> destruction is handled on (?)

Done.

[wjia(left Chromium), 2013-12-28 02:42:32]

Overall looks good. One comment.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/rtc_video_capture_delegate.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/rtc_video_capture_delegate.cc:22:
RenderThreadImpl::current()->video_capture_impl_manager()
Using RenderThreadImpl::current() will complicate unit test.

[Alpha Left Google, 2014-01-02 23:35:15]

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/rtc_video_capture_delegate.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/rtc_video_capture_delegate.cc:22:
RenderThreadImpl::current()->video_capture_impl_manager()
On 2013/12/28 02:42:32, wjia wrote:
> Using RenderThreadImpl::current() will complicate unit test.

I think this object is created on the render thread by design so I'm not sure
how would a test run it without a render thread.

And RenderThreadImpl is more than just a thread or message loop. It's a
process-wide object that provides a bunch of services. What is how it is used
here. You're supposed to access this object with a static accessor.

For example:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

[Alpha Left Google, 2014-01-06 19:01:26]

Anyone can look at this again? I would like to land this soon.

[wjia(left Chromium), 2014-01-06 19:17:22]

+perkj@ since he created the first version of rtc_video_capture_delegate.cc.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/rtc_video_capture_delegate.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/rtc_video_capture_delegate.cc:22:
RenderThreadImpl::current()->video_capture_impl_manager()
On 2014/01/02 23:35:16, Alpha wrote:
> On 2013/12/28 02:42:32, wjia wrote:
> > Using RenderThreadImpl::current() will complicate unit test.
> 
> I think this object is created on the render thread by design so I'm not sure
> how would a test run it without a render thread.

That's not my understanding. There is no restriction on which thread this class
has to be instantiated. That might be the reason vc_manager is injected in ctor.
Need confirmation from the original author.


> 
> And RenderThreadImpl is more than just a thread or message loop. It's a
> process-wide object that provides a bunch of services. What is how it is used
> here. You're supposed to access this object with a static accessor.
> 
> For example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

[Ami GONE FROM CHROMIUM, 2014-01-06 23:37:13]

I think this CL is a huge improvement!  Thanks for doing this.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/rtc_video_capture_delegate.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/rtc_video_capture_delegate.cc:22:
RenderThreadImpl::current()->video_capture_impl_manager()
On 2014/01/06 19:17:23, wjia wrote:
> On 2014/01/02 23:35:16, Alpha wrote:
> > On 2013/12/28 02:42:32, wjia wrote:
> > > Using RenderThreadImpl::current() will complicate unit test.
> > 
> > I think this object is created on the render thread by design so I'm not
sure
> > how would a test run it without a render thread.
> 
> That's not my understanding. There is no restriction on which thread this
class
> has to be instantiated. That might be the reason vc_manager is injected in
ctor.
> Need confirmation from the original author.
> 
> 
> > 
> > And RenderThreadImpl is more than just a thread or message loop. It's a
> > process-wide object that provides a bunch of services. What is how it is
used
> > here. You're supposed to access this object with a static accessor.
> > 
> > For example:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

My opinion: using RTI this way (instead of injecting the VCIM) _looks_ like a
reduction in testability but in fact does not reduce test coverage because the
"feature" of accepting a VCIM was never taken advantage of to write tests that
utilize it.
Since this class is in content/renderer it makes sense for tests for it to
assume a functional RTI is present.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/rtc_video_capture_delegate.cc:26:
RtcVideoCaptureDelegate::~RtcVideoCaptureDelegate() {
This silently relies on class-member-destruction order.
Is it ok to clear captured_callback_ and state_callback_ before capture_engine_,
for example?

I suspect you either want to tear down the members explicitly here or else doco
in the header that the declaration order is important, hopefully with some
rationale commentary.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:60: if
(!io_message_loop_proxy_->BelongsToCurrentThread()) {
This pattern of conditional trampolining used to be very prevalent in media code
but has been getting expurgated in favor of documenting and enforcing
callers-must-call-on-specific-thread APIs.
It would be very nice to avoid back-sliding here.

For example, I believe this method is always called on the Render thread.  If
that's the case it would be much nicer for it to DCHECK that and then trampoline
to the IO thread (and move the bulk of the method into an InitOnIOThread()
helper, for example).

While it doubles the number of methods, it allows readers to easily tell when a
PostTask will be taken vs. when it's there "just for safety" (a.k.a. "I have no
idea when it'll be taken").

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:68:
message_filter_->AddDelegate(this);
My comment above stands for other methods of this class, but for this particular
method, you could just as well PostTask the AddDelegate call directly.  Just
sayin'.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:71: void
VideoCaptureImpl::DeInit(base::Closure task) {
s/task/done_cb/ or just "done"?
("task" is particularly uninformative, as a name)

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:16: // The
|capture_message_loop_proxy_| is the working thread of VideoCaptureImpl.
This comment is now bogus.  Please replace it with a new comment explicating the
thread-related assumptions of VCI (where can it be called, where does it work,
where can it call back on)

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:54: void SuspendCapture(bool
suspend);
ToggleCapture?
(this predates your CL so fine to leave as is)

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:90: // Sends a IPC message to
browser all clients are done with the video frame.
Please rewrite comment.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:120: // client to this class via
DoStartCaptureOnIOThread.
DoStartCaptureOnIOThread is not a thing.  I think you mean StartCapture().

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:92:
impl->DeInit(base::Bind(&base::DeletePointer<VideoCaptureImpl>, impl));
Any reason to use DeletePointer instead of using base::Owned?

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:96: void
VideoCaptureImplManager::SuspendDevices(bool suspend) {
DCHECK(thread_checker_.CalledOnValidThread());
?

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:6: // want access to a video
capture device calls UseDevice() to get a handle
s/calls/call/

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:11: // This object lives
only on the render thread. All methods must be called
"This object" is unclear.  I believe you refer to VCIM but it's not obvious
you're not referring to VCH or VCI.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:38: // Thread-safe wrapper
for a media::VideoCapture object. This class
Can you drop VCH entirely in favor of RefCountedDeleteOnMessageLoop?

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:39: // ensures refcounting
is done on the render thread.
This sentence (and the corresponding one in the CL description) is a bit
unclear.  I think you mean to say in both places that VCH's purpose in life is
to ensure its underlying VC is deleted on the render thread.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:40: class CONTENT_EXPORT
VideoCaptureHandle : media::VideoCapture {
Is it easy to see why this class belongs in this file?

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:84:
linked_ptr<VideoCaptureImpl> > VideoCaptureDeviceMap;
FWIW if you made VideoCaptureDeviceMap be a 
std::map<
    media::VideoCaptureSessionId,
    std::pair<int,linked_ptr<VideoCaptureImpl> > >
then you could get away with a single map, which might make it easier to see why
you're guaranteed that lookups in the two maps always succeed or fail together.

But if you take my suggestion above to replace VCH with
RefCountedDeleteOnMessageLoop then I think this simplifies to what you have now
and drop use_count_ entirely.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:89:
base::WeakPtrFactory<VideoCaptureImplManager> weak_factory_;
IWBN to comment for both this and the next member that they're bound to the
render thread.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager_unittest.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager_unittest.cc:189:
EXPECT_FALSE(device1 == device2);
How do you guarantee that operator new() doesn't reuse the same address for the
second VC?

https://codereview.chromium.org/120893002/diff/530001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.cc:85: if (video_capture_)
nit: don't need the conditional

https://codereview.chromium.org/120893002/diff/530001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/render...
content/renderer/render_thread_impl.cc:448:
RemoveFilter(vc_manager_->video_capture_message_filter());
Would it make sense to also vc_manager_.reset() here to mimic the 
= NULL;
's above?
(I hope that it's possible to assert at this point that any vended handles have
been freed so that there is no ambiguity about the safety of freeing the VCMI
here; in particular would it make sense to clear media_stream_factory_ in this
function (presumably before this RemoveFilter)?)

https://codereview.chromium.org/120893002/diff/530001/content/renderer/render...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/render...
content/renderer/render_thread_impl.h:95: class VideoCaptureImplManager;
IMO this is worse than #include'ing v_c_i_m.h in this file, because RTI has a
public accessor returning a VCIM*, encouraging its callers to dereference that
pointer and make function calls on the underlying VCIM.  That means _they'll_
need to #include vcim.h, even though it's not obvious why.

Put another way, the public accessor on RTI makes VCIM part of the public API of
RTI, so IMO it is better to fully declare it in this file (via #include) rather
than only fwd-declare it.

Up to the content/ OWNER you get to review this, I suppose, since this file is
such a catch-all, they might prefer this way.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/render...
content/renderer/render_thread_impl.h:435: // Used on multiple threads.
This comment is now a lie.

[perkj_chrome, 2014-01-07 06:21:43]

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/rtc_video_capture_delegate.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/rtc_video_capture_delegate.cc:22:
RenderThreadImpl::current()->video_capture_impl_manager()
On 2014/01/06 23:37:14, Ami Fischman wrote:
> On 2014/01/06 19:17:23, wjia wrote:
> > On 2014/01/02 23:35:16, Alpha wrote:
> > > On 2013/12/28 02:42:32, wjia wrote:
> > > > Using RenderThreadImpl::current() will complicate unit test.
> > > 
> > > I think this object is created on the render thread by design so I'm not
> sure
> > > how would a test run it without a render thread.
> > 
> > That's not my understanding. There is no restriction on which thread this
> class
> > has to be instantiated. That might be the reason vc_manager is injected in
> ctor.
> > Need confirmation from the original author.
> > 
> > 
> > > 
> > > And RenderThreadImpl is more than just a thread or message loop. It's a
> > > process-wide object that provides a bunch of services. What is how it is
> used
> > > here. You're supposed to access this object with a static accessor.
> > > 
> > > For example:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> 
> My opinion: using RTI this way (instead of injecting the VCIM) _looks_ like a
> reduction in testability but in fact does not reduce test coverage because the
> "feature" of accepting a VCIM was never taken advantage of to write tests that
> utilize it.
> Since this class is in content/renderer it makes sense for tests for it to
> assume a functional RTI is present.

Good to know when RTI is ok to use and when not. I would have preferred
dependency injection but since this also make the rest of the code cleaner I am
ok.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/rtc_video_capture_delegate.h (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/rtc_video_capture_delegate.h:45: // These functions are
called from a thread that |capture_engine_| runs on.
While you are at it- make these OVERRIDES protected?
Are these know called on the IO thread? Can you document what thread?

[Alpha Left Google, 2014-01-08 00:23:35]

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/rtc_video_capture_delegate.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/rtc_video_capture_delegate.cc:26:
RtcVideoCaptureDelegate::~RtcVideoCaptureDelegate() {
On 2014/01/06 23:37:14, Ami Fischman wrote:
> This silently relies on class-member-destruction order.
> Is it ok to clear captured_callback_ and state_callback_ before
capture_engine_,
> for example?
> 
> I suspect you either want to tear down the members explicitly here or else
doco
> in the header that the declaration order is important, hopefully with some
> rationale commentary.

I'm calling StopCapture() here such that it explicitly reset the callback and
stop listening to capture events.

VideoCaptureHandle will then do the cleanup implicitly.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/rtc_video_capture_delegate.h (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/rtc_video_capture_delegate.h:45: // These functions are
called from a thread that |capture_engine_| runs on.
On 2014/01/07 06:21:44, perkj wrote:
> While you are at it- make these OVERRIDES protected?
> Are these know called on the IO thread? Can you document what thread?

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:60: if
(!io_message_loop_proxy_->BelongsToCurrentThread()) {
On 2014/01/06 23:37:14, Ami Fischman wrote:
> This pattern of conditional trampolining used to be very prevalent in media
code
> but has been getting expurgated in favor of documenting and enforcing
> callers-must-call-on-specific-thread APIs.
> It would be very nice to avoid back-sliding here.
> 
> For example, I believe this method is always called on the Render thread.  If
> that's the case it would be much nicer for it to DCHECK that and then
trampoline
> to the IO thread (and move the bulk of the method into an InitOnIOThread()
> helper, for example).
> 
> While it doubles the number of methods, it allows readers to easily tell when
a
> PostTask will be taken vs. when it's there "just for safety" (a.k.a. "I have
no
> idea when it'll be taken").

There are two sets of public methods:
* media::VideoCapture interface and
* Init, DeInit and SuspendDevice called by VCIM

I would like to assume they can both be called on any threads. We don't need to
assume in this class that VCIM access VCI only on render thread. Having a DCHECK
on Render thread is probably too much assumptions.

In any case I have changed the style to use a separate method for trampolining.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:68:
message_filter_->AddDelegate(this);
On 2014/01/06 23:37:14, Ami Fischman wrote:
> My comment above stands for other methods of this class, but for this
particular
> method, you could just as well PostTask the AddDelegate call directly.  Just
> sayin'.

Changed it to have a separate trampoline method for consistency.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:71: void
VideoCaptureImpl::DeInit(base::Closure task) {
On 2014/01/06 23:37:14, Ami Fischman wrote:
> s/task/done_cb/ or just "done"?
> ("task" is particularly uninformative, as a name)

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:16: // The
|capture_message_loop_proxy_| is the working thread of VideoCaptureImpl.
On 2014/01/06 23:37:14, Ami Fischman wrote:
> This comment is now bogus.  Please replace it with a new comment explicating
the
> thread-related assumptions of VCI (where can it be called, where does it work,
> where can it call back on)

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:54: void SuspendCapture(bool
suspend);
On 2014/01/06 23:37:14, Ami Fischman wrote:
> ToggleCapture?
> (this predates your CL so fine to leave as is)

I prefer to leave it as is.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:90: // Sends a IPC message to
browser all clients are done with the video frame.
On 2014/01/06 23:37:14, Ami Fischman wrote:
> Please rewrite comment.

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:120: // client to this class via
DoStartCaptureOnIOThread.
On 2014/01/06 23:37:14, Ami Fischman wrote:
> DoStartCaptureOnIOThread is not a thing.  I think you mean StartCapture().

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:92:
impl->DeInit(base::Bind(&base::DeletePointer<VideoCaptureImpl>, impl));
On 2014/01/06 23:37:14, Ami Fischman wrote:
> Any reason to use DeletePointer instead of using base::Owned?

If I do:

scoped_ptr<VideoCaptureImpl> impl(devices_[id].release());
impl->DeInit(base::Bind(base::Owned(&impl), impl));

then the behavior is undefined. I would need an extra variable to carry VCI*
anyway.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:96: void
VideoCaptureImplManager::SuspendDevices(bool suspend) {
On 2014/01/06 23:37:14, Ami Fischman wrote:
> DCHECK(thread_checker_.CalledOnValidThread());
> ?

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:6: // want access to a video
capture device calls UseDevice() to get a handle
On 2014/01/06 23:37:14, Ami Fischman wrote:
> s/calls/call/

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:11: // This object lives
only on the render thread. All methods must be called
On 2014/01/06 23:37:14, Ami Fischman wrote:
> "This object" is unclear.  I believe you refer to VCIM but it's not obvious
> you're not referring to VCH or VCI.

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:38: // Thread-safe wrapper
for a media::VideoCapture object. This class
On 2014/01/06 23:37:14, Ami Fischman wrote:
> Can you drop VCH entirely in favor of RefCountedDeleteOnMessageLoop?

I don't think we can do away without an extra class here for the following
reasons:

1. We can't give scoped_refptr<VideoCaptureImpl> to user. De-initialization of
VideoCaptureImpl is asynchronous and requires more than just destruction.
2. We want to give VideoCaptureImpl to multiple users. The de-initialization of
VCI is done only once when there is no user. Hence VCIM needs to know the
refcount.

The only way to not have VCH is that user return the VCI to VCIM through a
method like VCIM::Release(VCI*). However this adds threading requirements about
where to make this call. And user of VCI can run on any threads which means user
of VCI needs a reference to VCIM and VCIM::Release() needs to handle calls from
any threads.

I would like to hide the threading requirements inside VCIM and the only way I
can think of is to have a separate VCH class.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:39: // ensures refcounting
is done on the render thread.
On 2014/01/06 23:37:14, Ami Fischman wrote:
> This sentence (and the corresponding one in the CL description) is a bit
> unclear.  I think you mean to say in both places that VCH's purpose in life is
> to ensure its underlying VC is deleted on the render thread.

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:40: class CONTENT_EXPORT
VideoCaptureHandle : media::VideoCapture {
On 2014/01/06 23:37:14, Ami Fischman wrote:
> Is it easy to see why this class belongs in this file?

The reason I put this class in this file is because its only user is
VideoCaptureImplManager. I thought of putting it in
media/video/capture/video_capture_handle.h but it seems this mechanism is only
useful because of the threading requirements here. I'm open to suggestions.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:84:
linked_ptr<VideoCaptureImpl> > VideoCaptureDeviceMap;
On 2014/01/06 23:37:14, Ami Fischman wrote:
> FWIW if you made VideoCaptureDeviceMap be a 
> std::map<
>     media::VideoCaptureSessionId,
>     std::pair<int,linked_ptr<VideoCaptureImpl> > >
> then you could get away with a single map, which might make it easier to see
why
> you're guaranteed that lookups in the two maps always succeed or fail
together.
> 
> But if you take my suggestion above to replace VCH with
> RefCountedDeleteOnMessageLoop then I think this simplifies to what you have
now
> and drop use_count_ entirely.

Took your advice and used std::pair<> and removed use_count_.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:89:
base::WeakPtrFactory<VideoCaptureImplManager> weak_factory_;
On 2014/01/06 23:37:14, Ami Fischman wrote:
> IWBN to comment for both this and the next member that they're bound to the
> render thread.

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager_unittest.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager_unittest.cc:189:
EXPECT_FALSE(device1 == device2);
On 2014/01/06 23:37:14, Ami Fischman wrote:
> How do you guarantee that operator new() doesn't reuse the same address for
the
> second VC?

This test verifies that VidoeCaptureImpl is destroyed after the only handle is
gone. It's probably still valuable to exercise this code path but I'll remove
this check.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.cc:85: if (video_capture_)
On 2014/01/06 23:37:14, Ami Fischman wrote:
> nit: don't need the conditional

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/render...
content/renderer/render_thread_impl.cc:448:
RemoveFilter(vc_manager_->video_capture_message_filter());
On 2014/01/06 23:37:14, Ami Fischman wrote:
> Would it make sense to also vc_manager_.reset() here to mimic the 
> = NULL;
> 's above?
> (I hope that it's possible to assert at this point that any vended handles
have
> been freed so that there is no ambiguity about the safety of freeing the VCMI
> here; in particular would it make sense to clear media_stream_factory_ in this
> function (presumably before this RemoveFilter)?)

Done.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/render...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/120893002/diff/530001/content/renderer/render...
content/renderer/render_thread_impl.h:95: class VideoCaptureImplManager;
On 2014/01/06 23:37:14, Ami Fischman wrote:
> IMO this is worse than #include'ing v_c_i_m.h in this file, because RTI has a
> public accessor returning a VCIM*, encouraging its callers to dereference that
> pointer and make function calls on the underlying VCIM.  That means _they'll_
> need to #include vcim.h, even though it's not obvious why.
> 
> Put another way, the public accessor on RTI makes VCIM part of the public API
of
> RTI, so IMO it is better to fully declare it in this file (via #include)
rather
> than only fwd-declare it.
> 
> Up to the content/ OWNER you get to review this, I suppose, since this file is
> such a catch-all, they might prefer this way.

This seems to be the style used throughout this file so I'd just keep this.

https://codereview.chromium.org/120893002/diff/530001/content/renderer/render...
content/renderer/render_thread_impl.h:435: // Used on multiple threads.
On 2014/01/06 23:37:14, Ami Fischman wrote:
> This comment is now a lie.

Done.

[Ami GONE FROM CHROMIUM, 2014-01-08 01:43:58]

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/rtc_video_capture_delegate.h (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/rtc_video_capture_delegate.h:46: // These functions are
called from IO thread that |capture_engine_| runs on.
"that" implies that there might be more than one IO thread.

// These functions are called on the IO thread (same as where |capture_engine_|
runs).

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:59: void VideoCaptureImpl::Init() {
On 2014/01/08 00:23:36, Alpha wrote:
> I would like to assume they can both be called on any threads. 

Why?  What's the benefit?
(the cost is increased opacity in the code)

> In any case I have changed the style to use a separate method for
trampolining.

This unfortunately introduces an unnecessary PostTask when called on the target
thread.  Is there reason to believe that doesn't happen?

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:14: // asynchronous interfaces,
which means callers can call those interfaces
"async interface" doesn't usually mean "call on any thread", so this sentence is
tortured.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:15: // from any threads without
worrying about thread safety. Public methods of
s/any threads/any thread/

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:16: // VideoCaptureImpl can be
called on any threads. Internally it runs on
ditto

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:16: // VideoCaptureImpl can be
called on any threads. Internally it runs on
s/it runs/they run/

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:17: // the IO thread. Client of this
class implements interface
Client...implements 
->
Clients...implement

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:86: if (--it->second.first)
{
is this test backwards?

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:89:
impl->DeInit(base::Bind(&base::DeletePointer<VideoCaptureImpl>, impl));
On 2014/01/08 00:23:36, Alpha wrote:
> On 2014/01/06 23:37:14, Ami Fischman wrote:
> > Any reason to use DeletePointer instead of using base::Owned?
> 
> If I do:
> 
> scoped_ptr<VideoCaptureImpl> impl(devices_[id].release());
> impl->DeInit(base::Bind(base::Owned(&impl), impl));
> 
> then the behavior is undefined. I would need an extra variable to carry VCI*
> anyway.

Why that trailing ",impl"??

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:38: // Thread-safe wrapper
for a media::VideoCapture object. During
On 2014/01/08 00:23:36, Alpha wrote:
> On 2014/01/06 23:37:14, Ami Fischman wrote:
> > Can you drop VCH entirely in favor of RefCountedDeleteOnMessageLoop?
> 
> I don't think we can do away without an extra class here for the following
> reasons:
> 
> 1. We can't give scoped_refptr<VideoCaptureImpl> to user. De-initialization of
> VideoCaptureImpl is asynchronous and requires more than just destruction.
> 2. We want to give VideoCaptureImpl to multiple users. The de-initialization
of
> VCI is done only once when there is no user. Hence VCIM needs to know the
> refcount.
> 
> The only way to not have VCH is that user return the VCI to VCIM through a
> method like VCIM::Release(VCI*). However this adds threading requirements
about
> where to make this call. And user of VCI can run on any threads which means
user
> of VCI needs a reference to VCIM and VCIM::Release() needs to handle calls
from
> any threads.
> 
> I would like to hide the threading requirements inside VCIM and the only way I
> can think of is to have a separate VCH class.

Did you notice I recommended RefCountedDeleteOnMessageLoop, not
RefCountedThreadSafe?  I think this does what you need, already (ref counting
followed by delete on a specific pre-specified thread, regardless of which
thread the last ref is released on)

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:43: class CONTENT_EXPORT
VideoCaptureHandle : media::VideoCapture {
On 2014/01/08 00:23:36, Alpha wrote:
> On 2014/01/06 23:37:14, Ami Fischman wrote:
> > Is it easy to see why this class belongs in this file?
> 
> The reason I put this class in this file is because its only user is
> VideoCaptureImplManager. I thought of putting it in
> media/video/capture/video_capture_handle.h but it seems this mechanism is only
> useful because of the threading requirements here. I'm open to suggestions.

Could make it a public inner class of VCIM.  Not sure it's worth it.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:87: std::pair<int,
linked_ptr<VideoCaptureImpl> > >
doco the int is a ref count

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:92:
base::WeakPtrFactory<VideoCaptureImplManager> weak_factory_;
On 2014/01/08 00:23:36, Alpha wrote:
> On 2014/01/06 23:37:14, Ami Fischman wrote:
> > IWBN to comment for both this and the next member that they're bound to the
> > render thread.
> 
> Done.

I don't see it.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager_unittest.cc (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager_unittest.cc:187:
handle2.reset();
> This test verifies that VidoeCaptureImpl is destroyed after the only handle is
> gone.

Where does that verification happen?

https://codereview.chromium.org/120893002/diff/760001/content/renderer/render...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/render...
content/renderer/render_thread_impl.h:429: // Used on the render thread.
Not sure if this is necessary given the thread-local-on-a-single-thread nature
of RTI, but up to content OWNERS.

[Alpha Left Google, 2014-01-08 22:30:49]

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/rtc_video_capture_delegate.h (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/rtc_video_capture_delegate.h:46: // These functions are
called from IO thread that |capture_engine_| runs on.
On 2014/01/08 01:43:58, Ami Fischman wrote:
> "that" implies that there might be more than one IO thread.
> 
> // These functions are called on the IO thread (same as where
|capture_engine_|
> runs).

Done.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:59: void VideoCaptureImpl::Init() {
On 2014/01/08 01:43:58, Ami Fischman wrote:
> On 2014/01/08 00:23:36, Alpha wrote:
> > I would like to assume they can both be called on any threads. 
> 
> Why?  What's the benefit?
> (the cost is increased opacity in the code)
> 
> > In any case I have changed the style to use a separate method for
> trampolining.
> 
> This unfortunately introduces an unnecessary PostTask when called on the
target
> thread.  Is there reason to believe that doesn't happen?

That's not that case. Init, DeInit are called from VCIM which is from the render
thread. SuspendCapture is originated from RenderThreadImpl which is also on the
render thread.

The actual work is done on the IO thread so a PostTask is necessary.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:15: // from any threads without
worrying about thread safety. Public methods of
On 2014/01/08 01:43:58, Ami Fischman wrote:
> s/any threads/any thread/

Done.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:16: // VideoCaptureImpl can be
called on any threads. Internally it runs on
On 2014/01/08 01:43:58, Ami Fischman wrote:
> ditto

Done.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:17: // the IO thread. Client of this
class implements interface
On 2014/01/08 01:43:58, Ami Fischman wrote:
> Client...implements 
> ->
> Clients...implement

Done.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:86: if (--it->second.first)
{
On 2014/01/08 01:43:58, Ami Fischman wrote:
> is this test backwards?

Good catch. I've separated it into two lines (just like before) to be clear.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:89:
impl->DeInit(base::Bind(&base::DeletePointer<VideoCaptureImpl>, impl));
On 2014/01/08 01:43:58, Ami Fischman wrote:
> On 2014/01/08 00:23:36, Alpha wrote:
> > On 2014/01/06 23:37:14, Ami Fischman wrote:
> > > Any reason to use DeletePointer instead of using base::Owned?
> > 
> > If I do:
> > 
> > scoped_ptr<VideoCaptureImpl> impl(devices_[id].release());
> > impl->DeInit(base::Bind(base::Owned(&impl), impl));
> > 
> > then the behavior is undefined. I would need an extra variable to carry VCI*
> > anyway.
> 
> Why that trailing ",impl"??

I see what you mean now. But I would need a method to receive this scoped_ptr,
e.g.:

void DisposeVideoCaptureImpl(scoped_ptr<VideoCaptureImpl>);
impl->DeInit(base::Bind(&DisposeVideoCaptureImpl, base::Owned(impl)));

Using DeletePointer seems cleaner to me.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:38: // Thread-safe wrapper
for a media::VideoCapture object. During
On 2014/01/08 01:43:58, Ami Fischman wrote:
> On 2014/01/08 00:23:36, Alpha wrote:
> > On 2014/01/06 23:37:14, Ami Fischman wrote:
> > > Can you drop VCH entirely in favor of RefCountedDeleteOnMessageLoop?
> > 
> > I don't think we can do away without an extra class here for the following
> > reasons:
> > 
> > 1. We can't give scoped_refptr<VideoCaptureImpl> to user. De-initialization
of
> > VideoCaptureImpl is asynchronous and requires more than just destruction.
> > 2. We want to give VideoCaptureImpl to multiple users. The de-initialization
> of
> > VCI is done only once when there is no user. Hence VCIM needs to know the
> > refcount.
> > 
> > The only way to not have VCH is that user return the VCI to VCIM through a
> > method like VCIM::Release(VCI*). However this adds threading requirements
> about
> > where to make this call. And user of VCI can run on any threads which means
> user
> > of VCI needs a reference to VCIM and VCIM::Release() needs to handle calls
> from
> > any threads.
> > 
> > I would like to hide the threading requirements inside VCIM and the only way
I
> > can think of is to have a separate VCH class.
> 
> Did you notice I recommended RefCountedDeleteOnMessageLoop, not
> RefCountedThreadSafe?  I think this does what you need, already (ref counting
> followed by delete on a specific pre-specified thread, regardless of which
> thread the last ref is released on)

Yes I have considered your suggestion.

Currently work done when a VCI is released are: unref, VCI::DeInitOnIOThread()
and then delete. This can be done by RefCountedDeleteOnMessageLoop I think, by
calling VCI::DeInitOnIOThread() in VCI::~VCI().

However a reference to VCI must be saved in VCIM such that it can be shared to
multiple users. If VCI is RefCountedDeleteOnMessageLoop then we would a method
like VCIM::Release(VCI*) to remove this reference held by VCIM.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:92:
base::WeakPtrFactory<VideoCaptureImplManager> weak_factory_;
On 2014/01/08 01:43:58, Ami Fischman wrote:
> On 2014/01/08 00:23:36, Alpha wrote:
> > On 2014/01/06 23:37:14, Ami Fischman wrote:
> > > IWBN to comment for both this and the next member that they're bound to
the
> > > render thread.
> > 
> > Done.
> 
> I don't see it.

Done.

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
File content/renderer/media/video_capture_impl_manager_unittest.cc (right):

https://codereview.chromium.org/120893002/diff/760001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager_unittest.cc:187:
handle2.reset();
On 2014/01/08 01:43:58, Ami Fischman wrote:
> > This test verifies that VidoeCaptureImpl is destroyed after the only handle
is
> > gone.
> 
> Where does that verification happen?

I meant the test was supposed to verify that VCI is deleted. But because the
check can fail like you suggested so I took away.

I've used a different approach to verify this.

[Ami GONE FROM CHROMIUM, 2014-01-08 22:41:09]

LGTM % nits.

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
content/renderer/media/video_capture_impl.h:15: // be called on any thread.
Internally it runs on the IO thread. Client of this
Do the first two sentences of this paragraph summarize to "All public methods
can be called on any threads."?

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
content/renderer/media/video_capture_impl.h:15: // be called on any thread.
Internally it runs on the IO thread. Client of this
I think you missed this comment:

On 2014/01/08 01:43:58, Ami Fischman wrote:
> s/it runs/they run/

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
content/renderer/media/video_capture_impl.h:16: // class implements interface
media::VideoCapture::EventHandler which is called
On 2014/01/08 22:30:50, Alpha wrote:
> On 2014/01/08 01:43:58, Ami Fischman wrote:
> > Client...implements 
> > ->
> > Clients...implement
> 
> Done.

I don't see it.

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
content/renderer/media/video_capture_impl_manager.h:38: // Thread-safe wrapper
for a media::VideoCapture object. During
On 2014/01/08 22:30:50, Alpha wrote:
> On 2014/01/08 01:43:58, Ami Fischman wrote:
> > On 2014/01/08 00:23:36, Alpha wrote:
> > > On 2014/01/06 23:37:14, Ami Fischman wrote:
> > > > Can you drop VCH entirely in favor of RefCountedDeleteOnMessageLoop?
> > > 
> > > I don't think we can do away without an extra class here for the following
> > > reasons:
> > > 
> > > 1. We can't give scoped_refptr<VideoCaptureImpl> to user.
De-initialization
> of
> > > VideoCaptureImpl is asynchronous and requires more than just destruction.
> > > 2. We want to give VideoCaptureImpl to multiple users. The
de-initialization
> > of
> > > VCI is done only once when there is no user. Hence VCIM needs to know the
> > > refcount.
> > > 
> > > The only way to not have VCH is that user return the VCI to VCIM through a
> > > method like VCIM::Release(VCI*). However this adds threading requirements
> > about
> > > where to make this call. And user of VCI can run on any threads which
means
> > user
> > > of VCI needs a reference to VCIM and VCIM::Release() needs to handle calls
> > from
> > > any threads.
> > > 
> > > I would like to hide the threading requirements inside VCIM and the only
way
> I
> > > can think of is to have a separate VCH class.
> > 
> > Did you notice I recommended RefCountedDeleteOnMessageLoop, not
> > RefCountedThreadSafe?  I think this does what you need, already (ref
counting
> > followed by delete on a specific pre-specified thread, regardless of which
> > thread the last ref is released on)
> 
> Yes I have considered your suggestion.
> 
> Currently work done when a VCI is released are: unref, VCI::DeInitOnIOThread()
> and then delete. This can be done by RefCountedDeleteOnMessageLoop I think, by
> calling VCI::DeInitOnIOThread() in VCI::~VCI().
> 
> However a reference to VCI must be saved in VCIM such that it can be shared to
> multiple users. If VCI is RefCountedDeleteOnMessageLoop then we would a method
> like VCIM::Release(VCI*) to remove this reference held by VCIM. 

That seems like a great tradeoff to me in terms of reduced code complexity. 
Fine to do in another CL (or not, if you dislike it).

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
content/renderer/media/video_capture_impl_manager.h:84: virtual
VideoCaptureImpl* CreateVideoCaptureImpl(
Comment this is protected for testing (mock).

[Alpha Left Google, 2014-01-08 22:57:04]

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
content/renderer/media/video_capture_impl.h:15: // be called on any thread.
Internally it runs on the IO thread. Client of this
On 2014/01/08 22:41:10, Ami Fischman wrote:
> Do the first two sentences of this paragraph summarize to "All public methods
> can be called on any threads."?

Done.

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
content/renderer/media/video_capture_impl.h:16: // class implements interface
media::VideoCapture::EventHandler which is called
On 2014/01/08 22:41:10, Ami Fischman wrote:
> On 2014/01/08 22:30:50, Alpha wrote:
> > On 2014/01/08 01:43:58, Ami Fischman wrote:
> > > Client...implements 
> > > ->
> > > Clients...implement
> > 
> > Done.
> 
> I don't see it.

Done.

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
content/renderer/media/video_capture_impl_manager.h:38: // Thread-safe wrapper
for a media::VideoCapture object. During
On 2014/01/08 22:41:10, Ami Fischman wrote:
> On 2014/01/08 22:30:50, Alpha wrote:
> > On 2014/01/08 01:43:58, Ami Fischman wrote:
> > > On 2014/01/08 00:23:36, Alpha wrote:
> > > > On 2014/01/06 23:37:14, Ami Fischman wrote:
> > > > > Can you drop VCH entirely in favor of RefCountedDeleteOnMessageLoop?
> > > > 
> > > > I don't think we can do away without an extra class here for the
following
> > > > reasons:
> > > > 
> > > > 1. We can't give scoped_refptr<VideoCaptureImpl> to user.
> De-initialization
> > of
> > > > VideoCaptureImpl is asynchronous and requires more than just
destruction.
> > > > 2. We want to give VideoCaptureImpl to multiple users. The
> de-initialization
> > > of
> > > > VCI is done only once when there is no user. Hence VCIM needs to know
the
> > > > refcount.
> > > > 
> > > > The only way to not have VCH is that user return the VCI to VCIM through
a
> > > > method like VCIM::Release(VCI*). However this adds threading
requirements
> > > about
> > > > where to make this call. And user of VCI can run on any threads which
> means
> > > user
> > > > of VCI needs a reference to VCIM and VCIM::Release() needs to handle
calls
> > > from
> > > > any threads.
> > > > 
> > > > I would like to hide the threading requirements inside VCIM and the only
> way
> > I
> > > > can think of is to have a separate VCH class.
> > > 
> > > Did you notice I recommended RefCountedDeleteOnMessageLoop, not
> > > RefCountedThreadSafe?  I think this does what you need, already (ref
> counting
> > > followed by delete on a specific pre-specified thread, regardless of which
> > > thread the last ref is released on)
> > 
> > Yes I have considered your suggestion.
> > 
> > Currently work done when a VCI is released are: unref,
VCI::DeInitOnIOThread()
> > and then delete. This can be done by RefCountedDeleteOnMessageLoop I think,
by
> > calling VCI::DeInitOnIOThread() in VCI::~VCI().
> > 
> > However a reference to VCI must be saved in VCIM such that it can be shared
to
> > multiple users. If VCI is RefCountedDeleteOnMessageLoop then we would a
method
> > like VCIM::Release(VCI*) to remove this reference held by VCIM. 
> 
> That seems like a great tradeoff to me in terms of reduced code complexity. 
> Fine to do in another CL (or not, if you dislike it).

A tradeoff would be to have UseDevice return a media::VideoCapture* and a
callback. The callback is used to return the VC* when done. This way don't need
VCH and takes care of the threading requirement of calling VCIM. I can probably
do this in a separate CL.

https://codereview.chromium.org/120893002/diff/1080001/content/renderer/media...
content/renderer/media/video_capture_impl_manager.h:84: virtual
VideoCaptureImpl* CreateVideoCaptureImpl(
On 2014/01/08 22:41:10, Ami Fischman wrote:
> Comment this is protected for testing (mock).

Done.

[Alpha Left Google, 2014-01-08 22:57:23]

+jamesr for changes in content/renderer/render_thread_impl.{cc,h}.

[Alpha Left Google, 2014-01-08 23:10:04]

Actually +jamesr this time.

[jamesr, 2014-01-08 23:11:45]

lgtm

[Alpha Left Google, 2014-01-08 23:46:59]

+jam for content/content_tests.gypi

[jam, 2014-01-09 18:12:44]

On 2014/01/08 23:46:59, Alpha wrote:
> +jam for content/content_tests.gypi

lgtm. please feel free to tbr simple additions/removals from gypi files in the
future

[commit-bot: I haz the power, 2014-01-09 20:54:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/120893002/1340001

[commit-bot: I haz the power, 2014-01-09 23:19:20]

Failed to apply patch for content/renderer/media/video_capture_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/media/video_capture_impl.cc
  Hunk #8 FAILED at 239.
  Hunk #9 succeeded at 248 (offset -2 lines).
  Hunk #10 succeeded at 300 (offset -2 lines).
  Hunk #11 succeeded at 314 (offset -2 lines).
  Hunk #12 succeeded at 325 (offset -2 lines).
  Hunk #13 succeeded at 349 (offset -2 lines).
  Hunk #14 succeeded at 365 (offset -2 lines).
  1 out of 14 hunks FAILED -- saving rejects to file
content/renderer/media/video_capture_impl.cc.rej

Patch:       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
066daf2a1a286bad086f0967956d9e6efc529754..26048d4139aa3f402dc455e0a132519b4b9cd85b
100644
--- a/content/renderer/media/video_capture_impl.cc
+++ b/content/renderer/media/video_capture_impl.cc
@@ -42,11 +42,9 @@ int VideoCaptureImpl::CaptureFrameRate() {
 
 VideoCaptureImpl::VideoCaptureImpl(
     const media::VideoCaptureSessionId session_id,
-    base::MessageLoopProxy* capture_message_loop_proxy,
     VideoCaptureMessageFilter* filter)
     : VideoCapture(),
       message_filter_(filter),
-      capture_message_loop_proxy_(capture_message_loop_proxy),
       io_message_loop_proxy_(ChildProcess::current()->io_message_loop_proxy()),
       device_id_(0),
       session_id_(session_id),
@@ -59,90 +57,62 @@ VideoCaptureImpl::VideoCaptureImpl(
 VideoCaptureImpl::~VideoCaptureImpl() {}
 
 void VideoCaptureImpl::Init() {
-  if (!io_message_loop_proxy_->BelongsToCurrentThread()) {
-    io_message_loop_proxy_->PostTask(FROM_HERE,
-        base::Bind(&VideoCaptureImpl::AddDelegateOnIOThread,
-                   base::Unretained(this)));
-  } else {
-    AddDelegateOnIOThread();
-  }
+  io_message_loop_proxy_->PostTask(FROM_HERE,
+      base::Bind(&VideoCaptureImpl::InitOnIOThread,
+                 base::Unretained(this)));
 }
 
-void VideoCaptureImpl::DeInit(base::Closure task) {
-  capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoDeInitOnCaptureThread,
-                 base::Unretained(this), task));
+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) {
-  capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoStartCaptureOnCaptureThread,
+  io_message_loop_proxy_->PostTask(FROM_HERE,
+      base::Bind(&VideoCaptureImpl::StartCaptureOnIOThread,
                  base::Unretained(this), handler, params));
 }
 
-void VideoCaptureImpl::StopCapture(media::VideoCapture::EventHandler* handler)
{
-  capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoStopCaptureOnCaptureThread,
+void VideoCaptureImpl::StopCapture(
+    media::VideoCapture::EventHandler* handler) {
+  io_message_loop_proxy_->PostTask(FROM_HERE,
+      base::Bind(&VideoCaptureImpl::StopCaptureOnIOThread,
                  base::Unretained(this), handler));
 }
 
-void VideoCaptureImpl::OnBufferCreated(
-    base::SharedMemoryHandle handle,
-    int length, int buffer_id) {
-  capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoBufferCreatedOnCaptureThread,
-                 base::Unretained(this), handle, length, buffer_id));
-}
-
-void VideoCaptureImpl::OnBufferDestroyed(int buffer_id) {
-  capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoBufferDestroyedOnCaptureThread,
-                 base::Unretained(this), buffer_id));
-}
-
-void VideoCaptureImpl::OnBufferReceived(
-    int buffer_id,
-    base::TimeTicks timestamp,
-    const media::VideoCaptureFormat& format) {
-  capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoBufferReceivedOnCaptureThread,
-                 base::Unretained(this), buffer_id, timestamp, format));
-}
-
-void VideoCaptureImpl::OnStateChanged(VideoCaptureState state) {
-  capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoStateChangedOnCaptureThread,
-                 base::Unretained(this), state));
-}
-
-void VideoCaptureImpl::OnDelegateAdded(int32 device_id) {
-  capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoDelegateAddedOnCaptureThread,
-                 base::Unretained(this), device_id));
-}
-
-void VideoCaptureImpl::SuspendCapture(bool suspend) {
-  capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoSuspendCaptureOnCaptureThread,
-                 base::Unretained(this), suspend));
+void VideoCaptureImpl::InitOnIOThread() {
+  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  message_filter_->AddDelegate(this);
 }
 
-void VideoCaptureImpl::DoDeInitOnCaptureThread(base::Closure task) {
+void VideoCaptureImpl::DeInitOnIOThread(base::Closure done_cb) {
+  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
   if (state_ == VIDEO_CAPTURE_STATE_STARTED)
     Send(new VideoCaptureHostMsg_Stop(device_id_));
+  message_filter_->RemoveDelegate(this);
+  done_cb.Run();
+}
 
-  io_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::RemoveDelegateOnIOThread,
-                 base::Unretained(this), task));
+void VideoCaptureImpl::SuspendCaptureOnIOThread(bool suspend) {
+  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  suspended_ = suspend;
 }
 
-void VideoCaptureImpl::DoStartCaptureOnCaptureThread(
+void VideoCaptureImpl::StartCaptureOnIOThread(
     media::VideoCapture::EventHandler* handler,
     const media::VideoCaptureParams& params) {
-  DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
-
+  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
   if (state_ == VIDEO_CAPTURE_STATE_ERROR) {
     handler->OnError(this, 1);
     handler->OnRemoved(this);
@@ -183,9 +153,9 @@ void VideoCaptureImpl::DoStartCaptureOnCaptureThread(
   }
 }
 
-void VideoCaptureImpl::DoStopCaptureOnCaptureThread(
+void VideoCaptureImpl::StopCaptureOnIOThread(
     media::VideoCapture::EventHandler* handler) {
-  DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
 
   // A handler can be in only one client list.
   // If this handler is in any client list, we can just remove it from
@@ -202,10 +172,10 @@ void VideoCaptureImpl::DoStopCaptureOnCaptureThread(
   }
 }
 
-void VideoCaptureImpl::DoBufferCreatedOnCaptureThread(
+void VideoCaptureImpl::OnBufferCreated(
     base::SharedMemoryHandle handle,
     int length, int buffer_id) {
-  DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
 
   // In case client calls StopCapture before the arrival of created buffer,
   // just close this buffer and return.
@@ -216,7 +186,7 @@ void VideoCaptureImpl::DoBufferCreatedOnCaptureThread(
 
   scoped_ptr<base::SharedMemory> shm(new base::SharedMemory(handle, false));
   if (!shm->Map(length)) {
-    DLOG(ERROR) << "DoBufferCreatedOnCaptureThread: Map() failed.";
+    DLOG(ERROR) << "OnBufferCreated: Map failed.";
     return;
   }
 
@@ -228,8 +198,8 @@ void VideoCaptureImpl::DoBufferCreatedOnCaptureThread(
   DCHECK(inserted);
 }
 
-void VideoCaptureImpl::DoBufferDestroyedOnCaptureThread(int buffer_id) {
-  DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
+void VideoCaptureImpl::OnBufferDestroyed(int buffer_id) {
+  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
 
   ClientBufferMap::iterator iter = client_buffers_.find(buffer_id);
   if (iter == client_buffers_.end())
@@ -240,11 +210,11 @@ void
VideoCaptureImpl::DoBufferDestroyedOnCaptureThread(int buffer_id) {
   client_buffers_.erase(iter);
 }
 
-void VideoCaptureImpl::DoBufferReceivedOnCaptureThread(
+void VideoCaptureImpl::OnBufferReceived(
     int buffer_id,
     base::TimeTicks timestamp,
     const media::VideoCaptureFormat& format) {
-  DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
 
   if (state_ != VIDEO_CAPTURE_STATE_STARTED || suspended_) {
     Send(new VideoCaptureHostMsg_BufferReady(device_id_, buffer_id));
@@ -269,9 +239,9 @@ void VideoCaptureImpl::DoBufferReceivedOnCaptureThread(
           buffer->buffer->handle(),
           timestamp - first_frame_timestamp_,
           media::BindToLoop(
-              capture_message_loop_proxy_,
+              io_message_loop_proxy_,
               base::Bind(
-                  &VideoCaptureImpl::DoClientBufferFinishedOnCaptureThread,
+                  &VideoCaptureImpl::OnClientBufferFinished,
                   weak_this_factory_.GetWeakPtr(),
                   buffer_id,
                   buffer)));
@@ -280,15 +250,15 @@ void VideoCaptureImpl::DoBufferReceivedOnCaptureThread(
     it->first->OnFrameReady(this, frame);
 }
 
-void VideoCaptureImpl::DoClientBufferFinishedOnCaptureThread(
+void VideoCaptureImpl::OnClientBufferFinished(
     int buffer_id,
     const scoped_refptr<ClientBuffer>& buffer) {
-  DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
   Send(new VideoCaptureHostMsg_BufferReady(device_id_, buffer_id));
 }
 
-void VideoCaptureImpl::DoSt…
(message too large)

[commit-bot: I haz the power, 2014-01-10 00:27:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/120893002/1660001

[commit-bot: I haz the power, 2014-01-10 03:39:31]

Change committed as 244074

[Alpha Left Google, 2014-01-13 01:14:03]

fischman: can you look at patchset 12. This patch was reverted because of a TSan
warning. The additional change in patchset 12 will fix it.

[Alpha Left Google, 2014-01-13 01:14:47]

On 2014/01/13 01:14:03, Alpha wrote:
> fischman: can you look at patchset 12. This patch was reverted because of a
TSan
> warning. The additional change in patchset 12 will fix it.

I meant: can you look at it please? :)

[mcasas, 2014-01-13 13:46:41]

https://codereview.chromium.org/120893002/diff/1960001/content/renderer/media...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/120893002/diff/1960001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:170: client_buffers_.clear();
Alpha: I rebased my CL against this landed code and when
running video_capture_impl_unittests I indeed hit a race
in either l.170 or l.171, most concretely in the "Simple"
UT.

The rest of the UT are running but not doing what they are
supposed to do, i.e. if they are supposed to start/stop
two clients, I only see one being started/stopped.

[Alpha Left Google, 2014-01-13 17:02:36]

On 2014/01/13 13:46:41, mcasas wrote:
>
https://codereview.chromium.org/120893002/diff/1960001/content/renderer/media...
> File content/renderer/media/video_capture_impl.cc (right):
> 
>
https://codereview.chromium.org/120893002/diff/1960001/content/renderer/media...
> content/renderer/media/video_capture_impl.cc:170: client_buffers_.clear();
> Alpha: I rebased my CL against this landed code and when
> running video_capture_impl_unittests I indeed hit a race
> in either l.170 or l.171, most concretely in the "Simple"
> UT.
> 
> The rest of the UT are running but not doing what they are
> supposed to do, i.e. if they are supposed to start/stop
> two clients, I only see one being started/stopped.

Which test did you see the race? TSan complains only about
VideoCaptureImplManagerTest so I'm not aware of failures in
video_capture_impl_unittests.cc.

[Alpha Left Google, 2014-01-13 17:26:38]

On 2014/01/13 17:02:36, Alpha wrote:
> On 2014/01/13 13:46:41, mcasas wrote:
> >
>
https://codereview.chromium.org/120893002/diff/1960001/content/renderer/media...
> > File content/renderer/media/video_capture_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/120893002/diff/1960001/content/renderer/media...
> > content/renderer/media/video_capture_impl.cc:170: client_buffers_.clear();
> > Alpha: I rebased my CL against this landed code and when
> > running video_capture_impl_unittests I indeed hit a race
> > in either l.170 or l.171, most concretely in the "Simple"
> > UT.
> > 
> > The rest of the UT are running but not doing what they are
> > supposed to do, i.e. if they are supposed to start/stop
> > two clients, I only see one being started/stopped.
> 
> Which test did you see the race? TSan complains only about
> VideoCaptureImplManagerTest so I'm not aware of failures in
> video_capture_impl_unittests.cc.

You're right. There's a race I wasn't aware of. Looking into it now.

[Ami GONE FROM CHROMIUM, 2014-01-13 18:26:36]

https://codereview.chromium.org/120893002/diff/1960001/content/renderer/media...
File content/renderer/media/video_capture_impl_manager_unittest.cc (right):

https://codereview.chromium.org/120893002/diff/1960001/content/renderer/media...
content/renderer/media/video_capture_impl_manager_unittest.cc:150: //
destruction sequence of VideoCaptureImpl.
If the thing you're trying to assert here is that resetting the handles triggers
the device destructor then I think you can avoid the knowledge of dtor sequence
by giving the MockVideoCaptureImpl a set_dtor_cb method which would accept a
Closure and run it in the dtor, and then here replace l.145-168 with:
base::RunLoop run_loop;
device1.set_dtor_cb(run_loop.QuitClosure());
handle1.reset();
handle2.reset();
run_loop.Run();

No?

[Alpha Left Google, 2014-01-13 21:50:03]

On 2014/01/13 18:26:36, Ami Fischman wrote:
>
https://codereview.chromium.org/120893002/diff/1960001/content/renderer/media...
> File content/renderer/media/video_capture_impl_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/120893002/diff/1960001/content/renderer/media...
> content/renderer/media/video_capture_impl_manager_unittest.cc:150: //
> destruction sequence of VideoCaptureImpl.
> If the thing you're trying to assert here is that resetting the handles
triggers
> the device destructor then I think you can avoid the knowledge of dtor
sequence
> by giving the MockVideoCaptureImpl a set_dtor_cb method which would accept a
> Closure and run it in the dtor, and then here replace l.145-168 with:
> base::RunLoop run_loop;
> device1.set_dtor_cb(run_loop.QuitClosure());
> handle1.reset();
> handle2.reset();
> run_loop.Run();
> 
> No?

There were two races. One is in VideoCaptureImplManagerTest that waiting for VCI
to destruct is necessary. I used fischman@'s advice and used a callback to quit
the run loop. I couldn't set the callback directly because of VideoCaptureHandle
in between.

The other race was in VideoCaptureImplTest. It turns out that even after
OnStopped() is called there's still some activities. I need to call DeInit()
with a callback to safely shutdown.

Ran Tsan without error.

[Ami GONE FROM CHROMIUM, 2014-01-13 22:15:03]

PS#14 LGTM

https://codereview.chromium.org/120893002/diff/2630001/content/renderer/media...
File content/renderer/media/video_capture_impl_manager_unittest.cc (right):

https://codereview.chromium.org/120893002/diff/2630001/content/renderer/media...
content/renderer/media/video_capture_impl_manager_unittest.cc:53: :
destruct_callback_(destruct_callback) {}
explicit

https://codereview.chromium.org/120893002/diff/2630001/content/renderer/media...
content/renderer/media/video_capture_impl_manager_unittest.cc:53: :
destruct_callback_(destruct_callback) {}
member & arg names fail to connote that this cb is for the mvci's, not the
mvcim's, dtor.

[commit-bot: I haz the power, 2014-01-14 07:32:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/120893002/2690001

[commit-bot: I haz the power, 2014-01-14 13:51:50]

Change committed as 244687