Video Capture: Allow suspension of individual devices.

Video Capture: Allow suspension of individual devices.

Adds infrastructure to notify started video capture devices when there
are no consumers present, so that they may turn down to an "idle," low
resource utilization state. There are legitimate use cases where
MediaStream sessions are purposely left open without any sinks attached
for significant periods of time, and then attached when the application
is ready to consume the video.

This is a prerequisite to a soon-upcoming change that will allow screen
capture devices, which are very resource intensive, to suspend.

BUG=650113
Committed: https://crrev.com/1c98a2e10b2505761d1f88880e3c8267951d30e2
Cr-Commit-Position: refs/heads/master@{#421705}

[miu, 2016-09-25 23:09:47]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-25 23:09:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[miu, 2016-09-25 23:16:13]

miu@chromium.org changed reviewers:
+ emircan@chromium.org, mcasas@chromium.org

[miu, 2016-09-25 23:16:14]

emircan: PTAL at content/renderer/media/media_stream*

mcasas: PTAL at the rest (i.e., the video capture stack; all files whose
basenames start with video_capture).

[miu, 2016-09-25 23:27:16]

+cc chfremer and braveyao (FYI)

[miu, 2016-09-25 23:43:01]

...and the follow-up change that implements suspension for the screen capture
devices: https://codereview.chromium.org/2364413002/

[commit-bot: I haz the power, 2016-09-26 00:31:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 00:32:00]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[chfremer, 2016-09-27 00:54:58]

chfremer@chromium.org changed reviewers:
+ chfremer@chromium.org

[chfremer, 2016-09-27 00:54:59]

Some drive-by comments and questions.

https://codereview.chromium.org/2365223002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2365223002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:750: }
We are changing the behavior of VideoCaptureManager here, but this does not seem
to affect the existing unit tests. Is there a gap in the test coverage?

The test case PauseAndResumeClient exercises this code path, but it does not
verify any calls being made from here to the device, which it should.

What the test case does verify is calls made to an Observer (via interface
MediaStreamProviderListener). Would such an Observer be interested in Suspended
and Resumed events?

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_capturer_source.cc:373: }
I see a slight change of behavior here. Before, device start was reported as
failure in more of the cases. 
Did it actually happen that one of these states were called before one of the
others and was this a bug?
Or is this change just to handle it "more correctly" in case of future changes?

Are there any tests for this?

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_source.h:76: void
UpdateHasConsumers(MediaStreamVideoTrack* track, bool has_consumers);
As a reader unfamiliar with this class, I am confused about the two methods
UpdateHasConsumers() and SetHasConsumers() that based on their name to serve the
same purpose. The comment saying that one of the methods is called by someone
particular (|track|) seems to indicate that different clients of this class
interface with different subsets of the public methods. 

If that is the case, it may be helpful to expose separate interfaces for these
different types of clients in order to make the interactions between the
instances more clear.

Could it be that SetHasConsumers() is only called by the implementation of
UpdateHasConsumers() itself?
If that is the case, does it need to be part of this interface or could it
become a private methods?

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/vide...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/vide...
content/renderer/media/video_capture_impl_manager.h:98: void SuspendDevices(bool
suspend);
Deciding which devices need to be suspended/resumed based on state for both
individual devices plus a global flag seems to be enough responsibility for a
class of its own. Furthermore it seems unrelated to the responsibility this
class has had so far, which is to communicate with the Browser process through
IPC.

Adding this responsibility to this existing class makes this class quite large
and mixes responsibilities. Can we split the suspend/resume functionality out
into a new class and test it in isolation?

https://codereview.chromium.org/2365223002/diff/1/media/capture/video/video_c...
File media/capture/video/video_capture_device.h (right):

https://codereview.chromium.org/2365223002/diff/1/media/capture/video/video_c...
media/capture/video/video_capture_device.h:170: // Optionally suspends frame
delivery. The video capturer may or may not honor
nit: "video capturer" -> "VideoCaptureDevice" or "implementation".

https://codereview.chromium.org/2365223002/diff/1/media/capture/video/video_c...
media/capture/video/video_capture_device.h:172: // while there are no frame
consumers present.
nit: The names "Suspend" and "Resume" already somewhat imply this, but we could
state more explicitly that suspending and resuming is supposed to be
significantly faster than stopping and starting. Or is it legal for
implementations to internally use stop and start as long as that is "fast
enough"?

