879111 - Prevent WebAudio from outputting sound when allowMedia is false on the docshell

Status: RESOLVED FIXED

(Core :: Web Audio, enhancement)

Description

[Matthew N. [:MattN]]

Bug 759964 added an allowMedia flag to the docshell to disable <video>/<audio>. We can use this same flag for WebAudio.

(Quoting Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> You probably want to disable WebAudio based on this flag as well. Add some
> check to AudioContext...

Comment 1

[Drew Willcoxon :adw]

Attachment: patch?

This seems to work... I don't know how to write a test for it.

Comment 2

[(no longer active)]

Comment on attachment 767042 [details] [diff] [review]
patch?

Review of attachment 767042 [details] [diff] [review]:
-----------------------------------------------------------------

Testing this will be very hard if not impossible given the way that this patch works...

::: content/media/webaudio/AudioContext.cpp
@@ +58,5 @@
>    // Actually play audio
> +  nsCOMPtr<nsIDocShell> docShell = do_GetInterface(aWindow);
> +  if (!docShell || docShell->GetAllowMedia()) {
> +    mDestination->Stream()->AddAudioOutput(&gWebAudioOutputKey);
> +  }

What if the docshell's state changes later on?  Also, offline audio contexts should not be affected by this as they never play back anything.

Comment 3

[(no longer active)]

So we talked about this on IRC, and decided that muting the audio probably makes more sense.  Here's how you can implement that.

In Web Audio, every audio node has an associated engine, which takes an AudioChunk as input and produces one as output.  The easiest way to mute all audio output for a given AudioContext is to set the mVolume member of the output AudioChunk to 0 (mVolume is a multiplier that will get applied to every audio sample upon playback.)

The engine for a given node usually inherits from a base class called AudioNodeEngine, which just passes its input through its output.  For AudioDestinationNode, we already have an engine for the offline case (which, you don't need to worry about here, as I said in comment 2.)  You should derive a new engine class from AudioNodeEngine, maintain a float mVolume member in it, and then in its ProduceAudioBlock, do something like:

 *aOutput = aInput;
 aOutput->mVolume *= mVolume;

Then, you should add an AudioContext::Mute function which calls the AudioDestinationNode::Mute function which you should also add, and in that function use AudioNode::SendDoubleParameterToStream to send 0 to the engine class.  Then inside the engine, you can initialize mVolume to 1.0, override AudioNodeEngine::SetDoubleParameter, and receive the volume parameter and put it in mVolume.  This way, when you call Mute(), you end up sending the 0 value to the engine, which will be used as mVolume as soon as it's received, which guarantees that the engine will produce silence from that point on.

With this setup, implementing an Unmute function will be really simple, all you have to do is to use AudioNode::SendDoubleParameterToStream to send 1 to the engine so reset mVolume to 1.0.

Now, once you have all of that in place, see nsGlobalWindow.  There we have a list of all AudioContexts associated with the current window.  You need to iterate over them all and call Mute() on each one of them when the allowMedia flag is set.

It's *as simple* as that!  :-)

Comment 4

[Drew Willcoxon :adw]

Thanks, Ehsan.

So I'm working on this and I notice there are AudioContext::Suspend and Resume, and that nsGlobalWindow calls Suspend when it suspends timeouts, and Suspend calls MediaStream::ChangeExplicitBlockerCount, whose comment [1] is:

> // Explicitly block. Useful for example if a media element is pausing
> // and we need to stop its stream emitting its buffered data.

Sounds useful!  I looked some more at mExplicitBlockerCount and TimeVarying, and it looks like blocking counts can be set at certain times and apply thereafter, and if the count is > 0 the stream is "blocked," but I don't quite understand.

Ehsan, can we not use that here?  I'm sure you would have said so if so, but I thought I'd make sure.

[1] http://mxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.h#316

Comment 5

[Drew Willcoxon :adw]

Attachment: patch

I'll attach a test separately once I figure out how to write it.

Comment 6

[Drew Willcoxon :adw]

Ehsan, I'm having a hard time inspecting the destination node, since its max number of outputs is 0, so it can't connect to anything.  I was hoping to verify that all the samples (frames?) in its buffer are zero when the context is muted.  Any ideas?

Comment 7

[(no longer active)]

(In reply to Drew Willcoxon :adw from comment #4)
> Thanks, Ehsan.
> 
> So I'm working on this and I notice there are AudioContext::Suspend and
> Resume, and that nsGlobalWindow calls Suspend when it suspends timeouts, and
> Suspend calls MediaStream::ChangeExplicitBlockerCount, whose comment [1] is:
> 
> > // Explicitly block. Useful for example if a media element is pausing
> > // and we need to stop its stream emitting its buffered data.
> 
> Sounds useful!  I looked some more at mExplicitBlockerCount and TimeVarying,
> and it looks like blocking counts can be set at certain times and apply
> thereafter, and if the count is > 0 the stream is "blocked," but I don't
> quite understand.
> 
> Ehsan, can we not use that here?  I'm sure you would have said so if so, but
> I thought I'd make sure.

Suspend() pauses the audio, so it's not useful here.

Comment 8

[(no longer active)]

(In reply to Drew Willcoxon :adw from comment #6)
> Ehsan, I'm having a hard time inspecting the destination node, since its max
> number of outputs is 0, so it can't connect to anything.  I was hoping to
> verify that all the samples (frames?) in its buffer are zero when the
> context is muted.  Any ideas?

What gets to the destination node is invisible from the point of view of web content.  I don't think there's any good way to automatically test this... :(

Comment 9

[(no longer active)]

Comment on attachment 769868 [details] [diff] [review]
patch

Review of attachment 769868 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/AudioContext.cpp
@@ +532,5 @@
>  
> +void
> +AudioContext::Mute() const
> +{
> +  mDestination->Mute();

Please MOZ_ASSERT(!mIsOffline) here and below.

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +182,5 @@
> +{
> +public:
> +  explicit DestinationNodeEngine(AudioDestinationNode* aNode)
> +    : AudioNodeEngine(aNode)
> +    , mVolume(1)

Nit: 1.0f.

@@ +191,5 @@
> +                                 const AudioChunk& aInput,
> +                                 AudioChunk* aOutput,
> +                                 bool* aFinished) MOZ_OVERRIDE
> +  {
> +    AudioNodeEngine::ProduceAudioBlock(aStream, aInput, aOutput, aFinished);

Hmm, please just do |*aOutput = aInput;| here, it would be much easier to read.

@@ +201,5 @@
> +    if (aIndex == VolumeParameter) {
> +      mVolume = aParam;
> +      return;
> +    }
> +    AudioNodeEngine::SetDoubleParameter(aIndex, aParam);

You don't need to call the base class function here.

@@ +205,5 @@
> +    AudioNodeEngine::SetDoubleParameter(aIndex, aParam);
> +  }
> +
> +  enum {
> +    VolumeParameter,

Nit: please use the Parameters convention we have in other engines here, such as DelayNodeEngine, for example.

@@ +256,5 @@
>  
>  void
> +AudioDestinationNode::Mute()
> +{
> +  SendDoubleParameterToStream(DestinationNodeEngine::VolumeParameter, 0);

Please MOZ_ASSERT that we're not on an offline context.

Comment 10

[(no longer active)]

Also, please backport this to Aurora after it lands on central.  Thanks!

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> Also, please backport this to Aurora after it lands on central.  Thanks!

Why, out of curiosity?

Comment 12

[(no longer active)]

(In reply to comment #11)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> > Also, please backport this to Aurora after it lands on central.  Thanks!
> 
> Why, out of curiosity?

Because we are planning to ship Web Audio in 24.

Comment 13

[Drew Willcoxon :adw]

Thanks for your help, Ehsan!

https://tbpl.mozilla.org/?tree=Try&rev=266b926ba639
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9978562db88

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c9978562db88

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 769868 [details] [diff] [review]
patch

[Approval Request Comment]
Ehsan wants this on Aurora because we are planning to ship Web Audio in 24 (see comment 12).

Bug caused by (feature/regressing bug #): bug 759964
User impact if declined: minimal
Testing completed (on m-c, etc.): no automated testing
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 16

[bhavana bajaj [:bajaj]]

Comment on attachment 769868 [details] [diff] [review]
patch

Low risk enough and we are still in a phase to take new feature enhancements in Aurora.Approving the patch.

Ehsan, given there are no automated tests please give us some feeedback on how best this can be tested.If there are any testcases qa can help with, please add qawanted keyword here.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/856a1ecc4b02

Comment 18

[u279076]

Drew, is there a testcase QA can use to verify this is fixed?

Comment 19

[Drew Willcoxon :adw]

Attachment: tone.html

This is similar to the page I was testing with.  You can press the Play button, and after a tone starts playing, run this in the browser console (Shift+Ctrl+J -- not the web console):

gBrowser.selectedBrowser.docShell.allowMedia = false;

The tone should then stop.  Make sure your volume is really down!

Comment 20

[u279076]

Thank you Drew.

Comment 21

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 25 beta 1 - I used the instructions provided in Comment 19.

Verified on Windows 7, Ubuntu 13.04 and Mac OS X 10.8:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0
Build ID: 20130917123208