Closed
Bug 1259831
Opened 9 years ago
Closed 9 years ago
Regression: Web Audio cuts off when devtools is opened on the page.
Categories
(Core :: Web Audio, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla49
People
(Reporter: jujjyl, Assigned: padenot)
References
Details
(Keywords: regression)
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
STR:
1. Visit http://floooh.github.io/oryol/asmjs/Paclone.html
2. Observe that audio is playing once the page loads.
3. Right-click on the blue page background, and click on "Inspect Element (Q)".
Observed: Once the devtools window pops up, the game continues to play as normal, but audio cuts off from the page completely. Closing the devtools window does not bring audio back.
Reporter | ||
Comment 1•9 years ago
|
||
Mozregression points to the following bisection range where this broke:
6:34.48 INFO: Narrowed inbound regression window from [89c1cc2f, 8639f1a6] (3 revisions) to [89c1cc2f, 256e3e81] (2 revisions) (~1 steps left)
6:34.48 INFO: Oh noes, no (more) inbound revisions :(
6:34.48 INFO: Last good revision: 89c1cc2faea8903570688dd38d9f71ec0c5f0b87
6:34.49 INFO: First bad revision: 256e3e81fed77b17ea9158786a349698bb59a6ab
6:34.49 INFO: Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=89c1cc2faea8903570688dd38d9f71ec0c5f0b87&tochange=256e3e81fed77b17ea9158786a349698bb59a6ab
6:35.60 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1132501
James, would you have a moment to peek at this?
Flags: needinfo?(jlong)
Updated•9 years ago
|
Keywords: regression
Comment 2•9 years ago
|
||
It's not a regression in the classic sense. All it does is attach the debugger earlier, when the toolbox opens instead of when the debugger is clicked. Previous to that patch you would see this behavior when the debugger is clicked.
This is a bug when how the audio behaves when a debugger is initially attached to the window. I have no idea why attached it cuts it off. That's definitely a platform problem.
Flags: needinfo?(jlong)
Reporter | ||
Comment 3•9 years ago
|
||
James, your commit was on January the 6th. I tried using an earlier build
https://archive.mozilla.org/pub/firefox/nightly/2016/01/2016-01-05-03-02-11-mozilla-central/firefox-46.0a1.en-US.win64.zip
and opened the console, switched to debugger tab, and clicked around to browse the different script files emsc.js, Paclone.html and Paclone.js, and tried adding a breakpoint to emsc.js line 8 (not hit), and another location in Paclone.js line 1. This did not cut the audio off. (once the breakpoint was hit, audio was naturally paused along with the execution, but resuming execution resumed the audio as well)
James, do you have a STR to repro in a build earlier from the commit on Jan 6th from bug 1132501? You write "Previous to that patch you would see this behavior when the debugger is clicked.", but I'm not sure which action that would exactly be?
Comment 4•9 years ago
|
||
I assumed it would happen because all the patch does is attach the thread earlier. I did not actually test it. I don't know why it wouldn't happen before the patch; there isn't any difference on how the thread connects, just timing.
I'm not the right person to debug this. We need to start in the platform in the audio code and see why it stops and go from there, and see why the debugger attachment is triggering it.
Assignee | ||
Comment 5•9 years ago
|
||
This looks like it works, but I need to test it more. Jukka, I'd have provided
builds for you to test, but try is closed, if you don't have a tree handy, I'll
get you build tomorrow or something.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → padenot
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•9 years ago
|
||
We can do this now because authors can "suspend()" and "resume()" the context as needed, but we can also take the approach of finding the bad interaction between the auto-suspend and the suspend/resume the devtools are causing.
Component: Developer Tools → Developer Tools: Web Audio Editor
Assignee | ||
Updated•9 years ago
|
Component: Developer Tools: Web Audio Editor → Web Audio
Product: Firefox → Core
Updated•9 years ago
|
Priority: -- → P1
Assignee | ||
Comment 7•9 years ago
|
||
This is not really needed anymore now that suspend and resume are everywhere,
and simplifies the code quite a lot.
Attachment #8745311 -
Flags: review?(karlt)
Comment 8•9 years ago
|
||
Comment on attachment 8745311 [details] [diff] [review]
Remove the auto-suspend logic for AudioContext
(In reply to Paul Adenot (:padenot) from comment #7)
> This is not really needed anymore now that suspend and resume are everywhere,
> and simplifies the code quite a lot.
I think auto-suspend is actually useful to reduce resource usage, and that
authors shouldn't need to suspend explicitly because the implementation knows
when there is no sound.
It is difficult for authors to know exactly when the sound will fade out.
However, as currently implemented, the auto-suspend is of limited use because
it does not happen when there are quiescent/suspended nodes anywhere in the
context, which is probably the case more often than not. So, I guess it is
questionable whether it is worth having the code, if we are not going to make
it work in more cases.
Please remove the ExtraCurrentTime() declaration, if you'd like to go ahead and remove the definition.
Attachment #8745311 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 9•9 years ago
|
||
Another problem is the time it takes to re-open the stream, we have some data for that, for example for Windows that use WASAPI (Vista+), it can take hundreds of milliseconds:
https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-04-07&keys=__none__!__none__!__none__&max_channel_version=release%252F45&measure=AUDIOSTREAM_LATER_OPEN_MS&min_channel_version=null&os=Windows_NT%252C6.2!Windows_NT%252C10.0!Windows_NT%252C6.1!Windows_NT%252C6.0!Windows_NT%252C6.3!Windows_NT%252C5.2&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-03-03&table=0&trim=1&use_submission_date=0
I'm going to land this.
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
sorry had to back this out due the fact that bug 1268313 needed to be backed out for causing memory leaks and that push touched dom/media/webaudio/AudioDestinationNode.cpp - so this change here had to be backed out to make the backout of bug 1268313 possible, sorry paul!
Flags: needinfo?(padenot)
Comment 12•9 years ago
|
||
Comment 13•9 years ago
|
||
Comment 15•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•8 years ago
|
Version: unspecified → 46 Branch
Updated•8 years ago
|
Flags: qe-verify+
Comment 17•8 years ago
|
||
Hi,
I have tested this issue using the latest Nightly (id: 20160904030201) build on:
- Windows 10 x64
- MacOS 10.12 Sierra Developer Beta 8
- Ubuntu 14.04 LTS x64
This is Verified Fixed on Mac and Windows but the sound DOESN'T work altogether on Ubuntu.
Please note that the link in Comment #0 is broken but I have found the page at:
http://floooh.github.io/oryol-samples/asmjs/Paclone.html
Leaving the status flag as is for now.
Assignee | ||
Comment 18•8 years ago
|
||
This works for me on linux, ubuntu 16.04.
Comment 19•8 years ago
|
||
Since this only has a "status-firefox49: fixed" flag i have also tested it on the current Firefox RC 49.0 (id: 20160905130425) on:
- Windows 7 x64
- Ubuntu 16.04 x32
- Mac OSX 10.9.5
and the issue is still reproducible on all OS's using the STR in Comment #0 and the link:
http://floooh.github.io/oryol-samples/asmjs/Paclone.html
Comment 20•8 years ago
|
||
Paul, according to Comment 19, this is still an issue on Fx49 -- should we file a new bug or just reopen this one?
Flags: needinfo?(padenot)
Assignee | ||
Comment 21•8 years ago
|
||
This needs bug 1269741. It's too late in beta to take it, I believe, but the fix is in 50.
Flags: needinfo?(padenot)
Comment 22•8 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #21)
> This needs bug 1269741. It's too late in beta to take it, I believe, but the
> fix is in 50.
Thank you for following up on this. Updating flags accordingly.
Comment 23•8 years ago
|
||
I agree with Paul that it's too late for Beta, and it won't affect most users (those not playing with devtools). Plus, it can be worked around.
status-firefox48:
--- → wontfix
Comment 24•8 years ago
|
||
This issue is verified fixed on the latest Aurora 50.0a2 (2016-14-09), using Windows 10 x64, Ubuntu 16.04 X64 LTS and Mac OS X 10.11.
---Marking flags here accordingly.
Updated•2 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•