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/

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.

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.

[Tracking Requested - why for this release]: user-visible regression to a visible control. Makes it impossible to mute a tab.

Track 53+ as user-visible regression.

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

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.

Can you reproduce this? Can we get it fixed in 53?

Sure, I'm working on it now.

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.

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/

Comment on attachment 8850480 [details] Bug 1346872 - part3 : add and modify test. https://reviewboard.mozilla.org/r/123080/#review127448

(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

You're right, never mind. :-)

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

https://hg.mozilla.org/mozilla-central/rev/272c4a4d0e33 https://hg.mozilla.org/mozilla-central/rev/04eaba0da9f9 https://hg.mozilla.org/mozilla-central/rev/e93068689700

alwu, Per comment 5, Could you request uplift to Aurora and Beta?

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

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!

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.

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

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.

(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 :)

Alastor, Dão, what do you think about backing out bug 1192818 rather than landing this on beta 10?

(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.

I'm ok for backout bug 1192818 in aurora and beta.

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.

(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.

Can confirm that bug 1192818 backs out cleanly from Beta. Just say the word :)

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.

Alastor, How should we do for 54? Still backout Bug 1192818?

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.

We can also backout the bug 1192818 in 54.

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

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.