Closed
Bug 1346872
Opened 8 years ago
Closed 8 years ago
Audio indicator disappears after unmuting tab if an html5 page regains sound
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox52 | --- | unaffected |
firefox-esr52 | --- | unaffected |
firefox53 | + | verified |
firefox54 | + | verified |
firefox55 | --- | verified |
People
(Reporter: 684sigma, Assigned: alwu)
References
Details
(Keywords: regression)
Attachments
(5 files)
(deleted),
application/x-javascript
|
Details | |
(deleted),
text/x-review-board-request
|
ehsan.akhgari
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ehsan.akhgari
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ehsan.akhgari
:
review+
|
Details |
(deleted),
patch
|
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
I have a problem with Firefox Beta 52. It doesn't happen in Firefox ESR 45.
Sometimes audio indicator disappears if page has sound, when I unmute tab (Ctrl+M).
It happens unpredictably, however, I noticed one specific scenario when it happens
1. Open http://lagged.com/play/352/ , wait until advertisement finishes
2. Enable sound in the game
3. Mute tab, disable sound in the game, enable sound in the game, unmute tab
Result: No audio indicator in tab
Expected: Audio indicator in tab
Other examples
http://lagged.com/play/352/
http://lagged.com/play/535/
http://lagged.com/play/571/
Keywords: regression
Another bug happens on all examples above, filed as bug 1346880.
Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=32dcdde1fc64fc39a9065dc4218265dbc727673f&tochange=27dade5e0c8350189eeb6495d70a9fb25ce137a9
Suspect:
ede7e1b58095 Alastor Wu — Bug 1192818 - part2 : only dispatch DOMAudioPlaybackStarted when there is audible sound. r=baku
Reporter, I can't reproduce this problem in 52.0 & 52.0esr, please confirm again.
Blocks: 1192818, tab-sound-indicator
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox52:
--- → unaffected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox55:
--- → affected
Ever confirmed: true
Summary: Audio indicator disappears after unmuting tab if page has sound (sometimes) → Audio indicator disappears after unmuting tab if Flash regain sound
Version: 52 Branch → 53 Branch
Yes, there was a mistake in the first comment, because I have 2 versions of Beta. Sorry.
It doesn't happen in Firefox Beta 52, it was found in Firefox Beta 53.
Comment 4•8 years ago
|
||
[Tracking Requested - why for this release]:
user-visible regression to a visible control. Makes it impossible to mute a tab.
tracking-firefox53:
--- → ?
Flags: needinfo?(alwu)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → alwu
Flags: needinfo?(alwu)
It is an html5 game drawn on <canvas>, and it doesn't use <audio> or <video>, it uses Web Audio
https://developer.mozilla.org/en-US/docs/Web/API/Web_Audio_API
Summary: Audio indicator disappears after unmuting tab if Flash regain sound → Audio indicator disappears after unmuting tab if an html5 page regains sound
An easier way to reproduce the bug:
1. Open https://mdn.github.io/webaudio-examples/decode-audio-data/
2. Execute attached JavaScript code
3. Click "Play" button on the page. When audio starts, click "Reconnect" button on the page.
4. Mute tab, click "Mute" button on the page, click "Unmute" button on the page, unmute tab.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8850478 -
Flags: review?(ehsan)
Attachment #8850479 -
Flags: review?(ehsan)
Attachment #8850480 -
Flags: review?(ehsan)
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8850478 [details]
Bug 1346872 - part1 : notify audible state change when AudioDestinationNode was muted or suspended.
https://reviewboard.mozilla.org/r/123062/#review127174
::: dom/media/webaudio/AudioDestinationNode.cpp:336
(Diff revision 2)
> , mFramesToProduce(aLength)
> , mAudioChannel(AudioChannel::Normal)
> , mIsOffline(aIsOffline)
> , mAudioChannelSuspended(false)
> , mCaptured(false)
> + , mAudible(AudioChannelService::AudibleState::eAudible)
Why are you defaulting to eAudible and not to, for example, eNotAudible? I'm not 100% sure if this is a problem, but it certainly looks weird, because by default when you create a destination node, its output will not be audible unless a node with audible input gets connected to an input port into it...
::: dom/media/webaudio/AudioDestinationNode.cpp:390
(Diff revision 2)
> {
> if (mAudioChannelAgent && !Context()->IsOffline()) {
> mAudioChannelAgent->NotifyStoppedPlaying();
> mAudioChannelAgent = nullptr;
> + // Reset the state, and it would always be regard as audible.
> + mAudible = AudioChannelService::AudibleState::eAudible;
Similar comment to the above.
Attachment #8850478 -
Flags: review?(ehsan) → review+
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8850479 [details]
Bug 1346872 - part2 : only access agent related codes in nsNPAPIPluginInstance.
https://reviewboard.mozilla.org/r/123064/#review127176
::: commit-message-c5647:4
(Diff revision 2)
> +Bug 1346872 - part2 : only access agent related codes in nsNPAPIPluginInstance.
> +
> +nsNPAPIPlugin doesn't need to know about agent, nsNPAPIPluginInstance should wrap
> +the all details in its memeber function.
Nit: s/the all details/all of the details/
::: dom/plugins/base/nsNPAPIPluginInstance.cpp:1758
(Diff revision 2)
> + rv = WindowVolumeChanged(config.mVolume, config.mMuted);
> + if (NS_WARN_IF(NS_FAILED(rv))) {
> + return;
> + }
> +
> + // Since we only support for muting now, the implementation of suspend
Nit: s/for muting/muting for/
Attachment #8850479 -
Flags: review?(ehsan) → review+
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8850480 [details]
Bug 1346872 - part3 : add and modify test.
https://reviewboard.mozilla.org/r/123080/#review127448
Attachment #8850480 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 20•8 years ago
|
||
(In reply to :Ehsan Akhgari (busy) from comment #17)
> Comment on attachment 8850478 [details]
> Bug 1346872 - part1 : notify audible state change when AudioDestinationNode
> was muted or suspended.
>
> https://reviewboard.mozilla.org/r/123062/#review127174
>
> ::: dom/media/webaudio/AudioDestinationNode.cpp:336
> (Diff revision 2)
> > , mFramesToProduce(aLength)
> > , mAudioChannel(AudioChannel::Normal)
> > , mIsOffline(aIsOffline)
> > , mAudioChannelSuspended(false)
> > , mCaptured(false)
> > + , mAudible(AudioChannelService::AudibleState::eAudible)
>
> Why are you defaulting to eAudible and not to, for example, eNotAudible?
> I'm not 100% sure if this is a problem, but it certainly looks weird,
> because by default when you create a destination node, its output will not
> be audible unless a node with audible input gets connected to an input port
> into it...
>
Since we would only call NotifyStartedPlaying() when we're audible, and call NotifyStartedPlaying() when web audio becomes non-audible [1]. So the only case |mAudible| would be non-audible is when web audio is muted or suspended.
How do you think?
Thanks!
[1] https://goo.gl/ABmwP2
Flags: needinfo?(ehsan)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•8 years ago
|
||
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/272c4a4d0e33
part1 : notify audible state change when AudioDestinationNode was muted or suspended. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/04eaba0da9f9
part2 : only access agent related codes in nsNPAPIPluginInstance. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/e93068689700
part3 : add and modify test. r=Ehsan
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/272c4a4d0e33
https://hg.mozilla.org/mozilla-central/rev/04eaba0da9f9
https://hg.mozilla.org/mozilla-central/rev/e93068689700
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 27•8 years ago
|
||
alwu,
Per comment 5, Could you request uplift to Aurora and Beta?
Flags: needinfo?(alwu)
Assignee | ||
Comment 28•8 years ago
|
||
Approval Request Comment
[Feature/Bug causing the regression]: Regression by bug1192818.
[User impact if declined]: The sound indicator couldn't display correctly
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: Won't affect the normal playback and it can make sound indicator showing correctly
[String changes made/needed]: No
Flags: needinfo?(alwu)
Attachment #8853290 -
Flags: approval-mozilla-beta?
Attachment #8853290 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
status-firefox-esr52:
--- → unaffected
This seems like an ideal thing for manual testing since we have steps to reproduce it.
Also, seems like a good idea to verify the fix, it doesn't seem trivial glancing over the code or from the discussion in this bug!
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
Comment on attachment 8853290 [details] [diff] [review]
Bug 1346872 - notify audible state change for webaudio and NPAPI. (aurora-beta)
This seems a bit risky to uplift late in beta (heading into beta 9, a week from the RC build) But since it is a regression from 53, it would be nice not to ship it to release.
Andrei- can your team verify this once it lands in beta 9? Thanks.
Attachment #8853290 -
Flags: approval-mozilla-beta?
Attachment #8853290 -
Flags: approval-mozilla-beta+
Attachment #8853290 -
Flags: approval-mozilla-aurora?
Attachment #8853290 -
Flags: approval-mozilla-aurora+
Comment 31•8 years ago
|
||
bugherder uplift |
Comment 32•8 years ago
|
||
Comment 33•8 years ago
|
||
had to back this out from beta and aurora for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=88264348&repo=mozilla-aurora
Flags: needinfo?(alwu)
Updated•8 years ago
|
What if, instead, we back out the work from bug 1192818? With that removed, does the audio indicator work as expected ? Either way, we should aim to fix this for beta 10 later this week.
Comment 35•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #34)
> What if, instead, we back out the work from bug 1192818? With that
> removed, does the audio indicator work as expected ? Either way, we should
> aim to fix this for beta 10 later this week.
let me know if the sheriffs should do this :)
Flags: needinfo?(lhenry)
Alastor, Dão, what do you think about backing out bug 1192818 rather than landing this on beta 10?
Flags: needinfo?(dao+bmo)
Comment 37•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #36)
> Alastor, Dão, what do you think about backing out bug 1192818 rather than
> landing this on beta 10?
You'll need to ask DOM / Media folks. I'm gonna move this bug to the right component now.
Component: Tabbed Browser → DOM
Flags: needinfo?(dao+bmo)
Product: Firefox → Core
Target Milestone: Firefox 55 → mozilla55
Assignee | ||
Comment 38•8 years ago
|
||
I'm ok for backout bug 1192818 in aurora and beta.
Flags: needinfo?(alwu)
Comment 39•8 years ago
|
||
I have reproduced this issue using Firefox 53.0b1 (2017.03.07) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 55.0a1 on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.
Reporter | ||
Comment 40•8 years ago
|
||
(In reply to Alastor Wu [:alwu] from comment #38)
> I'm ok for backout bug 1192818 in aurora and beta.
It would also fix Bug 1351020 (temporarily), because it was also caused by 1192818, according to Bug 1351020 Сomment 3.
Comment 41•8 years ago
|
||
Can confirm that bug 1192818 backs out cleanly from Beta. Just say the word :)
Comment 42•8 years ago
|
||
uplift |
Backed out from Beta with IRL approval from Liz.
https://hg.mozilla.org/releases/mozilla-beta/rev/0274a55c5f30
Gerry, this may still be a problem in 54.
tracking-firefox54:
--- → +
Flags: needinfo?(lhenry)
Comment 44•8 years ago
|
||
Alastor,
How should we do for 54? Still backout Bug 1192818?
Flags: needinfo?(alwu)
Comment 45•8 years ago
|
||
I can confirm this issue is fixed, I verified using Firefox 53.0b10 on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 14.04 x64.
Comment 47•8 years ago
|
||
Backed out from 54. Timea, can you please verify that tomorrow's Aurora nightly is fixed as well? Thanks!
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3d016aadda6
Flags: needinfo?(timea.zsoldos)
Comment 48•8 years ago
|
||
I can confirm this issue is fixed, I verified using Firefox 54.0a2 on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 14.04 x64.
Flags: needinfo?(timea.zsoldos)
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•