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)
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)
59 bytes,
text/x-review-board-request
|
ehsan.akhgari
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
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.
Reporter | ||
Comment 1•7 years ago
|
||
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.
Reporter | ||
Comment 2•7 years ago
|
||
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)
Reporter | ||
Comment 3•7 years ago
|
||
This works in FF52ESR, but has the problem in FF54.
Reporter | ||
Updated•7 years ago
|
Keywords: regressionwindow-wanted
Comment 4•7 years ago
|
||
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
Comment 6•7 years ago
|
||
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
Assignee | ||
Comment 7•7 years ago
|
||
[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
Reporter | ||
Comment 8•7 years ago
|
||
(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.
Assignee | ||
Comment 9•7 years ago
|
||
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.
Reporter | ||
Comment 10•7 years ago
|
||
(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?
Assignee | ||
Comment 11•7 years ago
|
||
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.
Reporter | ||
Comment 12•7 years ago
|
||
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?
Comment 13•7 years ago
|
||
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.
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Summary: web audio demo does not play smoothly in background → web audio demo with moments of silence does not play smoothly in background
Reporter | ||
Comment 14•7 years ago
|
||
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.
Comment 15•7 years ago
|
||
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)
Reporter | ||
Comment 16•7 years ago
|
||
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)
Comment 17•7 years ago
|
||
(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)
Updated•7 years ago
|
Assignee: nobody → afarre
Comment 18•7 years ago
|
||
And we should probably cache scriptable top for AudioChannelService as we do for the active IndexedDB databases check. See nsPIDOMWindowInner::TryToCacheTopInnerWindow().
Comment hidden (obsolete) |
Comment 20•7 years ago
|
||
(In reply to Pulsebot from comment #19) *sigh* wrong bug number in that commit message, please disregard
Reporter | ||
Comment 21•7 years ago
|
||
(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!
Comment 22•7 years ago
|
||
Seems unlikely to fix for 55 but we could still take a patch/uplift request.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•7 years ago
|
||
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 26•7 years ago
|
||
mozreview-review |
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+
Comment 27•7 years ago
|
||
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?
Assignee | ||
Comment 28•7 years ago
|
||
(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).
Assignee | ||
Comment 29•7 years ago
|
||
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).
Comment 30•7 years ago
|
||
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
Backed this out for bustage in test_webaudioNotification.html https://treeherder.mozilla.org/logviewer.html#?job_id=116448256&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/1165fd9f0b437fd8fc6acc738029911ef23709c8
Flags: needinfo?(padenot)
Comment 32•7 years ago
|
||
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
Assignee | ||
Comment 33•7 years ago
|
||
I simply forgot to update a test.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(padenot)
Comment 34•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a9c11adcdfc7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 35•7 years ago
|
||
Should we let this ride the 56 train or does it need an approval request for Beta?
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(padenot)
Flags: in-testsuite+
Version: unspecified → 53 Branch
Assignee | ||
Comment 36•7 years ago
|
||
I think we want this one pushed as far as we can.
Flags: needinfo?(padenot)
Comment 37•7 years ago
|
||
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/
Assignee | ||
Comment 38•7 years ago
|
||
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 39•7 years ago
|
||
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+
Comment 40•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/9c54a60123e9
Comment 41•7 years ago
|
||
(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-
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•