Closed Bug 1181073 Opened 9 years ago Closed 8 years ago

Relax the setTimeout throttling for background tabs that are playing something through Web Audio

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: sole, Assigned: nika)

References

(Depends on 2 open bugs)

Details

(Keywords: dev-doc-complete, DevAdvocacy, Whiteboard: [DevRel:P2])

Attachments

(1 file, 1 obsolete file)

Relax setTimeout throttling for background tabs using web audio
6.00 KB, patch
Details | Diff | Splinter Review 
Right now as browsers try to save battery / resources as much as possible, the events for a site are throttled when it goes to the background. This is generally fine for "document based" sites, but not so fine when you are running music based apps that you actually want to keep running in the background.

The recommended way to schedule events for complex music arrangements in Web Audio is to use a combination of scheduling Audio Param changes with setTimeout as described on this document which is pretty much "canon" already in Web Audio literature:
http://www.html5rocks.com/en/tutorials/audio/scheduling/

This allows for generative scores, i.e. not the whole song is known when the first schedule is executed, and each time setTimeout runs the 'schedule' function, a new "slice" of audio events are queued.

But if this is used on a tab that goes to the background, the scheduling breaks and the music sounds "broken" as the setTimeout events are throttled and not run as fast as they should.

A solution for this problem would be to have some sort of API that lets the user tell the browser that they need maximum priority, so the timeouts and intervals are never throttled.
I don't know much about what we do with throttling at the moment, but in theory the browser could detect that web audio is in use in a tab and just disable throttling in that case.  That would avoid the need for a new API.
> a new "slice" of audio events are queued.

As long as that slice includes at least a second of audio that should work fine in a background tab as well, right?  Basically a background tab just needs a deeper buffer because of the lower rate of callbacks.

But yes, comment 1 is a viable alternative as well.  Having the _user_ (the actual user, not the website) messing with tab priorities seems weird; I know very few users who would even understand what that means.
This kind of automatic prioritization is what is done on b2g.  Its easier there, though, because we can just nice the entire process for the app/tab.
(Let's see if I know how to reference comments in bugzilla)

To Boris in comment 2 - no, I meant the *developer* be able to specify it.
> To Boris in comment 2 - no, I meant the *developer* be able to specify it.

What would keep people from just always specifying it, bringing us back to the situation we introduced clamping for in the first place?
Yeah, that's right.

I think I like the automatic prioritisation, but not sure if it happens automatically in b2g without having to specify 'magic' settings on the manifest.webapp file. Last time I checked there were some proprietary things you needed to add somewhere to get the audio to keep playing when the app went to the background, but I think I'm confusing two things here.

Anyway back to the point here: if there was a way in which the browser noticed we were using an AudioContext (not Offline) and let us keep a good priority, that would be amazing. Feasible?
So, something occurred to me.  The use case here is specific to Web Audio.  And we can tell whether the page is using Web Audio to playback something.  How would you feel about relaxing the setTimeout clamping for such pages when they go in the background?
Component: General → DOM
Flags: needinfo?(bzbarsky)
Yes, that's what comment 1 suggested, no?  I'm fine with that; see comment 2.
Flags: needinfo?(bzbarsky)
Right, sorry I missed that!
Summary: Provide an API to let the browser give maximum priority to a window/tab running Web Audio → Relax the setTimeout throttling for background tabs that are playing something through Web Audio
I've just seen this proposed API: https://w3c.github.io/requestidlecallback/
which goes "in the other way" if my understanding is correct.

What do you think of this one extension?
I don't think doing things when the browser is idle is the right approach for scheduling audio events.  That will probably be as unreliable as setTimeout currently is.
Whiteboard: [DevRel:P2]
Assignee: nobody → michael
Comment on attachment 8749822 [details] [diff] [review]
Relax setTimeout throttling for background tabs using web audio

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

LGTM, but I think we might want to tweak the timeout values in the test a bit to reduce the possibility of intermittents.

Soledad, does this do what you want?  Note that the setTimeout() throttling is only eased while audio is actually playing.  So if you do a setTimeout() before your first audio slice it will be delayed up to a second.  Does that work for you?

::: dom/media/webaudio/test/browser_bug1181073.js
@@ +36,5 @@
> +      }, 0);
> +    });
> +  });
> +
> +  ok(time < 500, "Interval is not throttled with audio playing (" + time + " ms)");

I'm worried the timing is to fine grained here for debug builds running on slow automation instances.  Can you bump up dom.min_background_timeout_value preference for this test?  Maybe something like 3 seconds.  Then we can make this check ok(time < 1000) and the other check something like ok(time > 2000).
Attachment #8749822 - Flags: review?(bkelly)
Attachment #8749822 - Flags: review+
Attachment #8749822 - Flags: feedback?(sole)
Comment on attachment 8749822 [details] [diff] [review]
Relax setTimeout throttling for background tabs using web audio

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

Sorry for the drive by review, a couple comments below.

::: dom/base/nsGlobalWindow.cpp
@@ +310,5 @@
> +  if (isBackground) {
> +    // Don't use the background timeout value when there are audio contexts with
> +    // active nodes, so that background audio can keep running smoothly.
> +    for (const AudioContext* ctx : mAudioContexts) {
> +      if (ctx->ActiveNodeCount() > 0) {

This should maybe also check that the context is not suspended, otherwise you can quickly create an AudioContext, suspend it so that it releases its audio thread, and let it sit while you're doing crazy things in the background tab. Relevant code in AudioContext::{Suspend,Resume,Close}.

You also have a way to know if it's emitting actual sound (DestinatinonNodeEngine::mLastInputMuted, also with events). Of course, you can trick it by playing a very very quiet sound (below the noise floor of the hardware), so that the input is not strictly silent, but you can't hear anything.
(In reply to Ben Kelly [:bkelly] from comment #13)
> Comment on attachment 8749822 [details] [diff] [review]
> Relax setTimeout throttling for background tabs using web audio
> 
> Review of attachment 8749822 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, but I think we might want to tweak the timeout values in the test a
> bit to reduce the possibility of intermittents.
> 
> Soledad, does this do what you want?  Note that the setTimeout() throttling
> is only eased while audio is actually playing.  So if you do a setTimeout()
> before your first audio slice it will be delayed up to a second.  Does that
> work for you?

Starting the audio, and then instantly afterwords calling setTimeout() should work fine as well, as the flags will be set then.

> ::: dom/media/webaudio/test/browser_bug1181073.js
> @@ +36,5 @@
> > +      }, 0);
> > +    });
> > +  });
> > +
> > +  ok(time < 500, "Interval is not throttled with audio playing (" + time + " ms)");
> 
> I'm worried the timing is to fine grained here for debug builds running on
> slow automation instances.  Can you bump up dom.min_background_timeout_value
> preference for this test?  Maybe something like 3 seconds.  Then we can make
> this check ok(time < 1000) and the other check something like ok(time >
> 2000).

It would have to be fairly slow for 1s to be mistaken with 4ms. 
I'll update it to be 3 seconds though, It'll be even less likely to flake, and should only cost a couple of seconds.
(In reply to Paul Adenot (:padenot) from comment #14)
> Comment on attachment 8749822 [details] [diff] [review]
> Relax setTimeout throttling for background tabs using web audio
> 
> Review of attachment 8749822 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the drive by review, a couple comments below.
> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ +310,5 @@
> > +  if (isBackground) {
> > +    // Don't use the background timeout value when there are audio contexts with
> > +    // active nodes, so that background audio can keep running smoothly.
> > +    for (const AudioContext* ctx : mAudioContexts) {
> > +      if (ctx->ActiveNodeCount() > 0) {
> 
> This should maybe also check that the context is not suspended, otherwise
> you can quickly create an AudioContext, suspend it so that it releases its
> audio thread, and let it sit while you're doing crazy things in the
> background tab. Relevant code in AudioContext::{Suspend,Resume,Close}.
> 
> You also have a way to know if it's emitting actual sound
> (DestinatinonNodeEngine::mLastInputMuted, also with events). Of course, you
> can trick it by playing a very very quiet sound (below the noise floor of
> the hardware), so that the input is not strictly silent, but you can't hear
> anything.

This patch isn't going to stop malicious pages from pegging the event loop. You can already peg the event loop using other techniques, even when the window is in the background. The setTimeout throttling is primarily intended to prevent websites which waste cycles out of ignorance rather than malice, and the fact that you can use AudioContext to get 4ms timeouts really shouldn't matter I would think.
New version with 3s timeouts for background tabs
Attachment #8749822 - Attachment is obsolete: true
Attachment #8749822 - Flags: feedback?(sole)
Comment on attachment 8753078 [details] [diff] [review]
Relax setTimeout throttling for background tabs using web audio

Re-adding the feedback which I apparently accidentally cleared :S
Attachment #8753078 - Flags: feedback?(sole)
How can I test this? Is it supposed to be in Nightly? How can I know?
(In reply to Soledad Penades [:sole] [:spenades] from comment #20)
> How can I test this? Is it supposed to be in Nightly? How can I know?

If Michael has a try build he could provide a link to the binary it generated.  Otherwise I think it should be in tomorrow's nightly build.
(In reply to Wes Kocher (:KWierso) from comment #23)
> The test this added is failing frequently on osx:
> https://treeherder.mozilla.org/logviewer.html#?job_id=28253857&repo=mozilla-
> inbound
> 
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a32fdbf877e0

It seems to me like invoking the GC to destroy the oscillator is unreliable. I'm tempted to cut that part from the test, 'cause I'm not sure how else it would be tested. Would that be acceptable to you bkelly?
Flags: needinfo?(michael) → needinfo?(bkelly)
Try using SpecialPowers.exactGC() which gives you a callback when its done.
Flags: needinfo?(bkelly)
Cycle collection is required after GC.
AudioNodes and AudioParams make cycles.
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a2e3de2a4a
Relax setTimeout throttling for background tabs using web audio, r=bkelly
https://hg.mozilla.org/mozilla-central/rev/f9a2e3de2a4a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1279072
Depends on: 1279092
Depends on: 1283004
Sole, can you verify in a recent nightly that this works as you'd expect? Thanks.
Flags: needinfo?(sole)
Hey

Sorry for the delay in coming back to this. This is certainly better than it used to be! Yay! There are still intermittent gaps in the audio because it seems like we periodically stop?

I put my test case online: http://sole.github.io/test_cases/web_audio/settimeout/ (press play and then switch to another tab)

If I run this with a recent Nightly we get an initial silence then the machine starts playing its rhythm pattern regularly for a while, then it stops for a bit, then continues... etc

If I run this with release Firefox, it is agonising to hear the badly scheduled rhythm - so a good improvement in Nightly!

Not sure if that can be improved even further? Or maybe it is a GC issue? I don't know...
Flags: needinfo?(sole)
Comment on attachment 8753078 [details] [diff] [review]
Relax setTimeout throttling for background tabs using web audio

This patch only disables the background timeout throttling when there is at least one active node on an audio context in the window. 

If you watch the audio notifications on the window as The beat plays, you may notice that they seem to be flickering as the beats are played. I believe that this is because each beat is a separate sound, and they turn on and off as time goes by. If a timeout is scheduled when one of these beats is not playing, the timeout may be throttled depending on how the GC is feeling at the time with this patch.

My guess is that that is what is causing the problems you are seeing on nightly.

To get by this, it might make more sense for us to disable the timeout entirely whenever any audio context is present, whether or not it is in use. We shouldn't have to worry about malicious actors (as they can peg the event loop already - no need to worry about the settimeout). What do you think ben?
Flags: needinfo?(bkelly)
Attachment #8753078 - Flags: feedback?(sole)
Yes, the sounds are separate samples which are triggered by separate BufferSourceNodes, depending on the timing (current beats per minute), via a scheduler that is triggered via setTimeout. It makes sense that this is getting sometimes cut out according to your explanation, as there will not always be a node playing at the time you check for active nodes.

It would also make sense for me that you check for an active AudioContext instead when deciding whether to throttle events or not.

So in short: yes to throttling when AudioContext is active.
(In reply to Michael Layzell [:mystor] from comment #31)
> To get by this, it might make more sense for us to disable the timeout
> entirely whenever any audio context is present, whether or not it is in use.
> We shouldn't have to worry about malicious actors (as they can peg the event
> loop already - no need to worry about the settimeout). What do you think ben?

Works for me.  I recommend opening a new bug for it, though.
Flags: needinfo?(bkelly)
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: