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

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

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

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

I added a new mochitest

Wrong use of the mutex and NodeMainThread(). https://tbpl.mozilla.org/?tree=Try&rev=38759dd0039d

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.

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

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

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

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

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

rebased

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

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

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

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

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