Closed Bug 793105 Opened 12 years ago Closed 11 years ago

Add a third "visibility" class: not optically visible but still perceived by user

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
B2G C4 (2jan on)
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: cjones, Assigned: kanru)

References

Details

Attachments

(1 file, 1 obsolete file)

Add a "backgroundPerceived" class for audio-channel-content. v1.1
13.08 KB, patch
kanru
: review+
Details | Diff | Splinter Review 
Currently, we partition all apps into { foreground, background }.  We prioritize foreground above background for access to system resources.

There's a third class, though: not optically visible but has user-visible effects.  A background music player falls into this class.  Something like a background email poller may or may not fall into this class, open to debate (I would say "no").

There's value in us distinguishing "background-user-can-observer" apps from purely background apps.  We should try to keep the former alive in priority to the latter.

We also have to be careful though not to let apps play silent audio, e.g., just to bump their priority class.

This seems like a problem that could be solved with a permission bit, maybe, except to be useful (not annoy the user when they background their music player), it'd have to be granted ahead of time.
How would this be implemented? Would the app tell us if it's doing things that are user visible, like polling for email? Or would we somehow detect that the app is? If the latter, what logic would we use?
The lowmemkiller driver we use supports 16 configurable OOM levels.  I think stock android uses 8 or so.  That's not hard at all.

The hard part is determining the class.  There are two options, I think
 - try to infer user-perceivable.  <audio> in a playing state is user-perceivable, for example.  But then apps can abuse this (play silent audio) to improve their OOM score.
 - make it a permission bit.  This solves the email case and prevents abuse.
Apps telling us that they're user-perceivable is another option, but because of the potential for abuse, I think we'd want to put that under a permission bit too.
I think a permission bit plus an explicit "please let me stay awake" command is probably the only sane thing to do here.

Inferring "please keep me awake" by the app's actions only works insofar as we're sufficiently imaginative.  For example, we'd need to keep the app running if

  * audio is playing (for obvious reasons)
  * the microphone is on (for a voice recording app, say)
  * GPS is being recorded (for turn-by-turn navigation in the background)
  * accelerometers are active (for a pedometer app)

and so on.  There is probably a good use-case for keeping the app alive even when the app isn't actively doing anything.  For example, perhaps my run-tracker app doesn't run the GPS continuously, but polls once a minute or so.

It's hard for me to believe that we'll be sufficiently creative to imagine all possible reasons an app might want to be alive in the background.
This is sounding exactly like the wakeLock("cpu") feature we have been talking about.

Can the OOM level for a process be changes dynamically after the process has started?
(In reply to Jonas Sicking (:sicking) from comment #5)
> This is sounding exactly like the wakeLock("cpu") feature we have been
> talking about.

Yes.  :)

> Can the OOM level for a process be changes dynamically after the process has
> started?

