Release the camera when WebRTC is not visible in Android

Currently when WebRTC isn't visible (app is in background or tab is not visible) in chrome Android, we stop sending captured video frame, but still hold the camera, this makes camera can not be used by other apps.

This patch releases the camera on pause, and allocate it again when resume, since SuspendDevices() is only called in Android, this is Android only behavior.

BUG=417659
Committed: https://crrev.com/0817b18c54feec8f6c484fa787c3ace56b107ab7
Cr-Commit-Position: refs/heads/master@{#297933}

[michaelbai, 2014-09-30 22:40:13]

michaelbai@chromium.org changed reviewers:
+ tommi@chromium.org

[tommi (sloooow) - chröme, 2014-10-01 07:17:03]

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:287: return;
should the event handler be notified of this somehow?

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:715: for
(ControllerClient* client : controller_clients_) {
oooh...! :)

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:717:
active_client_count++;
++active_client_count

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:99: // Pause or
resume the video capture for specified client.
Can you document the side effects of this?
(assuming my understanding is correct) If we pause, we don't hold on to the
camera anymore, so resuming might fail and might also take much longer than
pause/play usually does (which I'm guessing is the reason you're calling it
'resume' which I think is good).

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:107: int
GetActiveClientCount();
const?

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:306:
media_stream_manager_->video_capture_manager()->PauseCaptureForClient(
fix indent

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:310: // Send
PAUSED_STATE?
is this something for the next patch set?

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:326:
media_stream_manager_->video_capture_manager()->ResumeCaptureForClient(
indent

https://codereview.chromium.org/615043006/diff/1/content/renderer/media/video...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/615043006/diff/1/content/renderer/media/video...
content/renderer/media/video_capture_impl.cc:76: if (suspend)
nit:

Send(suspend ?
    new VideoCaptureHostMsg_Pause() :
    new VideoCaptureHostMsg_Resume());

?

[michaelbai, 2014-10-01 22:09:33]

PTAL

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:287: return;
On 2014/10/01 07:17:03, tommi wrote:
> should the event handler be notified of this somehow?

That's what I wondered in the first place, I think it depends on how we want to
handle pause, it seems VideoCaptureControllerEventHandler has nothing to do even
it knows video was paused.

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:715: for
(ControllerClient* client : controller_clients_) {
On 2014/10/01 07:17:03, tommi wrote:
> oooh...! :)

I am happy that how simple it is :)

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:717:
active_client_count++;
On 2014/10/01 07:17:02, tommi wrote:
> ++active_client_count

Done.

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:99: // Pause or
resume the video capture for specified client.
Thanks, I added the side effect in VideoCaptureManager::ResumeCaptureForClient()
because the camera is allocated there.

There is another issue here, if we can't allocate the camera, the error state
will be send out, the video capture can not be resumed automatically any more,
because all information was cleaned up. I were thinking developer could handle
this case by hooking into page visibility API and tried to recreate the video
channel, sounds good?

If we want video capture still resume-able, we need a way to let user know the
camera is occupied by other app so the camera could be released, it seems to me
we don't have that event in JavaScript API ( I assume VIDEO_CAPTURE_STATE_ERROR
is not suitable for this). If we don't notify this to user, there are less
chance camera will be released.

Actually, this issue is not critical, I will explain to you offline.  


On 2014/10/01 07:17:03, tommi wrote:
> Can you document the side effects of this?
> (assuming my understanding is correct) If we pause, we don't hold on to the
> camera anymore, so resuming might fail and might also take much longer than
> pause/play usually does (which I'm guessing is the reason you're calling it
> 'resume' which I think is good).

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.h:107: int
GetActiveClientCount();
On 2014/10/01 07:17:03, tommi wrote:
> const?

Done.

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:306:
media_stream_manager_->video_capture_manager()->PauseCaptureForClient(
On 2014/10/01 07:17:03, tommi wrote:
> fix indent

Done.

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:310: // Send
PAUSED_STATE?
On 2014/10/01 07:17:03, tommi wrote:
> is this something for the next patch set?

I don't think this ACK message is needed if current way to pause video capture
is good.

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_host.cc:326:
media_stream_manager_->video_capture_manager()->ResumeCaptureForClient(
On 2014/10/01 07:17:03, tommi wrote:
> indent

Done.

https://codereview.chromium.org/615043006/diff/1/content/renderer/media/video...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/615043006/diff/1/content/renderer/media/video...
content/renderer/media/video_capture_impl.cc:76: if (suspend)
On 2014/10/01 07:17:03, tommi wrote:
> nit:
> 
> Send(suspend ?
>     new VideoCaptureHostMsg_Pause() :
>     new VideoCaptureHostMsg_Resume());
> 
> ?

Done.

[tommi (sloooow) - chröme, 2014-10-02 11:10:40]

lgtm

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/615043006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:287: return;
On 2014/10/01 22:09:33, michaelbai wrote:
> On 2014/10/01 07:17:03, tommi wrote:
> > should the event handler be notified of this somehow?
> 
> That's what I wondered in the first place, I think it depends on how we want
to
> handle pause, it seems VideoCaptureControllerEventHandler has nothing to do
even
> it knows video was paused.

Acknowledged.

[Mike West, 2014-10-02 11:24:39]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org, tsepez@chromium.org

[Mike West, 2014-10-02 11:24:40]

Drive-by ICP comments; CCing tsepez@ for a real review of the message changes.

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

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_host.cc:324:
media_stream_manager_->video_capture_manager()->ResumeCaptureForClient(
Perhaps you could DCHECK that the capture was indeed paused?

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

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:419:
controller->PauseOrResumeClient(client_id, client_handler, false);
Nit: The bool at the end makes this is pretty hard to read, both here and above.
Consider changing it to an enum in a future CL, 'PAUSE' or 'RESUME' would be
significantly better than 'true' and 'false'.

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:423: // This is
first active client, allocate the camera.
Since resuming capture assumes that the user has already granted permission for
the device, I'm worried that it might bypass those checks if a corrupted
renderer sent a "resume" message rather than a "start recording" message.

Where is the user asked to grant permission for device access? Can we assert
something here about the permission grant?

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.h:87: // Called by
VideoCaptureHost to pause to update video buffer specifyed by
Nit: s/specifyed/specified/

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.h:95: // Called by
VideoCaptureHost to resume to update video buffer specifyed by
Nit: s/specifyed/specified/

[Tom Sepez, 2014-10-02 17:16:26]

What Mike said: the main question is what happens if a resume comes without a
preceding start / suspend?

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:715: for
(ControllerClient* client : controller_clients_) {
We're on the verge of approving this style of for loop, but has it been fully
signed-off upon?

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.h:100: void
PauseOrResumeClient(const VideoCaptureControllerID& id,
Maybe split it into separate pause/resume methods to make the meaning clearer at
the call sites.

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.h:106: // Return
the number of client that isn't paused.
nit: // Return the number of clients that aren't paused.

[michaelbai, 2014-10-02 18:55:40]

As to the security question, in resume, device_id is used to find corresponding
information in browser like, VideoCaptureController, if there is no match entry,
it just return.

I don't understand exact security issue here, if renderer is hacked, video frame
could be read anyway.

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:715: for
(ControllerClient* client : controller_clients_) {
This is listed in allowed features in http://chromium-cpp.appspot.com/, unless
there are something new, I think it should be approved.


On 2014/10/02 17:16:25, Tom Sepez wrote:
> We're on the verge of approving this style of for loop, but has it been fully
> signed-off upon?

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.h:100: void
PauseOrResumeClient(const VideoCaptureControllerID& id,
there will have a lot of duplicated code in implementation if split it.


On 2014/10/02 17:16:26, Tom Sepez wrote:
> Maybe split it into separate pause/resume methods to make the meaning clearer
at
> the call sites.

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.h:106: // Return
the number of client that isn't paused.
On 2014/10/02 17:16:26, Tom Sepez wrote:
> nit: // Return the number of clients that aren't paused.

Done.

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

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_host.cc:324:
media_stream_manager_->video_capture_manager()->ResumeCaptureForClient(
I added it in VideoCaptureController

On 2014/10/02 11:24:39, Mike West (OOO until 6th) wrote:
> Perhaps you could DCHECK that the capture was indeed paused?

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

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:419:
controller->PauseOrResumeClient(client_id, client_handler, false);
Yes, it is a little bit hard to read, but there are a lot of this kind of usage
in chromium code.

On 2014/10/02 11:24:39, Mike West (OOO until 6th) wrote:
> Nit: The bool at the end makes this is pretty hard to read, both here and
above.
> Consider changing it to an enum in a future CL, 'PAUSE' or 'RESUME' would be
> significantly better than 'true' and 'false'.

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.h:87: // Called by
VideoCaptureHost to pause to update video buffer specifyed by
On 2014/10/02 11:24:39, Mike West (OOO until 6th) wrote:
> Nit: s/specifyed/specified/

Done.

https://codereview.chromium.org/615043006/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.h:95: // Called by
VideoCaptureHost to resume to update video buffer specifyed by
On 2014/10/02 11:24:39, Mike West (OOO until 6th) wrote:
> Nit: s/specifyed/specified/

Done.

[Tom Sepez, 2014-10-02 19:03:46]

On 2014/10/02 18:55:40, michaelbai wrote:
> As to the security question, in resume, device_id is used to find
corresponding
> information in browser like, VideoCaptureController, if there is no match
entry,
> it just return.
> 
Ok.  That should work.

> This is listed in allowed features in http://chromium-cpp.appspot.com/, unless
> there are something new, I think it should be approved.
Ok.


> content/browser/renderer_host/media/video_capture_controller.h:100: void
> PauseOrResumeClient(const VideoCaptureControllerID& id,
> there will have a lot of duplicated code in implementation if split it.
>
I don't follow. You could move the duplicated code to some common routine? 
Maybe as a follow up.

> content/browser/renderer_host/media/video_capture_manager.cc:419:
> controller->PauseOrResumeClient(client_id, client_handler, false);
> Yes, it is a little bit hard to read, but there are a lot of this kind of
usage
> in chromium code.
What Mike is suggesting is a Blink style rule, and not a Chromium one. Still, I
think the code would be clearest if you took the effort to split these into
separate methods, avoiding the need for a bool or enum here.  Maybe as a
follow-up.
 

Messages themselves are LGTM.

[michaelbai, 2014-10-02 21:27:38]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-10-02 21:29:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615043006/60001

[commit-bot: I haz the power, 2014-10-02 22:43:29]

Committed patchset #4 (id:60001) as 48d1c8ce538626f92f4dad4489e655a28ca4edbe

[commit-bot: I haz the power, 2014-10-02 22:44:14]

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