1027172 - AudioContext should dispatch call AudioChannelAgent::StopPlaying() when the destination node doesn't have any input or it's muted.

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: bug.patch

My concern is about the runnable object that has a raw pointer to AudioDestinationNode. AudioDestinationNode is not thread-safe and this can be an issue. Any idea?

Comment 2

[(no longer active)]

Comment on attachment 8442206 [details] [diff] [review]
bug.patch

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

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +190,5 @@
> +  }
> +
> +  NS_IMETHOD Run()
> +  {
> +    mDestinationNode->MutedInput(mMutedInput);

This will all need to change... See below.

@@ +219,5 @@
>    {
>      *aOutput = aInput;
>      aOutput->mVolume *= mVolume;
> +
> +    bool mutedInput = aInput.IsNull() || aInput.IsMuted();

Nit: call this newInputMuted.

@@ +224,5 @@
> +    if (mutedInput != mMutedInput) {
> +      mMutedInput = mutedInput;
> +      nsCOMPtr<nsIRunnable> runnable =
> +        new DestinationNodeEngineRunnable(mDestinationNode, mMutedInput);
> +      NS_DispatchToMainThread(runnable);

Sorry, I should have described how you should do this earlier.  The AudioNodeStream* argument here is refcounted, and it has a reference to the node.  The correct way of implementing this kind of dispatch to the node on the main thread is to use DispatchToMainThreadAfterStreamStateUpdate like we do in BiquadFilterNodeEngine::ProduceAudioBlock.  See the PlayingRefChangeHandler::Run method for how to grab the node mutex in order to get your hands on the AudioDestinationNode pointer.

@@ +245,5 @@
>      return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
>    }
>  
>  private:
> +  AudioDestinationNode* mDestinationNode;

Given what I said above, you won't need another copy of the node.

@@ +250,2 @@
>    float mVolume;
> +  bool mMutedInput;

Nit: call this mLastInputMuted.

@@ +647,5 @@
>  
> +void
> +AudioDestinationNode::MutedInput(bool aMuted)
> +{
> +  if (!mAudioChannelAgent || Context()->IsOffline()) {

After more checking, offline destination nodes use a completely different engine (OfflineDestinationNodeEngine) so we shouldn't actually need to check IsOffline() here (but it makes sense to MOZ_ASSERT it.)

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #2)
> See the
> PlayingRefChangeHandler::Run method for how to grab the node mutex in order
> to get your hands on the AudioDestinationNode pointer.

Please use NodeMainThread() (bug 914392).

Comment 4

[Andrea Marchesini [:baku]]

Attachment: bug.patch

Comment 5

[(no longer active)]

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #3)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #2)
> > See the
> > PlayingRefChangeHandler::Run method for how to grab the node mutex in order
> > to get your hands on the AudioDestinationNode pointer.
> 
> Please use NodeMainThread() (bug 914392).

You're right!

Comment 6

[(no longer active)]

Comment on attachment 8442456 [details] [diff] [review]
bug.patch

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

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +197,5 @@
> +      // we can obtain the reference, we will hold the node alive in
> +      // this function.
> +      MutexAutoLock lock(mStream->Engine()->NodeMutex());
> +      node = mStream->Engine()->Node();
> +    }

Please use NodeMainThread() as Karl suggested.

@@ +235,5 @@
> +    bool newInputMuted = aInput.IsNull() || aInput.IsMuted();
> +    if (newInputMuted != mLastInputMuted) {
> +      mLastInputMuted = newInputMuted;
> +
> +      nsRefPtr<InputMutedRunnable> refchanged =

Nit: please call this something other than refchanged.  ;-)

@@ +236,5 @@
> +    if (newInputMuted != mLastInputMuted) {
> +      mLastInputMuted = newInputMuted;
> +
> +      nsRefPtr<InputMutedRunnable> refchanged =
> +        new InputMutedRunnable(aStream, mLastInputMuted);

Nit: use newInputMuted please to make it explicit that we're using the newly computed value here.

Comment 7

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=17ccb7bd4a0d
testing the patch on try.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: bug.patch

I added a new mochitest

Comment 9

[Andrea Marchesini [:baku]]

Attachment: bug.patch

Wrong use of the mutex and NodeMainThread().


https://tbpl.mozilla.org/?tree=Try&rev=38759dd0039d

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch 1 - AudioChannelService and notification

Marco, this patch is useful for having the same behaviour in IPC and on the main-process for the notification. Look this code:

1  mAudioChannelAgent->StartPlaying(&state);
2  mAudioChannelAgentPlaying =
3    state == AudioChannelState::AUDIO_CHANNEL_STATE_NORMAL;
4  SetCanPlay(mAudioChannelAgentPlaying);

if this code runs on the parent process, CanPlayChanged() is called after line 1 but if it runs on the child process, it runs after line 4.

The reason of that is because for the child process the notification is sent using IPC and it runs when the event loop receives the communication from the parent.

This patch fixes this issue.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch 1 - AudioChannelService and notification

The CPP test was failing. We have to spin the loop if we apply this patch.

Comment 12

[Andrea Marchesini [:baku]]

This bug blocks bug 1016277. Should it be done for2.0?

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Andrea Marchesini (:baku) from comment #12)
> This bug blocks bug 1016277. Should it be done for2.0?

It should as bug 1016277 should be included in v2.0 release.

Comment 14

[Jason Smith [:jsmith]]

(In reply to Andrea Marchesini (:baku) from comment #12)
> This bug blocks bug 1016277. Should it be done for2.0?

bug 1016277 however is considered feature work, so this should go through approval for 2.0.

Comment 15

[Marco Chen [:mchen]]

Comment on attachment 8442839 [details] [diff] [review]
patch 1 - AudioChannelService and notification

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

Sorry for the late response.

::: dom/audiochannel/AudioChannelService.cpp
@@ +665,5 @@
> +
> +  nsCOMPtr<nsIRunnable> runnable = new NotifyRunnable(this);
> +  NS_DispatchToCurrentThread(runnable);
> +}
> +

We can suppress NotifyRunnable event when there is already an one in the message queue of main thread. Then we can avoid sending redundant notification. How about it?

::: dom/audiochannel/tests/TestAudioChannelService.cpp
@@ +13,5 @@
>  #include "AudioChannelService.h"
>  #include "AudioChannelAgent.h"
>  
> +#include "nsIThread.h"
> +#include "nsThreadUtils.h"

nsThreadUtils.h already include nsIThread.h.

@@ +28,5 @@
>  
>  using namespace mozilla::dom;
>  
> +void
> +spin_events_loop_until_false(const bool* const aCondition)

the size of bool itself is not bigger then pointer so maybe we can directly pass bool value.

Comment 16

[Andrea Marchesini [:baku]]

> >  using namespace mozilla::dom;
> >  
> > +void
> > +spin_events_loop_until_false(const bool* const aCondition)
> 
> the size of bool itself is not bigger then pointer so maybe we can directly
> pass bool value.

In this case I need the address of the value what I want to check.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch 1 - AudioChannelService and notification

https://tbpl.mozilla.org/?tree=Try&rev=c695977f1f54

Comment 18

[Andrea Marchesini [:baku]]

Attachment: patch 2 - WebAudio patch

rebased

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch 2 - WebAudio patch

Comment 20

[Andrea Marchesini [:baku]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0da45d558dc6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/015841f10093

green on try: https://tbpl.mozilla.org/?tree=Try&rev=acbf387c0485

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0da45d558dc6
https://hg.mozilla.org/mozilla-central/rev/015841f10093

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 8445239 [details] [diff] [review]
patch 1 - AudioChannelService and notification

Approval Request Comment
[Feature/regressing bug #]: bug 1016277
[User impact if declined]: multiple telephony calls can play audio at the same time.
[Describe test coverage new/current, TBPL]: fully covered.
[Risks and why]: None
[String/UUID change made/needed]: None

Comment 23

[Andrea Marchesini [:baku]]

Comment on attachment 8445585 [details] [diff] [review]
patch 2 - WebAudio patch

Approval Request Comment
[Feature/regressing bug #]: bug 1016277
[User impact if declined]: multiple telephony apps can play audio at the same time.
[Describe test coverage new/current, TBPL]: fully covered.
[Risks and why]: None
[String/UUID change made/needed]: None

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Can't be uplifted until bug 1023175 is approved as well.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/46c8bd676a50
https://hg.mozilla.org/releases/mozilla-aurora/rev/3ee7e6b1dc88