[miu, 2016-09-27 23:42:23]

chfremer: Addressed your comments for patch set 2:

https://codereview.chromium.org/2365223002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2365223002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:750: }
On 2016/09/27 00:54:59, chfremer wrote:
> We are changing the behavior of VideoCaptureManager here, but this does not
seem
> to affect the existing unit tests. Is there a gap in the test coverage?

Seems so. I added some extra testing around this to check whether
VideoCaptureDevice::MaybeSuspend() and Resume() are called.

Note: In the follow-up change, the tab capture browser tests are also testing
this functionality (as part of an end-to-end test).

> What the test case does verify is calls made to an Observer (via interface
> MediaStreamProviderListener). Would such an Observer be interested in
Suspended
> and Resumed events?

The MediaStreamManager handles session-level events. I don't see a use case for
it to act on internal events like this one, so I don't think it would be
appropriate to add anything to that interface.

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_capturer_source.cc:373: }
On 2016/09/27 00:54:59, chfremer wrote:
> I see a slight change of behavior here. Before, device start was 
> ...
> Or is this change just to handle it "more correctly" in case of future
changes?

The purpose of this function was to report on whether the start succeeded
(because running_callback_ calls MediaStreamVideoSource::OnStartDone()). So,
yes, the prior implementation was wrong.

I realized it was wrong when I added the VIDEO_CAPTURE_STATE_PAUSED/RESUMED
status updates from the VideoCaptureHost, and this was calling OnStartDone()
multiple times.

> Are there any tests for this?

Not that I can tell. However, all the MediaStream-related stuff is covered in
layout tests. Also, my follow-up change requires this to work correctly (and
provides test coverage in the tab capture end-to-end browser tests).

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_source.h:76: void
UpdateHasConsumers(MediaStreamVideoTrack* track, bool has_consumers);
On 2016/09/27 00:54:59, chfremer wrote:
> As a reader unfamiliar with this class, I am confused about the two methods
> UpdateHasConsumers() and SetHasConsumers() that based on their name to serve
the
> same purpose.
> ...
> If that is the case, does it need to be part of this interface or could it
> become a private methods?

Done. Made the SetXXX() methods protected, renamed them to OnXXX(), and modified
method comments to better indicate they are meant to be optionally overridden by
subclasses.

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/vide...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/vide...
content/renderer/media/video_capture_impl_manager.h:98: void SuspendDevices(bool
suspend);
On 2016/09/27 00:54:59, chfremer wrote:
> Furthermore it seems unrelated to the responsibility this
> class has had so far, which is to communicate with the Browser process through
> IPC.

No, that's not the scoping/responsibility of this class. It doesn't make nor
process any IPC calls. Its purpose is to control an individual VideoCaptureImpl
and its lifetime based on multiple clients needing shared access to a single
instance.

There's also the threading concern: This is the main thread's interface to the
VideoCaptureImpls that run on the IO thread.

Granted, there's certainly room for clean-up here. These classes have evolved a
lot, and the comments, naming, and code factoring need a lot of improvement.

> Adding this responsibility to this existing class makes this class quite large
> and mixes responsibilities. Can we split the suspend/resume functionality out
> into a new class and test it in isolation?

For reasons stated above, I believe mitigating the global versus individual
suspend/resume should be a responsibility of this class. It would
over-complexify things to split-out the functionality into a separate class,
because now you'd have two classes that have to synchronize with each other, and
with each maintaining similar state. If you disagree, feel free to show me the
code. ;-)

https://codereview.chromium.org/2365223002/diff/1/media/capture/video/video_c...
File media/capture/video/video_capture_device.h (right):

https://codereview.chromium.org/2365223002/diff/1/media/capture/video/video_c...
media/capture/video/video_capture_device.h:170: // Optionally suspends frame
delivery. The video capturer may or may not honor
On 2016/09/27 00:54:59, chfremer wrote:
> nit: "video capturer" -> "VideoCaptureDevice" or "implementation".

Done.

https://codereview.chromium.org/2365223002/diff/1/media/capture/video/video_c...
media/capture/video/video_capture_device.h:172: // while there are no frame
consumers present.
On 2016/09/27 00:54:59, chfremer wrote:
> nit: The names "Suspend" and "Resume" already somewhat imply this, but we
could
> state more explicitly that suspending and resuming is supposed to be
> significantly faster than stopping and starting.

Done.

> Or is it legal for
> implementations to internally use stop and start as long as that is "fast
> enough"?

Generally, no, we always assume Start() and Stop() are not fast (i.e., could
take on the order of seconds to execute!).

[miu, 2016-09-28 00:12:54]

miu@chromium.org changed reviewers:
+ braveyao@chromium.org

[miu, 2016-09-28 00:12:55]

braveyao: PTAL at changes to
content/browser/renderer_host/media/video_capture_manager.cc I've removed the
overrides treating screen capture devices specially. The page *receiving* the
video frames is the one that would cause capture to be suspended (which is often
*not* the page being captured).

[chfremer, 2016-09-28 00:27:37]

lgtm 

Thanks for adding the extra verifications to the test case.

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/vide...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/2365223002/diff/1/content/renderer/media/vide...
content/renderer/media/video_capture_impl_manager.h:98: void SuspendDevices(bool
suspend);
On 2016/09/27 23:42:23, miu wrote:
> On 2016/09/27 00:54:59, chfremer wrote:
> > Furthermore it seems unrelated to the responsibility this
> > class has had so far, which is to communicate with the Browser process
through
> > IPC.
> 
> No, that's not the scoping/responsibility of this class. It doesn't make nor
> process any IPC calls. Its purpose is to control an individual
VideoCaptureImpl
> and its lifetime based on multiple clients needing shared access to a single
> instance.
> 
> There's also the threading concern: This is the main thread's interface to the
> VideoCaptureImpls that run on the IO thread.
> 
> Granted, there's certainly room for clean-up here. These classes have evolved
a
> lot, and the comments, naming, and code factoring need a lot of improvement.

Oops, I have misunderstood the responsibilities of this class then.
I feel somewhat mislead by the class description stating: 

// This class provides access to a video capture device in the browser
// process through IPC. The main function is to deliver video frames
// to a client.

Could we maybe update the class description to better convey the actual
responsibilities? Also, on second look, this class seems to manage a whole map
of VideoCaptureImpl instances, not just an individual one.
 
> > Adding this responsibility to this existing class makes this class quite
large
> > and mixes responsibilities. Can we split the suspend/resume functionality
out
> > into a new class and test it in isolation?
> 
> For reasons stated above, I believe mitigating the global versus individual
> suspend/resume should be a responsibility of this class. It would
> over-complexify things to split-out the functionality into a separate class,
> because now you'd have two classes that have to synchronize with each other,
and
> with each maintaining similar state. If you disagree, feel free to show me the
> code. ;-)

:-) Challenge accepted. Please go ahead with your proposed change as is. When I
find some quiet minutes, I'd love to make an attempt at separating the
responsibilities as I see them and then see what complexities I end up with.

[braveyao, 2016-09-28 00:42:16]

lgtm to content/browser/renderer_host/media/video_capture_manager.cc.
Scree capture on Android is not managed by controller. So the modification is
fine.
BTW: As to camera capture on Android, we purposely don't to stop/start it at tab
switching, since it may cause HW problem on some devices.

[emircan, 2016-09-28 17:16:24]

content/renderer/media/media_stream* lgtm % nit.

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:405: if (it ==
suspended_tracks_.end())
std::set would be a more appropriate container for |suspended_tracks_| as we
only have unique elements.

[miu, 2016-09-28 20:57:50]

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:405: if (it ==
suspended_tracks_.end())
On 2016/09/28 17:16:24, emircan wrote:
> std::set would be a more appropriate container for |suspended_tracks_| as we
> only have unique elements.

Actually, no, because std::set is an ordered set. There's no requirement for
ordering here. So, that would suggest std::unordered_set, except...

There's been some interesting discussion on the Project TRIM list around
techniques for saving memory (and also increasing performance) w.r.t. our data
structures in Chromium. std::set/map are implemented using a Red-Black tree,
which is very memory inefficient. (I myself have a research project on the side
to determine whether we should replace our STL implementations of std::set/map
and friends with something else.)

Generally speaking, you should always try to use std::vector when the number of
elements in a container is guaranteed to remain small. The memory overhead is
far smaller, it's much friendlier to the CPU caches, and the linear search
(std::find) will execute much faster than the log-n search (set::find). A more
complete explanation: http://lafstern.org/matt/col1.pdf

If you're curious, I can point you at other research and discussion.

[miu, 2016-09-28 20:59:46]

miu@chromium.org changed reviewers:
+ xhwang@chromium.org

[miu, 2016-09-28 20:59:46]

mcasas: Need OWNERS approval for content/browser/renderer_host/media/* and
media/capture/video/video_capture_device.h.

xhwang: Need OWNERS approval for media/base/video_capturer_source.h.

[xhwang, 2016-09-28 21:09:56]

I am not familiar with this class. Just some comments from API's perspective.

https://codereview.chromium.org/2365223002/diff/20001/media/base/video_captur...
File media/base/video_capturer_source.h (right):

https://codereview.chromium.org/2365223002/diff/20001/media/base/video_captur...
media/base/video_capturer_source.h:92: virtual void MaybeSuspend() {}
nit: How will the caller know whether it can/should call Resume() if it doesn't
know whether "suspend" request is honored?

https://codereview.chromium.org/2365223002/diff/20001/media/base/video_captur...
media/base/video_capturer_source.h:94: // Resumes frame delivery, if it was
suspended.
nit: What if "it was not suspended"? Will it fail, no-op, or DCHECK? What if
capturing never "started"?

[mcasas, 2016-09-28 21:34:45]

lgtm % a few comments/ suggestions below

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host.cc:243: if (it->second) {
prefer early return?
if (!it->second)
  return;
// ...

here an in l.266

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:744: if
(had_active_client && !controller->HasActiveClient()) {
Early return? 

if (!had_active_client || controller->HasActiveClient())
  return;
// else...

Also in l.774

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:745: if
(media::VideoCaptureDevice* device = entry->video_capture_device()) {
nit: I thought assignment-and-testing were discouraged
in Chromium, but can't find any reference to that in the
style guide (either Cr or G).  I'm fine either way but wdyt?

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:189: const
int32_t kNumberOfFakeDevices = 2;
nit: totally unrelated to this CL, but it seems like
many of the member variables don't need to be
held unique_ptr<>, could be const and initialized
in the ctor. Also |kNumberOfFakeDvices| could be
a constexpr. I might clean this up sometime in the
future.

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:573: const
int video_session_id = vcm_->Open(devices_.front());
Yay!

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:335: DVLOG(3) <<
__func__;
nit: remove DVLOG(3)s now.

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:358:
base::ResetAndReturn(&running_callback_).Run(true);
nit: Per recent conversation in chromium-dev (pkasting@),
this idiom should be reserved to callbacks that
can modify the state of (destroy?) the caller. In this
case, I believe it could be
  running_callback_.Run(true);
  running_callback_.Reset();

same in l365 with |false| argument.

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:198: // frames delivered from
the source.
nit: I'd add that |suspended_tracks_| is a subset of |tracks_|,
otherwise a developer might doubt if tracks are moved
between those two vectors or the can (partially) overlap.

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_track.cc:255: if (source_) {
Heh I feel like a scratched record, but, early return?
Here and in l.271.

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

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:64: if (entry.impl) {
This should never happen, right? The only
way |entry.impl| is nullptr is via l.69. If so,
remove this if.

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:67:
base::Unretained(entry.impl.get())));
Surprised to see this specific DeInit(): I would say
VideoCaptureImpl::DeInit() should be called from
~VideoCaptureImpl(), right? Otherwise, if an object
of that class is forcibly deleted, it would not send
the stop-streaming msg to browser, and capture
would go on... forever? (uses of deinit-delete are
all in this class -- maybe I can follow up and clean
this a la StopAndDeallocate() ).

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:234: if (it->impl) {
Early return?

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

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:110: struct DeviceEntry {
Can we forward declare this struct and define it
in video_capture_impl_manager.cc ?

https://codereview.chromium.org/2365223002/diff/20001/media/capture/video/vid...
File media/capture/video/video_capture_device.h (right):

https://codereview.chromium.org/2365223002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_device.h:174: virtual void MaybeSuspend() {}
Could we say here MaybePause()? It would sound
to  me as the complementary operation of 
Resume(), and sort-of like a VCR recorder. But
I'm no Merriam nor Webster so I'll agree to your view.

[miu, 2016-09-28 21:52:14]

xhwang: PTAL (augmented comments per your suggestions in PS3).

https://codereview.chromium.org/2365223002/diff/20001/media/base/video_captur...
File media/base/video_capturer_source.h (right):

https://codereview.chromium.org/2365223002/diff/20001/media/base/video_captur...
media/base/video_capturer_source.h:92: virtual void MaybeSuspend() {}
On 2016/09/28 21:09:56, xhwang (slow) wrote:
> nit: How will the caller know whether it can/should call Resume() if it
doesn't
> know whether "suspend" request is honored?

Added explanation in comments.

https://codereview.chromium.org/2365223002/diff/20001/media/base/video_captur...
media/base/video_capturer_source.h:94: // Resumes frame delivery, if it was
suspended.
On 2016/09/28 21:09:56, xhwang (slow) wrote:
> nit: What if "it was not suspended"? Will it fail, no-op, or DCHECK? What if
> capturing never "started"?

Added explanation in comments for this too.

[miu, 2016-09-28 22:35:09]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[miu, 2016-09-28 22:35:16]

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host.cc:243: if (it->second) {
On 2016/09/28 21:34:43, mcasas wrote:
> prefer early return?
> if (!it->second)
>   return;
> // ...
> 
> here an in l.266

Done.

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:744: if
(had_active_client && !controller->HasActiveClient()) {
On 2016/09/28 21:34:44, mcasas wrote:
> Early return? 
> 
> if (!had_active_client || controller->HasActiveClient())
>   return;
> // else...
> 
> Also in l.774

Done.

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:745: if
(media::VideoCaptureDevice* device = entry->video_capture_device()) {
On 2016/09/28 21:34:44, mcasas wrote:
> nit: I thought assignment-and-testing were discouraged
> in Chromium, but can't find any reference to that in the
> style guide (either Cr or G).  I'm fine either way but wdyt?

I've used it all over the place. Never had anyone say it was a bad idea before.
;)

I can understand the confusion between good 'ol == vs = logic errors, but the
added type declaration mitigates that. Compare this:

 if (device = entry->video_capture_device())

to:

 if (media::VideoCaptureDevice* device = entry->video_capture_device())

The first form raises red flags, and looks very much like a logic error. That's
because |device| is initialized somewhere else and could be used after this
statement. The second form has the type declaration, so you can clearly see the
intention is initialization (not comparison), and the scoping of the variable is
restricted to the body of the if-statement.

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/2365223002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:189: const
int32_t kNumberOfFakeDevices = 2;
On 2016/09/28 21:34:44, mcasas wrote:
> nit: totally unrelated to this CL, but it seems like
> many of the member variables don't need to be
> held unique_ptr<>, could be const and initialized
> in the ctor. Also |kNumberOfFakeDvices| could be
> a constexpr. I might clean this up sometime in the
> future.

Agree! Let's encourage others to stop thinking like Java programmers when
writing C++ code. :) Composing objects together can eliminate bugs (by allowing
for more compile-time checking of code structure), and because of data locality
there is a runtime+memory performance advantage.

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:335: DVLOG(3) <<
__func__;
On 2016/09/28 21:34:44, mcasas wrote:
> nit: remove DVLOG(3)s now.

They're actually very useful when debugging (they helped me to confirm things
were working as expected in a full browser). The other methods have them too, so
it's in-line with the style of the code in this file. These are eliminated in
release builds.

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:358:
base::ResetAndReturn(&running_callback_).Run(true);
On 2016/09/28 21:34:44, mcasas wrote:
> nit: Per recent conversation in chromium-dev (pkasting@),
> this idiom should be reserved to callbacks that
> can modify the state of (destroy?) the caller. In this
> case, I believe it could be
>   running_callback_.Run(true);
>   running_callback_.Reset();
> 
> same in l365 with |false| argument.

Wait. I thought that e-mail thread ended with pkasting saying:

  "* Use ResetAndReturn() liberally.
   * Prefer OnceCallback if you know you want a callback that should only ever
be run once."

However, OnceCallback still isn't available for use (AFAIK, it's WIP and coming
soon).

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:198: // frames delivered from
the source.
On 2016/09/28 21:34:44, mcasas wrote:
> nit: I'd add that |suspended_tracks_| is a subset of |tracks_|,
> otherwise a developer might doubt if tracks are moved
> between those two vectors or the can (partially) overlap.

Done.

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_track.cc:255: if (source_) {
On 2016/09/28 21:34:44, mcasas wrote:
> Heh I feel like a scratched record, but, early return?
> Here and in l.271.

Done.

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

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:64: if (entry.impl) {
On 2016/09/28 21:34:44, mcasas wrote:
> This should never happen, right? The only
> way |entry.impl| is nullptr is via l.69. If so,
> remove this if.

Done, and removed all null checks elsewhere since the DeviceEntry::impl should
never be null while it exists.

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:67:
base::Unretained(entry.impl.get())));
On 2016/09/28 21:34:44, mcasas wrote:
> Surprised to see this specific DeInit(): I would say
> VideoCaptureImpl::DeInit() should be called from
> ~VideoCaptureImpl(), right? Otherwise, if an object
> of that class is forcibly deleted, it would not send
> the stop-streaming msg to browser, and capture
> would go on... forever? (uses of deinit-delete are
> all in this class -- maybe I can follow up and clean
> this a la StopAndDeallocate() ).

Yes, this is a bit weird. Though, it could be solving a shutdown issue: Note
that, if the IO thread is destroyed, the DeInit() task will not run, but the
DeleteSoon() will still delete the VideoCaptureImpl (just not on the IO thread).
That could be why they were explicitly separated here.

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.cc:234: if (it->impl) {
On 2016/09/28 21:34:44, mcasas wrote:
> Early return?

Done.

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

https://codereview.chromium.org/2365223002/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl_manager.h:110: struct DeviceEntry {
On 2016/09/28 21:34:44, mcasas wrote:
> Can we forward declare this struct and define it
> in video_capture_impl_manager.cc ?

Done.

[commit-bot: I haz the power, 2016-09-28 22:35:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2016-09-28 22:44:57]

thanks! media/base/video_capturer_source.h lgtm

[commit-bot: I haz the power, 2016-09-29 00:39:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 00:39:13]

Dry run: This issue passed the CQ dry run.

[miu, 2016-09-29 01:08:14]

The CQ bit was checked by miu@chromium.org

[miu, 2016-09-29 01:08:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
braveyao@chromium.org, emircan@chromium.org, mcasas@chromium.org,
chfremer@chromium.org
Link to the patchset: https://codereview.chromium.org/2365223002/#ps60001
(title: "Style tweaks, per mcasas's comments.")

[commit-bot: I haz the power, 2016-09-29 01:08:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 01:18:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-29 01:20:47]

Description was changed from

==========
Video Capture: Allow suspension of individual devices.

Adds infrastructure to notify started video capture devices when there
are no consumers present, so that they may turn down to an "idle," low
resource utilization state. There are legitimate use cases where
MediaStream sessions are purposely left open without any sinks attached
for significant periods of time, and then attached when the application
is ready to consume the video.

This is a prerequisite to a soon-upcoming change that will allow screen
capture devices, which are very resource intensive, to suspend.

BUG=650113
==========

to

==========
Video Capture: Allow suspension of individual devices.

Adds infrastructure to notify started video capture devices when there
are no consumers present, so that they may turn down to an "idle," low
resource utilization state. There are legitimate use cases where
MediaStream sessions are purposely left open without any sinks attached
for significant periods of time, and then attached when the application
is ready to consume the video.

This is a prerequisite to a soon-upcoming change that will allow screen
capture devices, which are very resource intensive, to suspend.

BUG=650113

Committed: https://crrev.com/1c98a2e10b2505761d1f88880e3c8267951d30e2
Cr-Commit-Position: refs/heads/master@{#421705}
==========

[commit-bot: I haz the power, 2016-09-29 01:20:48]

Patchset 4 (id:??) landed as
https://crrev.com/1c98a2e10b2505761d1f88880e3c8267951d30e2
Cr-Commit-Position: refs/heads/master@{#421705}