Android: move peerconnection out of power observer.

Android: move peerconnection out of power observer.

Currently when screen is off (or Chrome goes to background) for more than 1 min,
peerconnection will be stoppted by PowerMonitor to save battery power. This is
not necesary now since we'll suspend video call when the tab is background. And
users prefer that audio call can continue while they are doing something else.

BUG=513633
Committed: https://crrev.com/22c91cc29993f5f7a9f1744349b9b2065ab266bd
Cr-Commit-Position: refs/heads/master@{#415372}

[braveyao, 2016-08-22 21:31:16]

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

[commit-bot: I haz the power, 2016-08-22 21:32:48]

Dry run: 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-08-22 22:43:51]

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

[commit-bot: I haz the power, 2016-08-22 22:43:52]

Dry run: This issue passed the CQ dry run.

[braveyao, 2016-08-22 23:36:12]

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

[braveyao, 2016-08-22 23:36:12]

Hi Tommi, do you have any idea why we add peerconnection into PowerObserver on
Android? Seems no one knows here :(
Given that video call will be suspended when chrome/tab goes background and
people like to keep audio call at same time, I try to move peerconnection out of
PowerObserver here.
Please help to have a look.

[braveyao, 2016-08-23 00:19:08]

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

[braveyao, 2016-08-23 00:19:31]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-08-23 00:20:16]

Dry run: 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-08-23 02:32:54]

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

[commit-bot: I haz the power, 2016-08-23 02:32:54]

Dry run: This issue passed the CQ dry run.

[braveyao, 2016-08-23 16:18:49]

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

[braveyao, 2016-08-23 16:18:50]

Hi Per, tommi is OOO. Could you please take a look at this?

[perkj_chrome, 2016-08-23 18:27:53]

On 2016/08/23 16:18:50, braveyao wrote:
> Hi Per, tommi is OOO. Could you please take a look at this?

To my understanding, this will also disable the power monitor for desktop
platforms. Something I dont' think we want to do. 
I am not sure what the correct thing to do on Android is though.  I guess that
sooner or later we should do the same on Android- regardless of if it is an
audio call or not.  I don't think a call should go on forever until there is no
battery left.

[braveyao, 2016-08-23 18:48:32]

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

[commit-bot: I haz the power, 2016-08-23 18:49:13]

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

[braveyao, 2016-08-23 18:53:56]

Sorry, I took for granted that power monitor is only valid on mobile devices.
Revised it to exclude Android only.

For a normal telephone call, it will go on forever until here is no battery
left. So this shouldn't be a problem. Please re-consider.

[perkj_chrome, 2016-08-23 19:11:02]

Description was changed from

==========
Android: move peerconnection out of power observer.

Currently when screen is off (or Chrome goes to background) for more than 1 min,
peerconnection will be stoppted by PowerMonitor to save battery power. This is
not necesary now since we'll suspend video call when the tab is background. And
users prefer that audio call can continue while they are doing something else.

BUG=513633
==========

to

==========
Android: move peerconnection out of power observer.

Currently when screen is off (or Chrome goes to background) for more than 1 min,
peerconnection will be stoppted by PowerMonitor to save battery power. This is
not necesary now since we'll suspend video call when the tab is background. And
users prefer that audio call can continue while they are doing something else.

BUG=513633
==========

[perkj_chrome, 2016-08-23 19:11:02]

perkj@chromium.org changed reviewers:
+ hta@chromium.org

[perkj_chrome, 2016-08-23 19:19:28]

On 2016/08/23 18:53:56, braveyao wrote:
> Sorry, I took for granted that power monitor is only valid on mobile devices.
> Revised it to exclude Android only.
> 
> For a normal telephone call, it will go on forever until here is no battery
> left. So this shouldn't be a problem. Please re-consider.

Adding, hta as well. I can not approve this without some more discussion. 

This is not only audio and video. If you visit an arbitrary webpage, it may use
data channels without user consent. So, doesn't it make sense then to teardown
the call sooner than out of battery? 
Will a running PeerConnection within Chrome prevent the device for going to
sleep if it is still alive or will the connection be broken anyway after a
while?

[commit-bot: I haz the power, 2016-08-23 20:04:08]

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

[commit-bot: I haz the power, 2016-08-23 20:04:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[braveyao, 2016-08-23 21:14:55]

Thanks Per. Good point on datachannel, which is a tricky one.

With audio/video call, user normally is deeply involved, people
watching/speaking/listening during a call, and user may also turn screen off to
save battery with audio-only. So it's reasonable to keep peer connection alive.
Duo does same thing too.
As to datachannel, it's true that user may not be aware of it. So it's good to
cut the peerconnection after Chrome goes to background. But on the other hand,
it's also a valid case that it takes quite long time to transfer a file with
datachannel, during which the screen might go off. So is it really user-friendly
to cut the transferring in the middle?

Any better idea?

[hta - Chromium, 2016-08-23 22:06:25]

On 2016/08/23 21:14:55, braveyao wrote:
> Thanks Per. Good point on datachannel, which is a tricky one.
> 
> With audio/video call, user normally is deeply involved, people
> watching/speaking/listening during a call, and user may also turn screen off
to
> save battery with audio-only. So it's reasonable to keep peer connection
alive.
> Duo does same thing too.
> As to datachannel, it's true that user may not be aware of it. So it's good to
> cut the peerconnection after Chrome goes to background. But on the other hand,
> it's also a valid case that it takes quite long time to transfer a file with
> datachannel, during which the screen might go off. So is it really
user-friendly
> to cut the transferring in the middle?
> 
> Any better idea?

I must admit to drawing a blank here. What's a PowerMonitor, and what effect
does attaching to it have?

I take it there are conditions under which being attached to the PowerMonitor
will cause something to stop. But when does this happen on desktop and/or
Android, and what effect will this have on the PeerConnection?

Agree that cutting communication is a Bad Thing.

My instinct is to disconnect from the whole thing on all platforms until I
understand why someone thinks it's a good idea. But I guess it's better to ask
first.

[Niklas Enbom, 2016-08-23 22:37:49]

I'm also torn on this. Harald, this is how it works now: 

If the WebRTC tab is not in the foreground the peerconnection is killed after
one minute.
- This clearly doesn't make sense for an audio call
- For a video call we already mute the video when the tab is put in the
background - so this doesn't make sense for a video call either
- Does this make sense for a data channel? A malicious site could eat up your
data and battery - but you might also be doing a file transfer or chat session
that you want to keep alive.

The easiest solution is to remove the restriction for all cases, like this CL
does.


On 2016/08/23 22:06:25, hta - Chromium wrote:
> On 2016/08/23 21:14:55, braveyao wrote:
> > Thanks Per. Good point on datachannel, which is a tricky one.
> > 
> > With audio/video call, user normally is deeply involved, people
> > watching/speaking/listening during a call, and user may also turn screen off
> to
> > save battery with audio-only. So it's reasonable to keep peer connection
> alive.
> > Duo does same thing too.
> > As to datachannel, it's true that user may not be aware of it. So it's good
to
> > cut the peerconnection after Chrome goes to background. But on the other
hand,
> > it's also a valid case that it takes quite long time to transfer a file with
> > datachannel, during which the screen might go off. So is it really
> user-friendly
> > to cut the transferring in the middle?
> > 
> > Any better idea?
> 
> I must admit to drawing a blank here. What's a PowerMonitor, and what effect
> does attaching to it have?
> 
> I take it there are conditions under which being attached to the PowerMonitor
> will cause something to stop. But when does this happen on desktop and/or
> Android, and what effect will this have on the PeerConnection?
> 
> Agree that cutting communication is a Bad Thing.
> 
> My instinct is to disconnect from the whole thing on all platforms until I
> understand why someone thinks it's a good idea. But I guess it's better to ask
> first.

[braveyao, 2016-08-24 18:22:44]

On 2016/08/23 22:37:49, Niklas Enbom wrote:
> I'm also torn on this. Harald, this is how it works now: 
> 
> If the WebRTC tab is not in the foreground the peerconnection is killed after
> one minute.
> - This clearly doesn't make sense for an audio call
> - For a video call we already mute the video when the tab is put in the
> background - so this doesn't make sense for a video call either
> - Does this make sense for a data channel? A malicious site could eat up your
> data and battery - but you might also be doing a file transfer or chat session
> that you want to keep alive.
> 
> The easiest solution is to remove the restriction for all cases, like this CL
> does.
> 
> 
> On 2016/08/23 22:06:25, hta - Chromium wrote:
> > On 2016/08/23 21:14:55, braveyao wrote:
> > > Thanks Per. Good point on datachannel, which is a tricky one.
> > > 
> > > With audio/video call, user normally is deeply involved, people
> > > watching/speaking/listening during a call, and user may also turn screen
off
> > to
> > > save battery with audio-only. So it's reasonable to keep peer connection
> > alive.
> > > Duo does same thing too.
> > > As to datachannel, it's true that user may not be aware of it. So it's
good
> to
> > > cut the peerconnection after Chrome goes to background. But on the other
> hand,
> > > it's also a valid case that it takes quite long time to transfer a file
with
> > > datachannel, during which the screen might go off. So is it really
> > user-friendly
> > > to cut the transferring in the middle?
> > > 
> > > Any better idea?
> > 
> > I must admit to drawing a blank here. What's a PowerMonitor, and what effect
> > does attaching to it have?
> > 
> > I take it there are conditions under which being attached to the
PowerMonitor
> > will cause something to stop. But when does this happen on desktop and/or
> > Android, and what effect will this have on the PeerConnection?
> > 
> > Agree that cutting communication is a Bad Thing.
> > 
> > My instinct is to disconnect from the whole thing on all platforms until I
> > understand why someone thinks it's a good idea. But I guess it's better to
ask
> > first.

Hi hta@, 

PowerMonitor is used to monitor the power state change and notify the observers
about the change event. On Android, putting Chrome into background(e.g.
switching to
HomeScreen or other Apps) and powering screen off will effect the power monitor
to start
the 1 min timer. Then when Chrome is notified about the power state change,
chrome will
stop the peerconnections. As to desktop, putting device into Sleep should
trigger the same
thing.

On Android, it's quite a comment case that user will switch to other Apps or
power off screen
to save some battery during a webrtc call session, which is totally unlike the
situation on
desktops. So we shouldn't cut peerconnection due to this.

[hta - Chromium, 2016-08-25 09:46:59]

On 2016/08/24 18:22:44, braveyao wrote:
> On 2016/08/23 22:37:49, Niklas Enbom wrote:
> > I'm also torn on this. Harald, this is how it works now: 
> > 
> > If the WebRTC tab is not in the foreground the peerconnection is killed
after
> > one minute.
> > - This clearly doesn't make sense for an audio call
> > - For a video call we already mute the video when the tab is put in the
> > background - so this doesn't make sense for a video call either
> > - Does this make sense for a data channel? A malicious site could eat up
your
> > data and battery - but you might also be doing a file transfer or chat
session
> > that you want to keep alive.
> > 
> > The easiest solution is to remove the restriction for all cases, like this
CL
> > does.
> > 
> > 
> > On 2016/08/23 22:06:25, hta - Chromium wrote:
> > > On 2016/08/23 21:14:55, braveyao wrote:
> > > > Thanks Per. Good point on datachannel, which is a tricky one.
> > > > 
> > > > With audio/video call, user normally is deeply involved, people
> > > > watching/speaking/listening during a call, and user may also turn screen
> off
> > > to
> > > > save battery with audio-only. So it's reasonable to keep peer connection
> > > alive.
> > > > Duo does same thing too.
> > > > As to datachannel, it's true that user may not be aware of it. So it's
> good
> > to
> > > > cut the peerconnection after Chrome goes to background. But on the other
> > hand,
> > > > it's also a valid case that it takes quite long time to transfer a file
> with
> > > > datachannel, during which the screen might go off. So is it really
> > > user-friendly
> > > > to cut the transferring in the middle?
> > > > 
> > > > Any better idea?
> > > 
> > > I must admit to drawing a blank here. What's a PowerMonitor, and what
effect
> > > does attaching to it have?
> > > 
> > > I take it there are conditions under which being attached to the
> PowerMonitor
> > > will cause something to stop. But when does this happen on desktop and/or
> > > Android, and what effect will this have on the PeerConnection?
> > > 
> > > Agree that cutting communication is a Bad Thing.
> > > 
> > > My instinct is to disconnect from the whole thing on all platforms until I
> > > understand why someone thinks it's a good idea. But I guess it's better to
> ask
> > > first.
> 
> Hi hta@, 
> 
> PowerMonitor is used to monitor the power state change and notify the
observers
> about the change event. On Android, putting Chrome into background(e.g.
> switching to
> HomeScreen or other Apps) and powering screen off will effect the power
monitor
> to start
> the 1 min timer. Then when Chrome is notified about the power state change,
> chrome will
> stop the peerconnections. As to desktop, putting device into Sleep should
> trigger the same
> thing.
> 
> On Android, it's quite a comment case that user will switch to other Apps or
> power off screen
> to save some battery during a webrtc call session, which is totally unlike the
> situation on
> desktops. So we shouldn't cut peerconnection due to this.

I still don't get it.

The PowerObserver has 3 callbacks: OnPowerStateChange, OnSuspend and OnResume.
Defaults are empty.

The only callback instantiated in PeerConnectionTrackerHost is OnSuspend.

This calls CloseClientPeerConnection() on all peerconnections tracked by
PeerConnectionTracker.

This is logical on all cases where "suspend" actually means that the system is
going to suspend.

Are you saying that when Chrome goes into background on Android, it will get the
OnSuspend callback, but it's still around and able to process media?

That seems like a really weird definition of "suspend" for the Android platform.

I'd think that a better place to put the #ifdef would be around the killing
action in PeerConnectionTracker::OnSuspend - that links the #ifdef much more
closely to the actual action that we want to avoid.

What's the unittest for this change?

[braveyao, 2016-08-25 17:55:20]

Patchset #3 (id:60001) has been deleted

[braveyao, 2016-08-25 17:55:39]

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

[commit-bot: I haz the power, 2016-08-25 17:56:43]

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

[braveyao, 2016-08-25 19:02:18]

On desktop I believe Suspend means more of a "system" level hard suspend. On
Android it is indicative more of the activity state. So this is kinda
reasonable.

Move the #ifdef to PeerConnectionTracker::OnSuspend. PTAL!

Can't think of a good idea for the unittest of this change in existing
unittests. Any suggestion?
(PS: I'm about to learn how to build a unittest in chromium. Then I'll revisit
this later. OK?)

[commit-bot: I haz the power, 2016-08-25 19:06:36]

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

[commit-bot: I haz the power, 2016-08-25 19:06:37]

Dry run: This issue passed the CQ dry run.

[hta - Chromium, 2016-08-25 20:48:25]

lgtm

for unittest: mock the peerconnection, let the unittest call
PeerConnectionTrackerHost::OnSuspend, and verify that
peerconnection.CloseClientPeerConnection() gets called on desktop, but does not
get called on android.

(I'd actually have moved the #ifdef even further in, and made it surround the
action in PeerConnectionTracker::OnSuspend, peer_connection_tracker.cc - but
this #ifdef is closer to where the action is than the previous one was.)

[perkj_chrome, 2016-08-26 08:27:15]

On 2016/08/25 20:48:25, hta - Chromium wrote:
> lgtm
> 
> for unittest: mock the peerconnection, let the unittest call
> PeerConnectionTrackerHost::OnSuspend, and verify that
> peerconnection.CloseClientPeerConnection() gets called on desktop, but does
not
> get called on android.
> 
> (I'd actually have moved the #ifdef even further in, and made it surround the
> action in PeerConnectionTracker::OnSuspend, peer_connection_tracker.cc - but
> this #ifdef is closer to where the action is than the previous one was.)

If we are to do this I think hta is right- Keep this code general in the browser
process for all platforms and only skip the action on Android in
PeerConnectionTracker::OnSuspend in the render process- (sorry should have
suggested that right away) . That way we can still get notified and take another
action if we want. Also easier to write a unit test for this change.

[braveyao, 2016-08-26 16:54:07]

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

[commit-bot: I haz the power, 2016-08-26 16:54:32]

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

[braveyao, 2016-08-26 18:25:34]

Patchset #5 (id:120001) has been deleted

[braveyao, 2016-08-26 18:36:19]

Got your point! Moved the check to PeerConnectionTracker. PTAL.

And I still have some problems to add a unittest in
peer_connection_tracker_unittest.cc. How to send a
PeerConnectionTracker_OnSuspend() message to PeerConnectionTracker? 
MockSendTargetThread has a Send() method, which practically won't send the
message out...

[braveyao, 2016-08-26 23:35:55]

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

[commit-bot: I haz the power, 2016-08-26 23:36:46]

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

[braveyao, 2016-08-27 00:21:09]

Never mind my previous question. I managed a unittest in latest patchset. PTAL.

[commit-bot: I haz the power, 2016-08-27 01:38:33]

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

[commit-bot: I haz the power, 2016-08-27 01:38:34]

Dry run: This issue passed the CQ dry run.

[perkj_chrome, 2016-08-29 06:10:18]

lgtm

[tommi (sloooow) - chröme, 2016-08-29 13:30:30]

Please add a comment that explains why we're handling the suspend notification
differently on Android.  With that, lgtm.

https://codereview.chromium.org/2263373003/diff/140001/content/renderer/media...
File content/renderer/media/rtc_peer_connection_handler.h (right):

https://codereview.chromium.org/2263373003/diff/140001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler.h:168: virtual void
CloseClientPeerConnection();
nit: add a comment that the methods is virtual for testing purposes.

[hta - Chromium, 2016-08-29 15:07:13]

lgtm

still looks OK to me, but indent comment.

https://codereview.chromium.org/2263373003/diff/140001/content/renderer/media...
File content/renderer/media/peer_connection_tracker_unittest.cc (right):

https://codereview.chromium.org/2263373003/diff/140001/content/renderer/media...
content/renderer/media/peer_connection_tracker_unittest.cc:97:
std::unique_ptr<IPC::Message> message(
These 3 lines are indented oddly. Time for git cl format?

[braveyao, 2016-08-29 16:38:04]

The CQ bit was checked by braveyao@chromium.org

[braveyao, 2016-08-29 16:38:04]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org,
hta@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/2263373003/#ps160001
(title: "address comments")

[commit-bot: I haz the power, 2016-08-29 16:43:36]

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-08-30 03:24:51]

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

[commit-bot: I haz the power, 2016-08-30 03:24:58]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[braveyao, 2016-08-30 16:25:57]

The CQ bit was checked by braveyao@chromium.org

[commit-bot: I haz the power, 2016-08-30 16:26:16]

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-08-30 18:47:37]

Description was changed from

==========
Android: move peerconnection out of power observer.

Currently when screen is off (or Chrome goes to background) for more than 1 min,
peerconnection will be stoppted by PowerMonitor to save battery power. This is
not necesary now since we'll suspend video call when the tab is background. And
users prefer that audio call can continue while they are doing something else.

BUG=513633
==========

to

==========
Android: move peerconnection out of power observer.

Currently when screen is off (or Chrome goes to background) for more than 1 min,
peerconnection will be stoppted by PowerMonitor to save battery power. This is
not necesary now since we'll suspend video call when the tab is background. And
users prefer that audio call can continue while they are doing something else.

BUG=513633
==========

[commit-bot: I haz the power, 2016-08-30 18:47:38]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-08-30 18:51:49]

Description was changed from

==========
Android: move peerconnection out of power observer.

Currently when screen is off (or Chrome goes to background) for more than 1 min,
peerconnection will be stoppted by PowerMonitor to save battery power. This is
not necesary now since we'll suspend video call when the tab is background. And
users prefer that audio call can continue while they are doing something else.

BUG=513633
==========

to

==========
Android: move peerconnection out of power observer.

Currently when screen is off (or Chrome goes to background) for more than 1 min,
peerconnection will be stoppted by PowerMonitor to save battery power. This is
not necesary now since we'll suspend video call when the tab is background. And
users prefer that audio call can continue while they are doing something else.

BUG=513633

Committed: https://crrev.com/22c91cc29993f5f7a9f1744349b9b2065ab266bd
Cr-Commit-Position: refs/heads/master@{#415372}
==========

[commit-bot: I haz the power, 2016-08-30 18:51:50]

Patchset 6 (id:??) landed as
https://crrev.com/22c91cc29993f5f7a9f1744349b9b2065ab266bd
Cr-Commit-Position: refs/heads/master@{#415372}