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

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla33
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.0, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: ft:loop)

Attachments

(2 attachments, 7 obsolete attachments)

No description provided.
Posted patch bug.patch (obsolete) — Splinter Review
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?
Attachment #8442206 - Flags: review?(ehsan)
Component: DOM → Web Audio
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.)
Attachment #8442206 - Flags: review?(ehsan) → review-
(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).
Posted patch bug.patch (obsolete) — Splinter Review
Attachment #8442206 - Attachment is obsolete: true
Attachment #8442456 - Flags: review?(ehsan)
(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.
Attachment #8442456 - Flags: review?(ehsan) → review+
Posted patch bug.patch (obsolete) — Splinter Review
I added a new mochitest
Attachment #8442456 - Attachment is obsolete: true
Blocks: 1016277
Posted patch bug.patch (obsolete) — Splinter Review
Wrong use of the mutex and NodeMainThread().


https://tbpl.mozilla.org/?tree=Try&rev=38759dd0039d
Attachment #8442686 - Attachment is obsolete: true
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.
Attachment #8442771 - Flags: review?(mchen)
The CPP test was failing. We have to spin the loop if we apply this patch.
Attachment #8442771 - Attachment is obsolete: true
Attachment #8442771 - Flags: review?(mchen)
Attachment #8442839 - Flags: review?(mchen)
This bug blocks bug 1016277. Should it be done for2.0?
blocking-b2g: --- → 2.0?
Attachment #8442839 - Flags: review?(mchen) → review?(ehsan)
(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.
blocking-b2g: 2.0? → ---
feature-b2g: --- → 2.0
Whiteboard: ft:loop
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.
Attachment #8442839 - Flags: review?(ehsan) → review+
> >  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.
Posted patch patch 2 - WebAudio patch (obsolete) — Splinter Review
rebased
Attachment #8442687 - Attachment is obsolete: true
Attachment #8445240 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0da45d558dc6
https://hg.mozilla.org/mozilla-central/rev/015841f10093
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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
Attachment #8445239 - Flags: approval-mozilla-aurora?
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
Attachment #8445585 - Flags: approval-mozilla-aurora?
Attachment #8445585 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8445239 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Can't be uplifted until bug 1023175 is approved as well.
Depends on: 1023175
You need to log in before you can comment on or make changes to this bug.