Closed Bug 1346872 Opened 3 years ago Closed 3 years ago

Audio indicator disappears after unmuting tab if an html5 page regains sound

Categories

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

53 Branch
defect
Not set

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)

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.
See Also: → 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.
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
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.
[Tracking Requested - why for this release]:
user-visible regression to a visible control.  Makes it impossible to mute a tab.
Flags: needinfo?(alwu)
Assignee: nobody → alwu
Flags: needinfo?(alwu)
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
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.
Can you reproduce this?  Can we get it fixed in 53?
Flags: needinfo?(alwu)
Sure, I'm working on it now.
Flags: needinfo?(alwu)
Attachment #8850478 - Flags: review?(ehsan)
Attachment #8850479 - Flags: review?(ehsan)
Attachment #8850480 - Flags: review?(ehsan)
See Also: → 1351020
Blocks: 1351020
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 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 on attachment 8850480 [details]
Bug 1346872 - part3 : add and modify test.

https://reviewboard.mozilla.org/r/123080/#review127448
Attachment #8850480 - Flags: review?(ehsan) → review+
(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)
You're right, never mind.  :-)
Flags: needinfo?(ehsan)
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
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
alwu, 
Per comment 5, Could you request uplift to Aurora and Beta?
Flags: needinfo?(alwu)
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?
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+
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)
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 :)
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)
(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
I'm ok for backout bug 1192818 in aurora and beta.
Flags: needinfo?(alwu)
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.
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
(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 :)
Gerry, this may still be a problem in 54.
Flags: needinfo?(lhenry)
Alastor,
How should we do for 54? Still backout Bug 1192818?
Flags: needinfo?(alwu)
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.
Flags: needinfo?(alwu)
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)
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)
Component: DOM → DOM: Core & HTML
See Also: 1351020
You need to log in before you can comment on or make changes to this bug.