Yes.  Currently, the OOM-priority (and CPU priority) change when the process enters/leaves the background.  Also, the root process is its own memory and CPU priority class.
(In reply to Justin Lebar [:jlebar] from comment #4)
> I think a permission bit plus an explicit "please let me stay awake" command
> is probably the only sane thing to do here.
> 

I don't disagree.

> It's hard for me to believe that we'll be sufficiently creative to imagine
> all possible reasons an app might want to be alive in the background.

Why?
(In reply to Jonas Sicking (:sicking) from comment #5)
> This is sounding exactly like the wakeLock("cpu") feature we have been
> talking about.
> 

Why?
>> It's hard for me to believe that we'll be sufficiently creative to imagine
>> all possible reasons an app might want to be alive in the background.
>
> Why?

Do you think you could have come up with the list of uses in comment 4?  And, how confident are you that it's exhaustive?  I'm not at all confident...

In general, an app might have lots of reasons for staying alive in the background, and I'm not confident I can enumerate all possible reasons.

>> This is sounding exactly like the wakeLock("cpu") feature we have been
>> talking about.
>
> Why?

I think Jonas is saying that he can't think of a reason that an application would want to be in this third visibility class without also keeping the CPU on while the device's screen is turned off.  I can't either.  If I don't care about keeping the CPU on, then I'm not doing any processing while I'm in the background, so why do I care if I'm alive?
(In reply to Justin Lebar [:jlebar] from comment #9)
> >> It's hard for me to believe that we'll be sufficiently creative to imagine
> >> all possible reasons an app might want to be alive in the background.
> >
> > Why?
> 
> Do you think you could have come up with the list of uses in comment 4? 
> And, how confident are you that it's exhaustive?  I'm not at all confident...
> 

I think we can asymptotically converge on the right list, yes.

> >> This is sounding exactly like the wakeLock("cpu") feature we have been
> >> talking about.
> >
> > Why?
> 
> I think Jonas is saying that he can't think of a reason that an application
> would want to be in this third visibility class without also keeping the CPU
> on while the device's screen is turned off.  I can't either.  If I don't
> care about keeping the CPU on, then I'm not doing any processing while I'm
> in the background, so why do I care if I'm alive?

This contradicts what you said in comment 4.
> This contradicts what you said in comment 4.

I don't see any contradiction, so we're probably not understanding each other properly.  Perhaps you could elaborate?

> I think we can asymptotically converge on the right list, yes.

But to be clear you're also OK with having an explicit "please keep me alive" command, per comment 7?  If so, we don't need to debate the feasibility of creating such a list.
(In reply to Justin Lebar [:jlebar] from comment #11)
> > This contradicts what you said in comment 4.
> 
> I don't see any contradiction, so we're probably not understanding each
> other properly.  Perhaps you could elaborate?
> 

From comment 4:

> There is probably a good use-case for keeping the app alive even when the app isn't actively doing anything.

I agree with that statement.  That's the use case of a background email poller, for example.

However, wakeLock("cpu") would imply locking the CPU out of low-power sleep, which is not what we want for that case.  It would kill battery.  And further, a background music app that's using a HW music decoder might barely even touch the CPU.

Wake locks don't require a permission bit so I don't think using them for this use case is appropriate.  But if we were to, I think what we mean is wakeLock('memory') is what we mean, since we're in effect trying as hard as we can to pin the page's RAM.

> > I think we can asymptotically converge on the right list, yes.
> 
> But to be clear you're also OK with having an explicit "please keep me
> alive" command, per comment 7?  If so, we don't need to debate the
> feasibility of creating such a list.

Yes.
> Wake locks don't require a permission bit so I don't think using them for this use case is 
> appropriate.

You don't need a permission bit to hold the screen awake?  I guess any app can do that, and webpages simply can't?  I guess that's OK...

Anyway, my intent with wake locks was that the CPU wake lock would have a permission bit associated with it.  We just never got around to implementing the CPU wake lock...

> There is probably a good use-case for keeping the app alive even when the app isn't actively doing 
> anything.

Ah, I see, yes, that's a contradiction...

How does polling in the background work right now?  Can I just do setTimeout, or do I need to register myself with some polling service?

In general, using the full app to poll seems kind of wasteful.  Given how little memory we have on these devices, waking up the app, letting it poll, and then killing it seems much better than simply letting it run arbitrarily (and giving up if there isn't enough RAM to keep alive all apps that want to poll).

> And further, a background music app that's using a HW music decoder might barely even touch the 
> CPU.

How would a background music app even know if it's appropriate to get the CPU wake lock, then?  It can't know a priori whether it's going to have a HW music decoder for a particular track, and whether the amount of CPU it needs can be met by the device's CPU while it's in low-power mode.

I'd have expected the CPU wake lock to keep the CPU from being /forced/ into a low-power mode, but to still allow the CPU to drop down into a low-power mode if it's not used.  If that's not the case, then maybe we need to re-think this at a higher level.
(In reply to Justin Lebar [:jlebar] from comment #13)
> > Wake locks don't require a permission bit so I don't think using them for this use case is 
> > appropriate.
> 
> You don't need a permission bit to hold the screen awake?  I guess any app
> can do that, and webpages simply can't?  I guess that's OK...
> 

Any arbitrary content can grab wake locks.

> Anyway, my intent with wake locks was that the CPU wake lock would have a
> permission bit associated with it.  We just never got around to implementing
> the CPU wake lock...
> 

Having some parts of an API require permissions while others don't was deemed Bad Style.

> How does polling in the background work right now?  Can I just do
> setTimeout, or do I need to register myself with some polling service?
> 

setTimeout() would work.  For an email poller on a long interval, the alarm API would probably be more appropriate since it will re-launch the poller if it gets killed.

> In general, using the full app to poll seems kind of wasteful.  Given how
> little memory we have on these devices, waking up the app, letting it poll,
> and then killing it seems much better than simply letting it run arbitrarily
> (and giving up if there isn't enough RAM to keep alive all apps that want to
> poll).
> 
> > And further, a background music app that's using a HW music decoder might barely even touch the 
> > CPU.
> 
> How would a background music app even know if it's appropriate to get the
> CPU wake lock, then?  It can't know a priori whether it's going to have a HW
> music decoder for a particular track, and whether the amount of CPU it needs
> can be met by the device's CPU while it's in low-power mode.
> 

I was arguing that CPU wake locks are inappropriate for this use case, so I don't know how to answer your question :).
>> Anyway, my intent with wake locks was that the CPU wake lock would have a
>> permission bit associated with it.  We just never got around to implementing
>> the CPU wake lock... 
>
> Having some parts of an API require permissions while others don't was deemed Bad Style.

Then let's add a permission for the screen wake lock.  That's easy enough.  There's no reason for an arbitrary webpage to be able to keep my screen on forever without prompting.

> I was arguing that CPU wake locks are inappropriate for this use case, so I don't know how to answer 
> your question :).

Well, CPU wake locks /may/ be inappropriate for this use-case, depending on whether HW-accelerated decoding is available for that particular track, and whether the CPU is capable of handling the music player's other CPU needs whilst in low-power mode, right?

So my question is, if the CPU wake lock is necessary only sometimes, how can the app tell?  And given that the answer probably is "it can't," I wonder if the details of the "CPU wake lock" that we've posited here aren't quite right.
First off, I want to make it clear that a "cpu" wakelock has not been implemented. At least that's the impression I had.

We have complete freedom in how we design the permissions around wakeLocks. For example right now the agreed upon model is that only content which is in fullscreen mode as well as installed application when grabbing the "screen" wakelock.

So we can definitely say that only installed applications have permission to grab the "cpu" wakelock.

Chris: I'm not entirely sure what behavior you had in mind for the "cpu" wakelock. The behavior I had envisioned was basically that holding the "cpu" wakelock means "don't shut my app down even if the user leaves the app".

It would not affect our ability to put the CPU to sleep between timeouts or other callbacks. So if the app is able to prime us with enough data that we can play audio without going out of low-power mode, then we can shut the CPU down even if the app is holding the "cpu" wakelock.

If you had some other behavior in mind for the "cpu" wakelock then I'm all ears.
> First off, I want to make it clear that a "cpu" wakelock has not been implemented.  At least that's 
> the impression I had.

Correct afaik.

> So we can definitely say that only installed applications have permission to grab the "cpu" wakelock.

That's not a permission I'd necessarily want to hand out to all installed apps.  But it's not clear what it means anymore, so I guess we'd need to get that sorted out.  :)
(In reply to Justin Lebar [:jlebar] from comment #15)
> So my question is, if the CPU wake lock is necessary only sometimes, how can
> the app tell?  And given that the answer probably is "it can't," I wonder if
> the details of the "CPU wake lock" that we've posited here aren't quite
> right.

Which use case do you have in mind?  This question as literally written is too unconstrained to answer.
(In reply to Jonas Sicking (:sicking) from comment #16)
> First off, I want to make it clear that a "cpu" wakelock has not been
> Chris: I'm not entirely sure what behavior you had in mind for the "cpu"
> wakelock. The behavior I had envisioned was basically that holding the "cpu"
> wakelock means "don't shut my app down even if the user leaves the app".
> 

I think we've all agreed that locking the cpu out of low-power sleep is not the behavior we want here, so I don't understand why we're continuing to discuss this proposal.

If we want to (ab)use wake locks for this API, which I don't have a strong opinion on, then as I said wakeLock("memory") matches the desired semantics more closely.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> (In reply to Jonas Sicking (:sicking) from comment #16)
> > First off, I want to make it clear that a "cpu" wakelock has not been
> > Chris: I'm not entirely sure what behavior you had in mind for the "cpu"
> > wakelock. The behavior I had envisioned was basically that holding the "cpu"
> > wakelock means "don't shut my app down even if the user leaves the app".
> > 
> 
> I think we've all agreed that locking the cpu out of low-power sleep is not
> the behavior we want here,

Agreed. But I don't think we ever want an API which locks the API out of low-power sleep mode. So I don't think wakeLock("cpu") 

> so I don't understand why we're continuing to
> discuss this proposal.

Please note that I wasn't asking "can we use wakeLock('cpu')". I was asking "what behavior do you envision wakeLock('cpu') to have".

Or, if you prefer "how do you enviosion wakeLock('cpu') to be different from what we need here".
(In reply to Jonas Sicking (:sicking) from comment #20)
> Please note that I wasn't asking "can we use wakeLock('cpu')". I was asking
> "what behavior do you envision wakeLock('cpu') to have".
> 

Wake-locking the CPU is an established paradigm that means "prevent the CPU from entering low-power sleep".  I don't think it's a good idea to choose a different behavior than that for wakeLock("cpu"), since there's a very good chance that that would confuse developers.

> Or, if you prefer "how do you enviosion wakeLock('cpu') to be different from
> what we need here".

What all the discussion here boils down to is pinning an app in RAM when we otherwise would not pin it.  That doesn't have anything to do with the CPU.  I understand what your proposal is aiming to do, but I think being loose about the terminology is going to be confusing and harmful in the long term.
Another strawman API could be mozApp.requestPin() or something to that effect.  But as I said, I don't have a strong opinion on the API.
What I don't understand is, if "wakeLock('cpu')" is the wrong name for the thing that a music player need to do, then

 a) Is there anything which /does/ need "wakeLock('cpu')"?
 b) If not, what makes our platform special in comparison to other platforms in that ours doesn't need wakeLock('cpu')?

Or alternatively, is it the case that a music player might need the CPU wake lock, under some circumstances (HW-decoding not available for this track or at all, or CPU in low-power state is not fast enough for the app's needs)?  If that's the case, how would you expect the music player to know whether to acquire the CPU wake lock?

> Wake-locking the CPU is an established paradigm that means "prevent the CPU from entering 
> low-power sleep".

I thought that the CPU wake lock meant something slightly different.

I thought these other platforms would forcibly quiesce all app processes when the screen turned off (and maybe when the app is backgrounded).  This incidentally allows the CPU to enter a mostly low-power state, because the OS has nothing to schedule most of the time.

But if I don't want my app to be forcibly paused when the screen turns off or when the app is backgrounded, I need to acquire the CPU lock.  I need to do this irrespective of whether I want my app to use 5% or 100% of the CPU while the device's screen is off.

If this interpretation is correct, then surely the CPU lock is the right thing for a music player, even if it doesn't need a lot of CPU resources.  Even if this interpretation is not what other platforms do, would you agree that

 a) It's sufficient for us, and
 b) It's sufficiently similar to prior art so as not to be unduly confusing?
(In reply to Justin Lebar [:jlebar] from comment #23)
> What I don't understand is, if "wakeLock('cpu')" is the wrong name for the
> thing that a music player need to do, then
> 
>  a) Is there anything which /does/ need "wakeLock('cpu')"?

We haven't come across a use case in b2g yet.

>  b) If not, what makes our platform special in comparison to other platforms
> in that ours doesn't need wakeLock('cpu')?
> 

I don't think this is relevant to the discussion at hand.  Lots of platforms expose junk that no one needs or uses.  The Web (TM) exposes a bunch of crap that's irrelevant on iOS, for example.

We haven't exposed an interface to lock the CPU out of low power sleep because we haven't needed it.  However, there are several libraries at the gonk level that use CPU wake locks to achieve goals that appear higher-level to web content (like fast event delivery).

> Or alternatively, is it the case that a music player might need the CPU wake
> lock, under some circumstances (HW-decoding not available for this track or
> at all, or CPU in low-power state is not fast enough for the app's needs)? 
> If that's the case, how would you expect the music player to know whether to
> acquire the CPU wake lock?
> 

Can we just say that a music player shouldn't ever try to lock the CPU out of low power sleep and move on? :)

> > Wake-locking the CPU is an established paradigm that means "prevent the CPU from entering 
> > low-power sleep".
> 
> I thought that the CPU wake lock meant something slightly different.
> 
> I thought these other platforms would forcibly quiesce all app processes
> when the screen turned off (and maybe when the app is backgrounded).  This
> incidentally allows the CPU to enter a mostly low-power state, because the
> OS has nothing to schedule most of the time.
> 

For which platform have you seen "CPU wake lock" refer to this behavior?  I'm not trying to be argumentative, I honestly don't know.

FWIW, Meego does something like this with background processes (SIGSTOPs them).  I don't know if it exposes an API to prevent that.  It probably does.

> But if I don't want my app to be forcibly paused when the screen turns off
> or when the app is backgrounded, I need to acquire the CPU lock.  I need to
> do this irrespective of whether I want my app to use 5% or 100% of the CPU
> while the device's screen is off.
> 
> If this interpretation is correct, then surely the CPU lock is the right
> thing for a music player, even if it doesn't need a lot of CPU resources. 
> Even if this interpretation is not what other platforms do, would you agree
> that
> 
>  a) It's sufficient for us, and
>  b) It's sufficiently similar to prior art so as not to be unduly confusing?

Do you agree that the goal of this bug is to pin apps in RAM when we otherwise would not currently?  If so, then I think we should have the discussion about a separate API to prevent freezing in another bug; probably best would be the eager-bfcache one.
Just like you felt strongly that we should not expose the concept of "crash" to web apps, I am not convinced that we should expose the concept of "pin in RAM" to web apps.

Instead, it seems to me that the concept of "please allow me to run in the background, instead of SIGSTOP'ing me" is the correct abstraction.  That's why I've been focused on the "CPU lock" here.  I hope you don't feel like that's a distraction.
> For which platform have you seen "CPU wake lock" refer to this behavior?  I'm not trying to be 
> argumentative, I honestly don't know.

That's how I read the Android docs.  But I admit it's a matter of reading between the lines.

There is no reason for exposing an API which says "prevent the CPU's frequency scaling from reducing the CPU frequency."  The CPU can adjust its frequency within a few microseconds.  So it seems to me that my interpretation here is at least plausible.  To say otherwise presumes that the designers of the Android API added a useless method, and that many app developers request permission to call this useless method.  Occam's razor suggests this is perhaps not the best assumption.
(In reply to Justin Lebar [:jlebar] from comment #25)
> Just like you felt strongly that we should not expose the concept of "crash"
> to web apps, I am not convinced that we should expose the concept of "pin in
> RAM" to web apps.
> 

I was against that because I don't want to expose the concept of OS processes to web content.  Processes don't consistently correspond to anything in any "Web APIs".  Adding them doesn't gain us anything.

However, every computer ever built or will be built has RAM, and web pages must be in RAM to function.  Every web browser engine has some notion of cache that pages can be in and can be evicted from.

> Instead, it seems to me that the concept of "please allow me to run in the
> background, instead of SIGSTOP'ing me" is the correct abstraction.  That's
> why I've been focused on the "CPU lock" here.  I hope you don't feel like
> that's a distraction.

But the goal of this bug is not to SIGSTOP pages in the background.

If it's simpler and makes more sense to everyone, we could add a single API that means "let me keep running in the background and don't evict me from RAM".  Fine by me.
(In reply to Justin Lebar [:jlebar] from comment #26)
> There is no reason for exposing an API which says "prevent the CPU's
> frequency scaling from reducing the CPU frequency."

There is, actually!  Talk to azakai and vlad about BananaBread.

>  So it seems to me that my
> interpretation here is at least plausible.  To say otherwise presumes that
> the designers of the Android API added a useless method, and that many app
> developers request permission to call this useless method.  Occam's razor
> suggests this is perhaps not the best assumption.

It's entirely possible that android apps have a use case for locking the CPU out of low power sleep and gaia doesn't, yet.  I'm not necessarily accusing android folks of being sloppy.  That is a possibility, though.
CPU wake lock is implemented in gaia. The use case is for apps like Music, Maps, which don't require the screen but need the CPU. Even the Music app is mostly idle because decoding is using dedicated hardware, we still need the main CPU to do some processing.
What does the gaia implementation do?
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/screen_manager.js#L85

Allow the cpu going to low-power mode if no "cpu" wake lock is hold.
I don't understand why the background music player needs that wake lock.  The tasks it enqueues should wake the cpu up out of low-power sleep.
Do you mean the kernel space wake lock? If the driver uses such wake lock then yes, the cpu will keep awake. Imagine you are playing music on your desktop, it wouldn't prevent you to 'echo mem > /sys/power/state' and suspend the whole system.
When the music app needs to use the CPU, once in a while, it should enqueue events to process more data.  Those events should result in CPU interrupts, and those interrupts should wake up the CPU from low-power sleep.  Right?

CPU wake locks are useful for ensuring the CPU doesn't accidentally go into low-power sleep while there's interesting work being handed off between TCBs where low processing latency is needed, and then wasting time transitioning in and out of low-power mode.  I don't think that accurately describes the music player; it wakes up relatively infrequently to process large batches of work that doesn't need low-latency hand-off between TCBs.  Am I misunderstanding something?
Ah. I understand what you meant "low-power sleep", the cpufreq and cpuidle driver will handle that. I don't think we have implemented any method to change the default cpufreq governor. (Our default governor is performance now, change that to ondemand might save a little power)

 root@android:/sys/devices/system/cpu/cpu0/cpufreq # cat scaling_available_freq
 122880 245760 320000 480000 600000 800000

However the cpu wake lock is different here. All wake locks are about the on/off state, so a wifi lock prevent the wifi being turned off, a cpu wake lock prevent the cpu (and the whole system) being suspended.
No, by "low-power sleep" I meant "lightweight suspend".

Let me ask my question a different way: what interrupts will wake the CPU up out of lightweight suspend?  Does the answer change depending on whether the screen is on or off?  Does the screen being on take an implicit lightweight-suspend lock?
Do you mean the C-states? That's why I mentioned cpuidle, because it manages the states. I think it depends on the governor and which part of the CPU is used and the screen being on won't change that.
I don't think we're quite on the same page :).  I'll try to catch up with you on IRC later.
OK, the part I was missing here is that timer normally used by the kernel on ARM is turned off along with suspend.  That sort of makes sense.  /dev/alarm programs an external chip (RTC).

So we absolutely have use cases in gaia for the traditional CPU wake lock, and already have it in place and implemented :).

So we need to find another mechanism to implement the use case here.
> and already have it in place and implemented :).

Much to both of our surprise!  And only pages which have the "power" permission can get navigator.mozPower.

> So we need to find another mechanism to implement the use case here.

Now that we understand what the CPU wake lock does, it's still not clear to me that it would be inappropriate to make holding the CPU wake lock implicitly stick you into this third visibility class.  Would you mind elaborating on this?

The only reason you'd hold the CPU wake lock is because you want to keep the CPU out of suspend.  And that means you should be doing something user-visible while you're in the background, no?

(Obviously if the root process holds one of these wake locks, it stays in the "root" visibility class; you don't get downgraded, only upgraded.)
No, I agree with this proposal now.  The way things work currently is
 - locking the screen on also locks the CPU out of suspend
 - normal timer events don't wake the CPU out of suspend; only /dev/alarm and a handful of interrupts that regular content can't affect

So for the use case of "frequently user perceivable", like music, it needs to hold a CPU wake lock in case the screen goes off.

The case of "intermittently user perceivable", like email polling, should use the alarm API.  It doesn't have to be locked in RAM because the alarm wakeup will re-launch if needed.  (Similarly for sms/telephony on incoming events, which generate interrupts that wake the CPU out of suspend.)  We need to ensure these guys are foregrounded or get the user-perceivable lowmemkiller class, though.

My only remaining concern is that we're re-using the "power" permission for CPU wake locks, which we also use for poweroff/reboot.  I don't think it's appropriate to hand that power to a music player just so it can play in the background.  But currently we ban remote content from reboot/poweroff, so maybe this is acceptable for v1.

It probably makes sense to hand out the screen lock and CPU lock permissions as a set.
> We need to ensure these guys are foregrounded or get the user-perceivable lowmemkiller class, though.

To be clear, you mean, we need to ensure that background tasks running via an alarm are not running with pure-background priority?  I totally agree with that.

> My only remaining concern is that we're re-using the "power" permission for CPU wake locks [and for 
> reboot/shut down].

We should just fix this.  I'd prefer if we could also separate out the screen lock and CPU lock permissions, because there's no reason my Instapaper app needs permission to burn the CPU in the background, but that's kind of tricky with the way I designed the API, so we I'm happy to punt on it for now.
(In reply to Justin Lebar [:jlebar] from comment #42)
> > We need to ensure these guys are foregrounded or get the user-perceivable lowmemkiller class, though.
> 
> To be clear, you mean, we need to ensure that background tasks running via
> an alarm are not running with pure-background priority?  I totally agree
> with that.
> 

Correct, but any app/service launched from a system message that doesn't need foreground UI.
Thinking about how to implement this bug, now that we agree on what to do:

When we designed the wake lock API, we believed it was important to push as much logic down into Gaia as possible.  Therefore none of the wake lock names mean anything to Gecko.  Instead, Gecko merely acts as the lock coordinator, and informs Gaia when a wake lock is acquired or released.  Gaia then uses that information to e.g. disable the screen-off timeout.

I think this is still sensible in the case of the screen-off timeout, since that is and should be controlled entirely by Gaia.

But making that same mechanism work for this bug seems pretty difficult.  If two apps both acquire the CPU wake lock, we want both of them to get the new BACKGROUND_ACTIVE priority class.  But Gaia currently won't even get a notification about the second app grabbing the CPU lock.  And anyway, Gaia doesn't even know which process grabbed the first lock, nor does it even have a sane way to /refer/ to a process.

ISTM that the simplest thing to do here is to declare that some wake locks are handled by Gaia (e.g. the screen wake lock) and others are handled by Gecko (e.g. the CPU wake lock).
(In reply to Chris Jones [:cjones] [:warhammer] from comment #41)
> The case of "intermittently user perceivable", like email polling, should
> use the alarm API.  It doesn't have to be locked in RAM because the alarm
> wakeup will re-launch if needed.  (Similarly for sms/telephony on incoming
> events, which generate interrupts that wake the CPU out of suspend.)

There's another, subtler problem here that I wasn't able to get an answer to yesterday: these apps also need to hold CPU wake locks after waking up, until they've finished their business.  I don't know how we ensure that for even the gaia system app processing a power-button event.
> these apps also need to hold CPU wake locks after waking up, until they've finished their business.  
> I don't know how we ensure that for even the gaia system app processing a power-button event.

Or Gaia needs to hold the CPU lock on behalf of the app, and enforce some limit on how long it is willing to keep the device awake.
One way to handle this would be to do the permissions checks in Gecko. I.e. gecko can check if "wakelock:<lockname>" permission is given to an app when the app calls requestWakeLock("<lockname>") is called and only if that is allowed per the permission manager database do we forward the call to gaia.

Populating the permission manager database would be done at installtime as with all permissions.

We can still allow gaia to decide if it wants to honor only requests from fullscreened apps etc though.
This doesn't sound like something we'd block v.1 on.  Noming to be sure.
blocking-basecamp: --- → ?
I don't think we have enough information to know yet.
Can we change this bug into something related to getting the information we need to decide on a course of action?  I think we could block on that so we don't forget about this in case it becomes important.
We need to hammer on the device with lots of activities and doing things like playing music in the background while churning a lot of other programs.  I fear we're only going to get that kind of beating through dogfooding.

Also, activities are too buggy otherwise right now for that kind of testing to be meaningful.
Let's re-nom if and when this becomes a problem.
blocking-basecamp: ? → ---
Blocks a blocker.

jlebar, do you have cycles to add this?
Blocks: 815355
No longer blocks: b2g-e10s-work
blocking-basecamp: --- → ?
Did we ever decide how we're going to decide what's in this visibility class?
I think we tentatively decided hasWakeLock("cpu").
(In reply to Chris Jones [:cjones] [:warhammer] from comment #55)
> I think we tentatively decided hasWakeLock("cpu").

Okay.  Kan-Ru, do you have cycles to take this bug?  I can do it if you don't, but you understand the wake locks better than anyone.
Flags: needinfo?(kchen)
For what it's worth, the way that background apps "opt in" to not getting muted when the user leaves the app is by using the "content" audio channel.

Channels other than the "content" channel is automatically muted when the user leaves the app, but the "content" channel is not. (There are other rules governing the "content" channel, which do things like ensure that two music apps aren't playing music at the same time).

So no "audio" wake-locks are involved.

The platform is aware of which apps uses which channels, but we're currently not surfacing that through any APIs visible to any apps.

I don't have strong opinions of simply using the "content" channel should affect the app's OOM class. We could require that the app also hold a "cpu" wakelock if it wants to not get killed.
Are audio clients notified when their channel is "active"?
yes, apps are notifoes when their channel is being actively played and wgen it is paused. The one exception is that we don't yet notify that for users of the FMRadio API. That could be fixed of course, but isn't currently the case.
Flags: needinfo?(kchen)
That sounds like exactly what we want then.
What should we do now?
If an app process has an active audio stream and it's in the background, it should be perceivable.

We can decide what to do with cpu wake locks separately.
Assignee: nobody → kchen
blocking-basecamp: ? → +
Priority: -- → P2
Target Milestone: --- → B2G C3 (12dec-1jan)
If the solution that's been discussed involves a substantial amount of work, instead of implementing a general purpose solution at this point in the schedule, is there a quick and dirty way to keep the music and home screen (any other?) apps running in the background for v1?
The level of effort would be approximately the same.
I'm going to attach patches for bug 821440 soon; it's relatively simple, and I think this bug should not be a lot more complex than that one.
Ok, bug 821440 has landed, can we clean this up?  We have another blocker (bug 815355) open because of it.
> can we clean this up?

Do you mean, can we write a patch for this?  It might make sense to wait for me to come back from PTO instead of pulling cjones off his other work.  But if kanru or someone else can write a patch, I'll try to review it.

> We have another blocker (bug 815355) open because of it.

These two bugs are basically identical; I would not use the fact that there are two bugs here instead of one to increase the urgency of this bug.
This is one my list.
Kan-ru is going to submit another patch today.

        Attachment #695455 -
        Flags: review?(justin.lebar+bug)

    
    

    
  
Comment on attachment 695455 [details] [diff] [review]
Add a "backgroundPerceived" class for audio-channel-content. v1

Try roc for the AudioChannelAgent part.

        Attachment #695455 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 695455 [details] [diff] [review]
Add a "backgroundPerceived" class for audio-channel-content. v1

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

::: dom/audiochannel/AudioChannelServiceChild.h
@@ +37,5 @@
>     * Return true if this type + this mozHidden should be muted.
>     */
>    virtual bool GetMuted(AudioChannelType aType, bool aMozHidden);
>  
> +  bool IsContentChannelActive();

Document this

        Attachment #695455 -
        Flags: review?(roc) → review+

    
    

    
  
Comment on attachment 695455 [details] [diff] [review]
Add a "backgroundPerceived" class for audio-channel-content. v1

r=me with some nits and a question about the audio channel API addressed.

>diff --git a/b2g/app/b2g.js b/b2g/app/b2g.js
>--- a/b2g/app/b2g.js
>+++ b/b2g/app/b2g.js

>@@ -549,16 +549,18 @@ pref("dom.ipc.processPriorityManager.ena
>+pref("hal.processPriorityManager.gonk.backgroundPerceivedOomScoreAdjust", 100);
>+pref("hal.processPriorityManager.gonk.backgroundPerceivedKillUnderMB", 5);

Ideally this oom_score_adj would be an integral oom_adj.  The formula (from bug
821440 comment 23) is

  oom_adj = (oom_score_adj * 15) / 1000.

So oom_score_adj == 100 isn't an integral oom_adj.  134 should work, though.
(It's not /exactly/ an integer oom_adj, but neither is 67.)

>diff --git a/dom/audiochannel/AudioChannelServiceChild.cpp b/dom/audiochannel/AudioChannelServiceChild.cpp
>--- a/dom/audiochannel/AudioChannelServiceChild.cpp
>+++ b/dom/audiochannel/AudioChannelServiceChild.cpp

>@@ -77,26 +74,37 @@ AudioChannelServiceChild::RegisterAudioC
>                                                AudioChannelType aType)
> {
>   AudioChannelService::RegisterAudioChannelAgent(aAgent, aType);
> 
>   ContentChild *cc = ContentChild::GetSingleton();
>   if (cc) {
>     cc->SendAudioChannelRegisterType(aType);
>   }
>+
>+  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>+  obs->NotifyObservers(nullptr, "audio-channel-agent-changed", nullptr);
> }

obs can be null if we call this after shutdown.  Given how many bugs we've had
where stuff breaks when we do it after shutdown, I think we should probably
have a null-check here and in UnregisterAudioChannelAgent.

>+bool
>+AudioChannelServiceChild::IsContentChannelActive()
>+{
>+  return mChannelCounters[AUDIO_CHANNEL_CONTENT] > 0;
>+}

Nit: ContentChannelIsActive() would be a better name, IMO.

I don't understand why this method must live on AudioChannelServiceChild.  Why
can't it live on AudioChannelService?  Then we wouldn't need the static cast in
ProcessPriorityManager.

>diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp
>--- a/dom/ipc/ProcessPriorityManager.cpp
>+++ b/dom/ipc/ProcessPriorityManager.cpp

>@@ -71,16 +72,26 @@ GetPPMLog()
> #endif
> 
> /**
>  * Get the appropriate backround priority for this process.
>  */
> ProcessPriority
> GetBackgroundPriority()
> {
>+  bool isPerceived = false;
>+
>+  AudioChannelServiceChild* service =
>+    static_cast<AudioChannelServiceChild*>(AudioChannelServiceChild::GetAudioChannelService());
>+  isPerceived = service->IsContentChannelActive();
>+
>+  if (isPerceived) {
>+    return PROCESS_PRIORITY_BACKGROUND_PERCEIVED;
>+  }

Nit: Please don't use this extra variable; just do

    if (service->IsContentChannelActive()) {
      return ...;
    }

> void
>+ProcessPriorityManager::OnAudioChannelAgentChanged()
>+{
>+  if (mProcessPriority != PROCESS_PRIORITY_BACKGROUND &&
>+      mProcessPriority != PROCESS_PRIORITY_BACKGROUND_HOMESCREEN &&
>+      mProcessPriority != PROCESS_PRIORITY_BACKGROUND_PERCEIVED) {

Nit: We now have this if statement in three places; can you please factor it
out as an IsBackgroundPriority(aPriority) function?

>+    return;
>+  }
>+  SetPriority(GetBackgroundPriority());
>+}

Nit: I might write this as

  if (IsBackgroundPriority(mPriority)) {
    SetPriority(GetBackgroundPriority());
  }

(The early-return idiom is useful if the thing you're not doing is long, but
here it's just one line.)

>diff --git a/hal/HalTypes.h b/hal/HalTypes.h
>--- a/hal/HalTypes.h
>+++ b/hal/HalTypes.h

>@@ -65,16 +65,17 @@ enum SwitchState {
>   NUM_SWITCH_STATE
> };
> 
> typedef Observer<SwitchEvent> SwitchObserver;
> 
> enum ProcessPriority {
>   PROCESS_PRIORITY_BACKGROUND,
>   PROCESS_PRIORITY_BACKGROUND_HOMESCREEN,
>+  PROCESS_PRIORITY_BACKGROUND_PERCEIVED,
>   PROCESS_PRIORITY_FOREGROUND,
>   PROCESS_PRIORITY_MASTER,
>   NUM_PROCESS_PRIORITY
> };

I think I'd prefer that we called this BACKGROUND_PERCEIVABLE.  PERCEIVED
implies that we know the user is sitting there listening to music (or
whatever), when what we're actually saying is that the user /might/ be doing
so.

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp
>--- a/hal/gonk/GonkHal.cpp
>+++ b/hal/gonk/GonkHal.cpp

>@@ -1042,17 +1042,17 @@ EnsureKernelLowMemKillerParamsSet()
>   // we'll get notified when there are fewer than Z pages of memory free.  (See
>   // GonkMemoryPressureMonitoring.cpp.)
> 
>   // Build the adj and minfree strings.
>   nsAutoCString adjParams;
>   nsAutoCString minfreeParams;
> 
>   const char* priorityClasses[] =
>-    {"master", "foreground", "background", "backgroundHomescreen"};
>+    {"master", "foreground", "background", "backgroundHomescreen", "backgroundPerceived"};

Nit: Please wrap this line.

        Attachment #695455 -
        Flags: review?(justin.lebar+bug) → review+

    
  
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)

    
    

    
  
(In reply to Justin Lebar [:jlebar] from comment #75)
> I don't understand why this method must live on AudioChannelServiceChild. 
> Why can't it live on AudioChannelService?  Then we wouldn't need the static cast
> in ProcessPriorityManager.

It could, but the number returned will have different meaning on the parent side and child side. On the parent side it is global (if there any content channel alive). It might cause a surprise if later we use this function in other place.

> > ProcessPriority
> > GetBackgroundPriority()
> > {
> >+  bool isPerceived = false;
> >+
> >+  AudioChannelServiceChild* service =
> >+    static_cast<AudioChannelServiceChild*>(AudioChannelServiceChild::GetAudioChannelService());
> >+  isPerceived = service->IsContentChannelActive();
> >+
> >+  if (isPerceived) {
> >+    return PROCESS_PRIORITY_BACKGROUND_PERCEIVED;
> >+  }
> 
> Nit: Please don't use this extra variable; just do
> 
>     if (service->IsContentChannelActive()) {
>       return ...;
>     }

I wanted to leave a place that we could plug other metrics into the isPerceived calculation. But either way is fine with me.

    
    

    
  
Is this ready for checkin now, Kan-Ru, or are you waiting for clarifications on comment #76?

    
    

    
  
>  or are you waiting for clarifications on comment #76?

I explicitly asked for us to figure out the first issue from comment 76 ("r=me with a question about the audio channel API addressed"), so I would like us to discuss it a bit more, if that's OK.

> It could, but the number returned will have different meaning on the parent side and child side. On 
> the parent side it is global (if there any content channel alive).

Although it's not ideal, I think that would be a better API than what we currently have, because the static cast is unsafe if we're not in a child process.  So for example the correctness of this patch relies on the fact that we don't run the process priority manager in the parent; if we did, then GetBackgroundPriority() would cause us to crash in the parent process.

Roc, what do you think about exposing IsContentChannelActive on both the parent and child (where the meaning is "is a content channel active in this process or one of its subprocesses?).
Flags: needinfo?(roc)

    
    

    
  
> I wanted to leave a place that we could plug other metrics into the isPerceived calculation. But 
> either way is fine with me.

I usually say You Ain't Gonna Need It, but whatever you prefer is fine with me.

    
    

    
  
(In reply to Justin Lebar [:jlebar] from comment #78)
> Roc, what do you think about exposing IsContentChannelActive on both the
> parent and child (where the meaning is "is a content channel active in this
> process or one of its subprocesses?).

Sounds reasonable.
Flags: needinfo?(roc)

    
    

    
  


  
Comments addressed.

        Attachment #695455 -
        Attachment is obsolete: true

        Attachment #698219 -
        Flags: review+

    
    

    
  
aurora and b2g18 will land after bug 823610

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/dcd2c2de5e65
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: