Closed Bug 1375119 Opened 7 years ago Closed 7 years ago

web audio demo with moments of silence does not play smoothly in background

Categories

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

53 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- wontfix
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: bkelly, Assigned: padenot)

References

Details

(Keywords: regression)

Attachments

(1 file)

From bug 1291741 comment 12:

- I am going to http://sole.github.io/test_cases/web_audio/settimeout/
- Click play
- Change to another tab so this one goes to the background

It seems we still have problems accurately determining if a background tab is really using audio.
This drum demo has a fair amount of time between beats where things are silent.  I wonder if we are deciding its background during the silent parts.
Maybe we need to have some kind of delay from going silent until we mark the window as not using audio.  Ehsan, what do you think?

(Note, I haven't actually debugged anything here.)
Flags: needinfo?(ehsan)
This works in FF52ESR, but has the problem in FF54.
Looks like this has broken some time between 52 and 54.  Alice, any chance you could please help us find a regression range?
Flags: needinfo?(ehsan) → needinfo?(alice0775)
Keywords: regression
Sole, how bad is this regression for web audio apps?
Flags: needinfo?(sole)
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8b738dfc4843e164b111e0866b951f4a2aa063a6&tochange=77313ca8fd71c1dabd8ba33c85569503146f6af8

Regressed by:
77313ca8fd71	Ehsan Akhgari — Bug 1336484 - Don't throttle timeouts in background tabs that are playing audio; r=baku
Blocks: 1336484
Flags: needinfo?(alice0775)
[0] is the issue. In sole's demo, there are a lot of gaps in the audio rendered. Because our implementation is smart, we don't even allocate buffers when no buffers are needed (because they would all be full of zeroes anyways).

We should only consider that the page is "silent" and throttle `setTimeout` and friends when all AudioContexts are suspended or closed or the like.

[0]: http://searchfox.org/mozilla-central/source/dom/media/webaudio/AudioDestinationNode.cpp#261
(In reply to Paul Adenot (:padenot) from comment #7)
> We should only consider that the page is "silent" and throttle `setTimeout`
> and friends when all AudioContexts are suspended or closed or the like.

Well, I believe there have been cases of people using silent audio nodes specifically to disable background timing.  We probably need some kind of "silent for at least X amount of time" check.
This would be very fragile.

Besides, there is a very visible icon when a tab is playing audio, and the icon should be present even if the page is playing silent audio, so that authors are less tempted to do this "playing silent audio to deactivate throttling" hack.

Muting audio on the page (via a click on the icon) should restore this throttling, though.
(In reply to Paul Adenot (:padenot) from comment #9)
> Besides, there is a very visible icon when a tab is playing audio, and the
> icon should be present even if the page is playing silent audio, so that
> authors are less tempted to do this "playing silent audio to deactivate
> throttling" hack.

My read of the comments in bug 1336484 is that we were trying to match the audio icon behavior in the tab.  Why doesn't the icon come and go on this demo?
It does, but it's a bit hard to check in this demo because the drum sounds used by the page have a long tail (that is not necessarily audible or visible in the visualization). Do the following:

- Lower the BPM using the slider so that the progression now (around 40 bpm worked for me)
- Only check one of the <input>
- Let it run for a loop or two, looking at the icon

You'll see it disappears and re-appears as expected, and that shows the problem.
So our timer audio activity is based on this:

http://searchfox.org/mozilla-central/source/dom/audiochannel/AudioChannelService.cpp#636

What is the icon based on?
The icon has some kind of a timer that removes the icon after a certain period of time has passed since the last audio for the page was played.
Summary: web audio demo does not play smoothly in background → web audio demo with moments of silence does not play smoothly in background
I think we should move the timer functionality out of the tab icon code and down into the AudioChannelService.  Then both the window and the icon could use the same API.

Unfortunately I don't think I will have time to work on this for a while.  If someone familiar with the code could take a look it would be very helpful.
Ehsan: the same demo plays fine in Chrome even when the tab is in the background. So it makes Firefox sound literally bad 8-)
Flags: needinfo?(sole)
Andreas, could you look at this as part of your background budget throttling work?  See comment 14 for a possible way forward.  Our current theory is we are to quick to mark a window as "no audio" if there are moments of silence during a song.
Flags: needinfo?(afarre)
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #16)
> Andreas, could you look at this as part of your background budget throttling
> work?  See comment 14 for a possible way forward.  Our current theory is we
> are to quick to mark a window as "no audio" if there are moments of silence
> during a song.

I can pick this up, but I think that I'll do it separately in that case.
Flags: needinfo?(afarre)
Assignee: nobody → afarre
And we should probably cache scriptable top for AudioChannelService as we do for the active IndexedDB databases check. See nsPIDOMWindowInner::TryToCacheTopInnerWindow().
(In reply to Pulsebot from comment #19)

*sigh* wrong bug number in that commit message, please disregard
(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> (In reply to Pulsebot from comment #19)
> 
> *sigh* wrong bug number in that commit message, please disregard

Sorry!
Seems unlikely to fix for 55 but we could still take a patch/uplift request.
Taking, as discussed on irc.
Assignee: afarre → padenot
Ehsan, this is quite orange on Windows debug [0] (other platforms are fine), and I don't know much about those bc jobs.

In any case, I verified that the test fails when the modification in `nsGlobalWindow.cpp` are not present (and that it fixes the problem sole had).

[0]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98db086c70da459adf1db4189f13b49c8f511fd7&selectedJob=115945728
Comment on attachment 8888294 [details]
Bug 1375119 - Consider a page active if it has running AudioContexts.

https://reviewboard.mozilla.org/r/159246/#review164762

::: dom/media/webaudio/AudioContext.cpp:498
(Diff revision 1)
>  }
>  
> +bool
> +AudioContext::IsRunning() const
> +{
> +  return mAudioContextState == AudioContextState::Running;

Nit: Any reason this isn't inlined?
Attachment #8888294 - Flags: review?(ehsan) → review+
I wonder if the oranges are related to your change?  Have you tried adding some printf statement to see if your change to return true is running when these oranges happen?
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #26)
> Comment on attachment 8888294 [details]
> Bug 1375119 - Consider a page active if it has running AudioContexts.
> 
> https://reviewboard.mozilla.org/r/159246/#review164762
> 
> ::: dom/media/webaudio/AudioContext.cpp:498
> (Diff revision 1)
> >  }
> >  
> > +bool
> > +AudioContext::IsRunning() const
> > +{
> > +  return mAudioContextState == AudioContextState::Running;
> 
> Nit: Any reason this isn't inlined?

Two reasons:
- This enum is defined in the audiocontext bindings header and I didn't want to pull everything in `nsGlobalWindow.cpp`, it's big enough as it is.
- I'm changing this in another patch, and it's handy to have this, because this new `IsRunning` will cover a bit more stuff than `mAudioContextState == AudioContextState::Running` (mainly to properly handle pausing AudioContext when breaking in the JS debugger).
Yeah so those super orange tests on windows 10 64bits are not even tier 3, they are simply not running on regular branches, but somehow I forced running them on try. I've tried your suggestion of adding a print, and it's not printed (as expected, I would be surprised if say, popup tests were using AudioContexts).
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd615bff6069
Consider a page active if it has running AudioContexts. r=ehsan
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c11adcdfc7
Consider a page active if it has running AudioContexts. r=ehsan
I simply forgot to update a test.
Flags: needinfo?(padenot)
https://hg.mozilla.org/mozilla-central/rev/a9c11adcdfc7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Should we let this ride the 56 train or does it need an approval request for Beta?
Flags: needinfo?(padenot)
Flags: in-testsuite+
Version: unspecified → 53 Branch
I think we want this one pushed as far as we can.
Flags: needinfo?(padenot)
Drive by comment, but as reporter of the bug I can happily confirm that it works perfect in the latest Nightly. Thank you all <o/
Comment on attachment 8888294 [details]
Bug 1375119 - Consider a page active if it has running AudioContexts.

Approval Request Comment
[Feature/Bug causing the regression]: When a web page is not in the foreground, and is using setTimeout (or other timer functions) to schedule Web Audio API calls, timing becomes off.
[User impact if declined]: Breakage of Web Audio API applications when the page is not in the foreground.
[Is this code covered by automated tests?]: Yes, I adapted the tests for this. I check that the test I modified fail when the C++ patch is not applied.
[Has the fix been verified in Nightly?]: Yes, sole just said that it was now working for her (she originally reported the bug). 
[Needs manual test from QE? If yes, steps to reproduce]: No need, we just had an independent verification (see previous question), and I manually and carefully  checked.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: This is reverting to our previous behavior (prior to  bug 1336484), so I'd say no.
[Why is the change risky/not risky?]: See above.
[String changes made/needed]: None
Attachment #8888294 - Flags: approval-mozilla-beta?
Comment on attachment 8888294 [details]
Bug 1375119 - Consider a page active if it has running AudioContexts.

fix issues when playing audio in background, beta55+

should be in 55.0b13
Attachment #8888294 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Paul Adenot (:padenot) from comment #38)
> [Is this code covered by automated tests?]: Yes, I adapted the tests for
> this. I check that the test I modified fail when the C++ patch is not
> applied.
> [Has the fix been verified in Nightly?]: Yes, sole just said that it was now
> working for her (she originally reported the bug). 
> [Needs manual test from QE? If yes, steps to reproduce]: No need, we just
> had an independent verification (see previous question), and I manually and
> carefully  checked.

Setting qe-verify- based on Paul's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.