828283 - Apps playing sound should not be muted when the screen is put to sleep.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Daniel Coloma:dcoloma]

There are multiple apps in e.me and in Mozilla Market to play music. When you are listening to music and screen is turned off or apps are moved to the background, those apps cannot continue playing music, unless they explicitly request to do so, which requires a lot of evangelisation work. We've talked to Mozilla Product Team (Paxton) and we all agree that the default behaviour should be continue playing sound. Nominating as this is a must have for product team.

Comment 1

[Jason Smith [:jsmith]]

I'm guessing by your comment that you meant to nom.

Comment 2

[Daniel Coloma:dcoloma]

(In reply to Jason Smith [:jsmith] from comment #1)
> I'm guessing by your comment that you meant to nom.

thanks!

Comment 3

[Jason Smith [:jsmith]]

Btw - there's controversy around comment 0. We have a bug that says the exact opposite in Android Web Apps - 3rd party apps should *not* play music by default if they are in the background. I wonder if there's security piece we should throw in here.

Should a developer be required to specify a permission to allow music to play in the background?

Jonas & Lucas - What do you think? Allow by default or allow when a permission is specified?

Comment 4

[Lucas Adamski [:ladamski]]

This sounds like a problem best solved by https://wiki.mozilla.org/WebAPI/AudioChannels

Trying to make changes around audio channel behavior seems highly risk so late in the cycle.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Implementation-wise this is not hard to fix. It's not too big of a code change to change the AudioChannelService such that we don't mute channels for applications that are in the background.

The problem isn't a coding one though.

If we simply allow applications to use audio while in the background that will mean that if you are playing a game which has sound effects or background music (which in games very quickly becomes very repetative) that audio will keep playing when you leave the application.

Similarly, audio from pages and apps can and will suddenly start playing and the user will have no idea which application is playing. Keep in mind that the application switcher (i.e. the card view) is a power-user feature.

This means that users not only will have no clue which applications are currently running so that they can look through them and see which one is likely to be causing the audio to play. The user will have to try to remember which apps they have been using and re-open them all using the home screen.

Additionally the user might not always know how to stop the audio from playing since the only way to reliably do that is to shut down the application, which again requires knowing how to use the card view.

Another problem we would have is that we couldn't implement logic like the one in bug 812691. I.e. if two everything.me applications are both playing audio at the same time we would end up playing them over each other rather than muting all but the last one that was started.

All of this is why we went through the trouble of implementing the current policy.


I do absolutely understand the goal of wanting to enable everything.me application and homescreen bookmarks to implement background music playing applications. But I think doing that requires a more comprehensive system than simply letting any app play its audio in the background.


If you still think that we need to enable this use case, then we need to get a group together and talk through the various requirements and how to solve the many edge cases that exist.

We are still fixing edge cases with the current, simpler, policies. So fixing this bug definitely will not be possible to do by the end of this week.

Comment 6

[Lucas Adamski [:ladamski]]

Awaiting blocking decision per conversations between Jonas and Daniel.

Comment 7

[Peter Dolanjski [:pdol]]

The Product and UX teams met with Jonas to discuss this.
The conclusion is that we would like the following approach taken:
Any application (no matter the type), which is in the foreground, should continue to play audio when the device:
a) is placed in a lock state (through the lock button) by the user OR
b) goes to sleep after the specified timeout in the settings.

Further, any application (no matter the type) which is moved to the background should cease playing audio.
If an application that is in the background is moved into the foreground, formerly ceased audio playback for that application should resume.

The key use case that we are looking to satisfy with this approach is the user using a media web application while not actively interacting with the device.  Eg. Starting audio playbook, locking the device and placing into a pocket.

Jonas, if it is better to - this one and file a new bug, please do so.

Comment 8

[Lucas Adamski [:ladamski]]

(In reply to Peter Dolanjski from comment #7)
> Further, any application (no matter the type) which is moved to the
> background should cease playing audio.
> If an application that is in the background is moved into the foreground,
> formerly ceased audio playback for that application should resume.

Including telephony?

What about a voip app (once we support webRTC)?

Comment 9

[Lucas Adamski [:ladamski]]

... and navigation?

Comment 10

[Peter Dolanjski [:pdol]]

(In reply to Lucas Adamski from comment #8)
> 
> Including telephony?
> 
> What about a voip app (once we support webRTC)?

I should have clarified that this wouldn't apply to packaged apps which use the appropriate API to continue audio while in the background.  Forgive me, I don't know which API this is.  Jonas described it to us.

We can re-evaluate after v1.0 and possibly remove the difficulty for the user in visually understanding what is causing the sound, or providing a simpler way to kill an app. (or alternatives)

Comment 11

[Lucas Adamski [:ladamski]]

Ok, thanks for the clarification, makes sense.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yup, we're going to do this. But we won't have time to do it in time for the basecamp deadline since it was a late change.

Comment 13

[Kai-Chih Hu [:khu]]

Should we discuss this again to see if we should tef+? 
Thanks.

Comment 14

[Daniel Coloma:dcoloma]

(In reply to khu from comment #13)
> Should we discuss this again to see if we should tef+? 
> Thanks.

No, this is a product blocker, no discussion required, can you set up the flag again.

Comment 15

[Kai-Chih Hu [:khu]]

Sure. Sorry. I saw it was tef+ on 1/11 and thought it might need to re-triage again. 

tef+ again. Done.

Comment 16

[Marco Chen [:mchen]]

(In reply to Peter Dolanjski from comment #7)
> The Product and UX teams met with Jonas to discuss this.
> The conclusion is that we would like the following approach taken:
> Any application (no matter the type), which is in the foreground, should
> continue to play audio when the device:
> a) is placed in a lock state (through the lock button) by the user OR
> b) goes to sleep after the specified timeout in the settings.

Considering the case b on following scenario, 
  1. User is looking at a web page with background music (no control panel). According to some events he want to suspend the device and go to other thing. In this time, he will find the music continue to play. After waking up device again, there is no way to close the music until he finally move browser to background.

I think no matter what content in the web page it is still a web page only. If it want to support more, maybe it need to have a new app in market for this. Just provide my thought.

Comment 17

[Andrew Overholt [:overholt]]

Consider the following scenario:

- a co-worker is playing a game on his phone with background music that is very repetitive
- this co-worker gets called away from his desk quickly and leaves his phone with the game active
- time passes and his device automatically locks
- I get very annoyed with the game's music and want to turn it off but the phone is locked
- my options at this point are:

a) realize the hardware volume buttons work and turn it down
b) rip out the battery :)

I just want to make sure we're doing this intentionally.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There are definitely very tricky tradeoffs here. There are no good solutions.

The reason we decided to go with the solution in comment 7 is mainly to deal with the fact that there is tons of awesome web content out there. But very little of it is currently available as a "real app" in the B2G sense. I.e. very little of it has app manifest making it installable as a real app.

I think we've been decently good at making it very easy to turn a website into a installable webapp. And we've been good at making it possible to flag an audio channel as "should keep playing even when leaving the application" by simply adding a mozaudiochannel="content" attribute.

However *any* work needed on the website's side means that we immediately drop the vast majority of websites.


So as a way to bootstrap, the solution in comment 7 was the one with the lowest impact we could come up with. Leaving the app silences it, but it's still possible to use a normal website as your music player.


And after all, if there's music that is annoying you, or the people around you, you are unlikely to let your phone just sit there and wait for the screen timeout to silence it.

I'm more worried about users being on some noisy website and then wanting to quickly turn the phone off and stop using it. Here the user will have to remember to first quit the app and then press the power button.

I'm also somewhat worried about some website silently playing audio and the user not noticing, which can cause the phone to not go to sleep when the user presses the power button, which results in battery drainage.


But we should give this a try for now. A nice thing with this policy is that there's still plenty of incentive for apps to use the proper "content" channel since that's the only way to get audio working while still letting the user use other apps.

So we can possibly change this policy in the future once there are more websites that have been converted into proper apps. Or we can add more APIs and UI to give the user more control over behavior.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch for gaia

Comment 20

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 21

[Andrea Marchesini [:baku]]

These 2 patches do not call setVisible(false) to the app when the device is locked if something is playing. I need a feedback.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 704011 [details] [diff] [review]
patch

Review of attachment 704011 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/chrome/content/shell.js
@@ +1049,5 @@
>      shell.sendChromeEvent({
>        type: 'audio-channel-changed',
>        channel: aData
>      });
> +    shell.audioActive = (aData !== 'none');

Maybe change this to be aData === "normal" || aData === "content"?

Comment 23

[Justin Lebar (not reading bugmail)]

Comment on attachment 704011 [details] [diff] [review]
patch

> +        if (window.windowState == window.STATE_MINIMIZED && !this.audioActive) {
>           this.contentBrowser.setVisible(false);

This says that we don't mark a page as visible==false when the phone is locked, if the page is playing audio when the lock-screen appears.

This far-reaching and content-visible effects.  It will prevent the page from receiving a visibilitychanged event.  It will affect whether we're willing to throw away webgl contexts on the page.  It will affect the page's CPU and OOM-killer priority.  And so on.

Maybe this is the right thing to do given our time-schedule, but we should be aware of the effects.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think the general approach here seems workable. It's a bit unfortunate to rely on the audio-channel-changed notification since it complicates situations like if the alarm is sounding while there's a website playing audio. But I think it'll work well enough for now.

We should have someone from the gaia team review the gaia patch though.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Justin Lebar [:jlebar] from comment #23)
> Maybe this is the right thing to do given our time-schedule, but we should
> be aware of the effects.

Other proposals welcome. I can think of at least two other solutions:

* Move more of the audio management to the browser API rather than doing it automatically in gecko. We of course couldn't allow the browser app to prevent audio from being muted when there's an incoming phone call. But we could probably come up with some sort of policy which says that if *any* of the ancestors in the <iframe mozbrowser> ancestor chain says that something should be muted it will be.

This way the system app could avoid pausing the front-most app when turning off the screen, but we could otherwise set the page to being non-visible.

* Being able to propagate "visible-but-frontmost" down the iframe hierarchy and into the AudioChannelService the same way that we currently propagate the visibility state. It would have to be a parallel state in some sense of course, since the goal here is to keep the current visibility behavior the same.

Comment 26

[Andrea Marchesini [:baku]]

Attachment: patch for gaia

Comment 27

[Andrea Marchesini [:baku]]

Attachment: patch

This patch implements a new event called 'visible-audio-channel-changed'.
This informs the highest audio-channel that is visible and it's playing. It can be 'none' if the active audio-channel is in background.

Comment 28

[Marco Chen [:mchen]]

May I know that is the decision here conflict with Bug 833211?
Which one asked youtube player to stop play during user pressing power key.

Thanks.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 704527 [details] [diff] [review]
patch for gaia

Review of attachment 704527 [details] [diff] [review]:
-----------------------------------------------------------------

This needs to be reviewed by someone from the gaia team. It looks good to me though.

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 704528 [details] [diff] [review]
patch

Review of attachment 704528 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: dom/audiochannel/AudioChannelService.cpp
@@ +267,1 @@
>    }

Could you simply do:

AudioChannelType visibleHigher = higher;

after all of these statement?

@@ +317,5 @@
> +      channelName.AssignLiteral("none");
> +    }
> +
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    obs->NotifyObservers(nullptr, "visible-audio-channel-changed", channelName.get());

Can't this result in two notifications being sent? That seems unnecessary. Can you collapse the code so that we only send one.

Comment 31

[Alexandre Poirot [:ochameau]]

Comment on attachment 704527 [details] [diff] [review]
patch for gaia

Forwarding the review to alive as I'm not sure I reached the reviewer status for this code ;)

Note that we current gaia (dcb5d76960492b87864f17325faa0f03e8306827) and b2g-18 (dd0939447e649bd569674fff4ca85f5b9e464e2c) the sound never stop untils I kill the music app.

Comment 32

[James Lal [:lightsofapollo]]

Is there a pull request for gaia with this patch? Review normally happens on the pull request itself.

Comment 33

[David Flanagan [:djf]]

Comment on attachment 704527 [details] [diff] [review]
patch for gaia

Review of attachment 704527 [details] [diff] [review]:
-----------------------------------------------------------------

This feels like a hack, but we're up against the deadline, so a reluctant r+, if you consider my commenting and variable naming suggestions.

Also, I don't have time to build the gecko patch and try it out myself, so the review is based on code inspection not testing.

::: apps/system/js/window_manager.js
@@ +97,4 @@
>    // The origin of the currently displayed app, or null if there isn't one
>    var displayedApp = null;
>  
> +  var visibleAudioActive = false;

Please add a comment saying what this variable is for, including where it is used and where it is set, since it is so far away from the code that uses it.

I'd prefer it if it was named currentAppPlayingAudio or something.

@@ +1485,5 @@
>          setVisibilityForCurrentApp(true);
>          break;
>        case 'lock':
> +        if (!visibleAudioActive)
> +          setVisibilityForCurrentApp(false);

I'd add a comment explaining the if statement, since it is not clear to me that the visiblity of an app has anything to do with its ability to continue playing sound.

@@ +1511,5 @@
>          break;
> +      case 'mozChromeEvent':
> +        if (evt.detail.type == 'visible-audio-channel-changed') {
> +          visibleAudioActive = (evt.detail.channel !== 'none');
> +        }

add a break at the end of this case.

Should we consider checking whether the phone is locked here and setting or clearing its visibility based on the state of the audio channel?

Comment 34

[(inactive after 6/18) Alive Kuo [:alive]]

I am fine with the hack, since we may not to fix 833211 for v1.
We have to revisit the visibility set path afterwards. I had already mentioned this in dev-gaia.

Comment 35

[(inactive after 6/18) Alive Kuo [:alive]]

And for long term, I hope platform supports that the app could know the difference: visibility=false from screen Is Off. Now they are the same thing to the app.
The patch of this bug in platform looks like another hack I think. Why do we have to prevent an audio-playing app to setVisible(false); ? How about if there are some visibility issues afterwards? I agree with djf this is totally unclear.

Comment 36

[Marco Chen [:mchen]]

The patches here will also effect the video tag too.
Once a video is playing and user pressed power key for screen off. After landing this bug, the sound will be continued but user can't see any video frames. Is this a correct user story?

Or the patches here should distinguish whether the one using audio channel is from audio or video tag?

Comment 37

[(inactive after 6/18) Alive Kuo [:alive]]

For me I would hope this is configurable. Music Video(MV) is a case that needs to be played continuously when device is off.
Otherwise.. as an user I don't really care.

(In reply to Marco Chen [:mchen] from comment #36)
> The patches here will also effect the video tag too.
> Once a video is playing and user pressed power key for screen off. After
> landing this bug, the sound will be continued but user can't see any video
> frames. Is this a correct user story?
> 
> Or the patches here should distinguish whether the one using audio channel
> is from audio or video tag?

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Marco Chen [:mchen] from comment #36)
> The patches here will also effect the video tag too.
> Once a video is playing and user pressed power key for screen off. After
> landing this bug, the sound will be continued but user can't see any video
> frames. Is this a correct user story?
> 
> Or the patches here should distinguish whether the one using audio channel
> is from audio or video tag?

The current behavior is on request from Telefonica and what we ended up doing a very quick and last-minute design for in Berlin which resulted in the agreement in comment 7. I definitely wish we would have had more time to design something better as well as get you, Alive and Andrea in on the discussion, but unfortunately we didn't have time.

I think for v2 we should design something better. The current APIs are definitely very hacky and the behavior in bug 833211 is just one sideeffect of that.

What I would in particular like to do is to expose more information through the Browser API which gives more fine-grained control over what audio is paused and also allows building better UIs. For example to allow gaia to select on a per-website or per-application basis what audio keeps playing when the screen is turned off.

But let's file separate bugs on that and make sure we get something good in place for v2.

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Andrea: Are we ready to land the patches here? The bug is marked as tef+ so you should be able to land on all needed branches.

Comment 40

[Daniel Coloma:dcoloma]

> 
> The current behavior is on request from Telefonica 

A minor comment here, Telefónica team discussed this with Mozilla Product team and the behaviour is a request from the Product team.

Comment 41

[Andrea Marchesini [:baku]]

> > +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> > +    obs->NotifyObservers(nullptr, "visible-audio-channel-changed", channelName.get());
> 
> Can't this result in two notifications being sent? That seems unnecessary.
> Can you collapse the code so that we only send one.

Collapsing these 2 notifications means the creation of a nsISupports object or the creation of a JSON object. is it? I prefer to do this in a follow up bug.

Comment 42

[Andrea Marchesini [:baku]]

Attachment: patch b

Comment 43

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0fc7ce285e1

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2f062607dae3

Comment 44

[Andrea Marchesini [:baku]]

Comment on attachment 704527 [details] [diff] [review]
patch for gaia

Review of attachment 704527 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/system/js/window_manager.js
@@ +1511,5 @@
>          break;
> +      case 'mozChromeEvent':
> +        if (evt.detail.type == 'visible-audio-channel-changed') {
> +          visibleAudioActive = (evt.detail.channel !== 'none');
> +        }

This would mean several things:

. The app stops playing when the device is locked. I don't think we should change the visibility because maybe it will start playing soon (I'm thinking about a playlist: between a song and the next one maybe the active channel can change).

. the app starts playing when the device is locked: we can ignore this case.

Comment 45

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/7820

Comment 46

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/b30d5224b47b

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0fc7ce285e1

Comment 48

[Nick Davidson]

Verified - resolved  on Unagi
Build ID 20130214070203
gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e
Gaia: 6544fdb8dddc56f1aefe94482402488c89eeec49

Comment 49

[Marco Chen [:mchen]]

Hi Andrea,

It seems that the gaia patch doesn't be landed to v1-train. Does it need to do this?

Comment 50

[Andrea Marchesini [:baku]]

Can someone from gaia team land this patch? Good catch, Marco.

Comment 51

[Justin Lebar (not reading bugmail)]

Does this keep the music-playing app in the /foreground/ while it's playing music?  We never call setVisibility(false) on the app?

Not only would this cause bug 842506, but it would also cause the music-playing app to have priority FOREGROUND, which means that the music-playing app will compete for resources with the currently-active app.  In particular, the music app will compete for resources with an app we're trying to load.  In other words, this cause us not to be able to load apps.

Comment 52

[Justin Lebar (not reading bugmail)]

Comment on attachment 707014 [details] [diff] [review]
patch b

I don't think this is correct, sorry.

Comment 53

[Justin Lebar (not reading bugmail)]

For example, AFAICT we don't ever get to this code

>diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp
>--- a/dom/ipc/ProcessPriorityManager.cpp
>+++ b/dom/ipc/ProcessPriorityManager.cpp
>@@ -70,17 +70,17 @@ static PRLogModuleInfo* logModule = PR_N
> 
> /**
>  * Get the appropriate backround priority for this process.
>  */
> ProcessPriority
> GetBackgroundPriority()
> {
>   AudioChannelService* service = AudioChannelService::GetAudioChannelService();
>-  if (service->ContentChannelIsActive()) {
>+  if (service->ContentOrNormalChannelIsActive()) {

because we never mark the audio-playing app as being in the background.

Comment 54

[Justin Lebar (not reading bugmail)]

Oh, I see; we keep the app in the fg only when we're showing the lock screen.  I guess the main side-effect is bug 842506, then.  Another side-effect is that this process will incorrectly compete with apps who try to load due to receiving a system message.  Another is that WebGL uses the process's visibility state to know when to throw away memory it's using...

Comment 55

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Indeed, we are not competing with other apps.

Justin, if you have proposals for a better-but-still-safe way to fix this, then I'm all ears.

I think the fact that we are using more batteries than needed when music is playing and the screen is turned off is very unfortunate, but not a blocker. I'm happy to try to figure out a better strategy for the v1.1 release. But let's do that in a different bug.

Comment 56

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, we are already by design competing with other apps that are loaded through system messages. Any audio-playing app should have a higher OOM-priority than apps that are running in the background since audio-playing apps are perceivable to the user.

Comment 57

[Justin Lebar (not reading bugmail)]

> Justin, if you have proposals for a better-but-still-safe way to fix this, then I'm all ears.

I don't think it's sensible that content will get a visibilitychange event when the screen turns off iff content is not playing audio.  That is already breaking content; see bug 836214.

The issue here is that Gaia is not giving Gecko enough information to determine whether this audio channel should be playing.  setVisible isn't the right bit of info.  The obvious solution (to me) is to add an additional bit of info.  We can debate exactly what that should be; we have options.

Comment 58

[Justin Lebar (not reading bugmail)]

> We can debate exactly what that should be; we have options.

To be less coy about it, we could for example have gaia setVisible('hidden') or something, which means something different than setVisible(false).  Or we could have a separate setActive() method.

Comment 59

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So what we need is setVisibility("hidden-but-audio-still-playing") which acts just like visibility hidden but otherwise signals to the audio channel manager that audio should still keep playing.

Is that something that you think we can implement in time for the shira/tef release?

Comment 60

[Justin Lebar (not reading bugmail)]

> So what we need is setVisibility("hidden-but-audio-still-playing") 

"hidden-but-audio-may-still-play" is what I was thinking.  Unless the UX you want is that if the app is not playing sound when the screen is switched off, it can never start playing sound?  I can see why that would be good, but it's also confusing and racy...

> Is that something that you think we can implement in time for the shira/tef release?

That would be a change comparable in complexity to this patch, so not a huge deal, I think.

Comment 61

[Marco Chen [:mchen]]

Is it possible to split the visible and screen off events and not set foreground app to invisible during screen off?

Visible event now will be used as this document is on the foreground.
Screen off will be used to notify the screen is off now.

So the composition can let app know it is on the foreground but actually the screen is off.
How about this?

Comment 62

[Justin Lebar (not reading bugmail)]

We could do this, but we'd have to introduce a "screen-off" event, AFAICT.

IMO the use-case in bug 836214 is /exactly/ what the visibilitychange event is for, and we should use that.  We'll want other apps which do things in the background to also stop updating their visual elements when the screen is off, and we shouldn't have to evangelize a new, unnecessary event.

Comment 63

[(inactive after 6/18) Alive Kuo [:alive]]

Could we have this done in platform only? I guess platform also know the status of screen on/off, and a new API about setVisible("hidden-but-audio-still-playing") could be used there(shell.js?) only.(instead of expose to gaia)

Comment 64

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #62)
> We'll want other apps which do things in the background to also stop updating their visual 
> elements when the screen is off

The mechanism in Comment 61 doesn't effect other apps "which are really in the background" but only the app in the top.

> use-case in bug 836214 is /exactly/ what the visibilitychange event is for

Consider to the desktop browser. An tab on the focus will be visible state then once user close the power of panel it is still visible and can receive any key or mouse events.
Mapping to the mobile phone, it is the same situation. But in order to save the power by freeze UI we just introduce a new event to indicate screen off.

Comment 65

[(inactive after 6/18) Alive Kuo [:alive]]

Marco's suggestion also works for me. I had the same thought in comment 35.
But expose new screen state needs Web API love ;)

Comment 66

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We should not introduce new events here. We already have the set of events needed and we also have a host of code in gecko which ensures that pages that have their visibility set to hidden will use less CPU.

But I'm very reluctant to rewrite how we are doing all this audio stuff unless we have proof that it's actually needed. My understanding of bug 836214 is that most of the bad behavior seen there is not due to visibility changes not happning, but rather due to other gecko internals. Please correct me if I'm wrong.

Comment 67

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

According to the comments in bug 836214 what we have right now is acceptable for v1.0 "assuming we can't shake out some *very* low risk win for v1.0.x".

Justin, would rewriting the visibility stuff really be so low risk at this point? I know the schedules are a disaster which makes the riskyness next to impossible to gauge. Which IMHO means that we should play it safe.

Comment 68

[Marco Chen [:mchen]]

(In reply to Jonas Sicking (:sicking) from comment #66)
> But I'm very reluctant to rewrite how we are doing all this audio stuff
> unless we have proof that it's actually needed. My understanding of bug
> 836214 is that most of the bad behavior seen there is not due to visibility
> changes not happning, but rather due to other gecko internals. Please
> correct me if I'm wrong.

The issue introduced from bug 836214 on music app is caused by "visibilitychange event". Music app needs to receive mozvisibilitychange event for indicating non-visible state, so it can stop to update UI. But the patch here will block that event.

Consider that, a user plays a game app and he presses the power key to screen off state for some reason. In this case the game app will not receive non-visible state so the game is still on going. This is not "make sense" and not the behavior on any mobile OS now.

Comment 69

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marco: I'm very aware of that. The question isn't what we would do in an ideal world, the question is what we can do safely given that we're about 9 days from branch point.

Comment 70

[Justin Lebar (not reading bugmail)]

Jonas doesn't want to fix this regression for v1, and I don't think it's a useful exercise to continue arguing the point; we have other bugs that we can fix without debate.

Comment 71

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Justin, the question isn't what I want. The question is what we can do safely at this point. If you have a counter proposal to the currently attached patch that you think is safe, then I'm fine with taking that.

Comment 72

[Andrew Overholt [:overholt]]

To be clear here, it appears we have landed the Gecko patch but not the corresponding Gaia patch.

Comment 73

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I talked to Justin on irc and he said he'd write up an alternative patch so that we could evaluate risk of it.

Comment 74

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Or rather, he'd try to find the time to do so. No promises at this point.

Comment 75

[Ryan VanderMeulen [:RyanVM]]

Given the lack of consensus, removing checkin-needed for now.

Comment 76

[Justin Lebar (not reading bugmail)]

Comment on attachment 704527 [details] [diff] [review]
patch for gaia

Jonas will check with product management, but he believes that the feature here is more important than the feature that this breaks.  If so, I retract my r-.

Comment 77

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The patches here are not enough to fix this bug.

As far as I can tell nothing is preventing the device from going to sleep after the screen has turned off. I.e. nothing is preventing the CPU from going into sleep state even though the current front-most app is playing audio.

I thought that the system app was monitoring not just the "cpu" wakelock when deciding when to set power.cpuSleepAllowed, but also monitored if someone is using the "content" channel. And that the patch here changed it so that we also monitor the front-most app playing audio.

That doesn't appear to be the case and is something that we need to fix.

At the very least we need to set power.cpuSleepAllowed to false whenever we are wanting to keep playing audio from a website even when the screen is turned off.

Comment 78

[Justin Lebar (not reading bugmail)]

> That doesn't appear to be the case and is something that we need to fix.

Okay, I didn't understand that we wanted to fix this bug.

Do you know whether it's actually a problem?  Does audio not play properly if the CPU goes into low-power mode?

Comment 79

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Audio is silenced once we enter sleep mode. So if we want anything to keep playing we need to prevent entering sleep mode.

Comment 80

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I also checked with Michael Vines and for v1.0 it's ok if we use the solution here as a way to let audio play while the device has the screen turned off. We definitely should find a better solution for v1.1 but I think we want to make some bigger changes there.

Ideal would be if we do set the visibility of the frame to "hidden" if all audio is coming from the "content" channel. That way we get good battery behavior for the built-in music app even if we don't when random webpages are used to play audio.

Comment 81

[Justin Lebar (not reading bugmail)]

This is easy enough to fix.  We can take a "cpu" wake lock on behalf of the appropriate process while audio is playing and release it (say) 1s after audio stops playing in that process.  See Hal.h.

Comment 82

[Andrea Marchesini [:baku]]

Attachment: patch for gaia - cpuSleepAllowed

Sicking, can you give a feedback about this patch? Is it enough?

Comment 83

[Justin Lebar (not reading bugmail)]

Comment on attachment 718460 [details] [diff] [review]
patch for gaia - cpuSleepAllowed

Can you please use the CPU wake lock for this?  That's what it's for.

We should not even have this audio-channel-changed chrome event; I want to write a patch to remove it.

Comment 84

[Justin Lebar (not reading bugmail)]

Also, /any/ page--app or not--playing sound should keep the device from going to sleep, I think.

It does not make sense to require apps to acquire the CPU wake lock but to acquire the CPU wake lock on behalf of pages, particularly since wake locks are not an app-only feature.

Comment 85

[Andrea Marchesini [:baku]]

Attachment: patch for gaia - cpuSleepAllowed

I'm just uploading a working patch. The previous one was uncompleted.
But I agree with JLebar. Let me try to implement something in the AudioChannelService if we agree with it.

Comment 86

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Justin, the plan is definitely to remove audio-channel-changed. The plan is to move the information to the browser API instead. The original intent of audio-channel-changed was to just use that to drive decisions about which volume to adjust when the volume buttons are pressed, we need to get back to that after 1.0

(In reply to Justin Lebar [:jlebar] from comment #84)
> Also, /any/ page--app or not--playing sound should keep the device from
> going to sleep, I think.

Agreed, that's what the latest patch does, modulo some bugs (need to update power.cpuSleepAllowed in more places)

> It does not make sense to require apps to acquire the CPU wake lock but to
> acquire the CPU wake lock on behalf of pages, particularly since wake locks
> are not an app-only feature.

I'm not successfully parsing this.

Comment 87

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 718478 [details] [diff] [review]
patch for gaia - cpuSleepAllowed

Review of attachment 718478 [details] [diff] [review]:
-----------------------------------------------------------------

The approach looks good, but r- from me due to the issue below.

::: apps/system/js/screen_manager.js
@@ +290,5 @@
> +          } else {
> +            var self = this;
> +            this._audioCpuSleepTimerId = setTimeout(function cpuSleepTimer() {
> +              self.refreshCpuSleepAllowed();
> +              self._audioCpuSleepTimerId = 0;

There's a race bug here. If a wakelock changes while the timer is active then we might set cpuSleepAllowed=true before the timeout happens.

Either change the logic here so that _audioActive is only set to false after the timeout fires, or change it so that the refreshCpuSleepAllowed function looks for either _audioActive being set, or _audioCpuSleepTimerId being set.

I think setting _audioActive to false from the timer is probably the cleaner solution.

Comment 88

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 704527 [details] [diff] [review]
patch for gaia

Review of attachment 704527 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/system/js/window_manager.js
@@ +1510,5 @@
>          }
>          break;
> +      case 'mozChromeEvent':
> +        if (evt.detail.type == 'visible-audio-channel-changed') {
> +          visibleAudioActive = (evt.detail.channel !== 'none');

Could we change this to be

visibleAudioActive = (evt.detail.channel === 'normal');

(and maybe rename to something more appropriate).

That way we still call setVisibility("hidden") if things like "content" or "alarm" is being used. Which means that we'll still get the battery savings when proper channels are being used, like in the music app.

And only do the battery-suboptimal strategy when the "normal" channel is being used.

Comment 89

[Andrea Marchesini [:baku]]

Attachment: patch for gaia - cpuSleepAllowed (b)

Comment 90

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 718493 [details] [diff] [review]
patch for gaia - cpuSleepAllowed (b)

Review of attachment 718493 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great

Comment 91

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 718493 [details] [diff] [review]
patch for gaia - cpuSleepAllowed (b)

Review of attachment 718493 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if we could accept this change only for v1.

Comment 92

[(inactive after 6/18) Alive Kuo [:alive]]

If we all agree this is a sad work around, Justin or Jonas, please file another one, or two, bug to track it.
Sounds like a new BrowserAPI...any let's discuss there, and I could take the gaia part. Thanks

Comment 93

[Andrea Marchesini [:baku]]

Attachment: patch for gaia

Sorry, I merged 2 patches: it's easier for me to have them in one.
I didn't change the screen_manager.js but I just applied some changes following the comments to window_manager.js.

Comment 94

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 718939 [details] [diff] [review]
patch for gaia

Review of attachment 718939 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if below is true.

::: apps/system/js/window_manager.js
@@ +1623,5 @@
>        case 'lock':
> +        // If the audio is active, the app should not set non-visible
> +        // otherwise it will be muted.
> +        if (!normalAudioChannelActive) {
> +          setVisibilityForCurrentApp(false);

My question is: 
You are leaving the content-playing app with visibility on here.
But when the content music stops, who would be the one to turn off the visible state?
Is platform doing that?

Comment 95

[Andrea Marchesini [:baku]]

Attachment: patch for gaia (b)

Good point. I don't know if this is enough but with this additional patch we have a timer that starts when the normal channel is not active and the device is locked.

Comment 96

[Andrea Marchesini [:baku]]

I added this dependence because audio-channel-changed is emitted wrongly without the patch for that bug.

Comment 97

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 718980 [details] [diff] [review]
patch for gaia (b)

Review of attachment 718980 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if you try another way to track device lock state.

I don't test this patch manually, please try to find anything that may cause race condition if possible.
Although this code looks fine to me.

::: apps/system/js/window_manager.js
@@ +103,5 @@
> +  var normalAudioChannelActive = false;
> +
> +  // Track if the device is locked or not. This is needed to restore
> +  // the visibility for apps that play audio.
> +  var deviceLocked = false;

I believe you don't need an additional variable here.
See ScreenManager or Lockscreen, there should be an attribute for this.

@@ +1678,5 @@
> +
> +          if (normalAudioChannelActive && evt.detail.channel !== 'normal' &&
> +              deviceLocked) {
> +            deviceLockedTimer = setTimeout(function setVisibility() {
> +              setVisibilityForCurrentApp(false);

So the flow of device off/on to audio playing app now is:
1. Screen is off.
2.a Lockscreen is enabled => lock event is dispatched
3.a WM emit the visibility change action if it got a visible-audio-channel-changed event before screen off.

2.b Lockscreen is disabled => gecko turn off visibility of current app(but with the platform patch of this bug, it would do nothing if the app content is playing audio.
3.b If the audio stops playing, gecko fires visible-audio-channel-changed event and then WM would turn off its visibility after 3 sec.
4.b It the screen is turned on before 3sec, WM discards the turn off timer.

Comment 98

[Andrea Marchesini [:baku]]

> r+ if you try another way to track device lock state.

I'm trying LockScreen.locked


> So the flow of device off/on to audio playing app now is:
> 1. Screen is off.
> 2.a Lockscreen is enabled => lock event is dispatched
> 3.a WM emit the visibility change action if it got a
> visible-audio-channel-changed event before screen off.

right.

> 2.b Lockscreen is disabled => gecko turn off visibility of current app(but
> with the platform patch of this bug, it would do nothing if the app content
> is playing audio.
> 3.b If the audio stops playing, gecko fires visible-audio-channel-changed
> event and then WM would turn off its visibility after 3 sec.
> 4.b It the screen is turned on before 3sec, WM discards the turn off timer.

yes.

Comment 99

[Andrea Marchesini [:baku]]

Attachment: patch for gaia

Comment 100

[Andrea Marchesini [:baku]]

of course checkin-needed is just for gaia.

Comment 101

[John Ford [:jhford] CET/CEST Berlin Time]

master: bd65308e5a9fa045e9a0cc83368df0e0ab7b9c3e
v1-train: 79f9d478acf6eb2ebe277b58b9c5ba6f9cb88c7d
v1.0.1: 706123b3ccbff7c773b882cf0dc9b95a133db245

Comment 102

[Michael Vines [:m1] [:evilmachines]]

FYI.  We see a pretty big power regression on v1.0.1 as a result of this bug.  I think this is expected but just adding some detail here for posterity and so that folks fully appreciate the power trade-off we made here.

Consider a case where we are running game which is graphics intensive with sound enabled and if user turns screen off, game will still be running because we did not let CPU to go in sleep (game still think it's visible and continues rendering in addition to playing audio).

Comment 103

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I strongly suspect that the majority of battery consumption is due to the fact that we're not putting the CPU to sleep. The fact that we're not putting the app in the non-visible state surely doesn't help, but I would think has a smaller effect.

Bug 853101 should help with he visibility issue. It should also let us build more advanced UI which would enable the user to choose which apps this happens to and which ones it doesn't.

Comment 104

[:CSchmoeckel]

Added 3 new Test Cases to the User Sound test suite in MozTrap. 

Screen Sleeps - https://moztrap.mozilla.org/manage/cases/?filter-id=6855&filter-suite=208
Moved to Background - https://moztrap.mozilla.org/manage/cases/?filter-id=6858&filter-suite=208
Lock/Unlock Device - https://moztrap.mozilla.org/manage/cases/?filter-id=6859&filter-suite=208

Comment 105

[mdaniloff]

This bug has been Verified-Fixed. Video playback was at 25-29 FPS with the Show Frames per second enabled in the Developer settings:
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/4931ec89ebbe
Gaia: 85fd164691bb852f1cfaf82405df4380629ced6e

Comment 106

[hanj.kim25]

This patch causes a regression to the Camera app.

The Camera app listens to the 'mozvisibilitychange' event. When a phone is locked, it calls the Camera.stopPreview(). Because this patch prevents from sending the 'mozvisibilitychange' event to the Camera app, the Camera app doesn't call stopPreview even when a phone is locked and results in battery drain. I'm reopening this bug..

Comment 107

[hanj.kim25]

This is where I am talking about. 

In window_manager.js

       case 'lock':
-        setVisibilityForCurrentApp(false);
+        // If the audio is active, the app should not set non-visible
+        // otherwise it will be muted.
+        if (!normalAudioChannelActive) {
+          setVisibilityForCurrentApp(false);
+        }
+        resetDeviceLockedTimer();
         break;

And in camera.js

document.addEventListener('mozvisibilitychange', function() {
  if (document.mozHidden) {
    Camera.stopPreview();
    Camera.cancelPick();
    Camera.cancelPositionUpdate();
    if (this._secureMode) // If the lockscreen is locked
      Filmstrip.clear();  // then forget everything when closing camera
  } else {
    Camera.startPreview();
  }
});



(In reply to hanj.kim25 from comment #106)
> This patch causes a regression to the Camera app.
> 
> The Camera app listens to the 'mozvisibilitychange' event. When a phone is
> locked, it calls the Camera.stopPreview(). Because this patch prevents from
> sending the 'mozvisibilitychange' event to the Camera app, the Camera app
> doesn't call stopPreview even when a phone is locked and results in battery
> drain. I'm reopening this bug..

Comment 108

[Jason Smith [:jsmith]]

Please don't reopen bugs for regressions from a patch. File followups and link to them to the bug.

Comment 109

[hanj.kim25]

As per our spec, we need to mute the device when the screen is put to sleep. (to reduce CPU drain) And this is the very opposite of what this patch does.

From the latest v1-train, I could put the following in the overlayEventHandler method and mute the device when the screen is put to sleep. 

   function overlayEventHandler(evt) {
     ...
     case 'mozChromeEvent':
        if (isRunningFirstRunApp)
          return;
     ...
   }

Does this look okay to you? or do you see any possible side-effects?


p.s. I didn't want to create a new bug just for this question. Hopefully the answer is simple. Let me know if you want a new bug though.

Thanks.

Comment 110

[(inactive after 6/18) Alive Kuo [:alive]]

You do need a new bug for any new issue.
I don't know what does this section of code means...isRunningFirstApp check should be removed in v1-train.

(In reply to hanj.kim25 from comment #109)
> As per our spec, we need to mute the device when the screen is put to sleep.
> (to reduce CPU drain) And this is the very opposite of what this patch does.

Even the user is playing music app? What's the spec detail?