Reconcile Web Audio with audio channels

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: baku)

Tracking

Trunk
mozilla27
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox25 wontfix, firefox26 verified, firefox27 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [blocking-webaudio-])

Attachments

(1 attachment, 10 obsolete attachments)

Reporter

Description

6 years ago
So currently Web Audio doesn't integrate with audio channels at all.  Andrea, can you please tell me what things I should do in order to integrate Web Audio with audio channels, and how I can test this stuff?

Thanks!
Flags: needinfo?(amarchesini)
Assignee

Comment 1

6 years ago
Any audio component (MediaElement, FMRadio, telephony, etc) should have a Audio Channel: https://wiki.mozilla.org/WebAPI/AudioChannels (I suspect this will be a mozilla only attribute).

Then, the WebAudio component should
1. implement nsIAudioChannelAgentCallback
2. create an nsIAudioChannelAgent object with its AudioChannel
3. communicate any visibility change to this AudioChannelAgent
4. call StartPlaying() StopPlaying() when WebAndio starts/stops playing audio.
5. nsIAudioChannelAgent will call CanPlayChanged(bool canPlay) when the audio component should mute/pause itself.

If you want I can take this bug if you drive me into the code a bit.
Flags: needinfo?(amarchesini)
Reporter

Comment 2

6 years ago
(In reply to Andrea Marchesini (:baku) from comment #1)
> Any audio component (MediaElement, FMRadio, telephony, etc) should have a
> Audio Channel: https://wiki.mozilla.org/WebAPI/AudioChannels (I suspect this
> will be a mozilla only attribute).
> 
> Then, the WebAudio component should
> 1. implement nsIAudioChannelAgentCallback

Ugh, that's an XPCOM only interface.  :/

> 2. create an nsIAudioChannelAgent object with its AudioChannel
> 3. communicate any visibility change to this AudioChannelAgent
> 4. call StartPlaying() StopPlaying() when WebAndio starts/stops playing
> audio.
> 5. nsIAudioChannelAgent will call CanPlayChanged(bool canPlay) when the
> audio component should mute/pause itself.
> 
> If you want I can take this bug if you drive me into the code a bit.

Hmm, so first things first, this all sort of assumes that there is a central place which controls the playback, right?  In Web Audio, we have a graph of nodes, where source nodes create audio data which "flows" through the graph and gets played back when the data gets to the destination node.  The source nodes do not have a clear idea when their stuff will be played back since the graph can contain delay nodes which delay the audio output by a fixed or variable time, and the destination node cannot have a very good idea when the playback is started/finished since the nodes may provide silent buffers, which are indistinguishable from silence due to lack of anything to play back.

To make things worse, the Web Audio API doesn't have the notion of "pausing" playback, and pausing the playback while the page is alive may result in unexpected things to happen because there are scheduled AudioParam events which happen on the media graph thread, which require the graph to progress in real time.  It seems like the AudioChannel APIs are not designed for this kind of real-time usage.

How does WebRTC handle this stuff?
Assignee

Comment 3

6 years ago
> Ugh, that's an XPCOM only interface.  :/

Don't worry, I can easily convert it to webIDL.

> Hmm, so first things first, this all sort of assumes that there is a central

I guess there is an 'exit' node. a root node where all the chain of effects, generators, things... are connected.
Can you mute the root node?

> To make things worse, the Web Audio API doesn't have the notion of "pausing"
> playback, and pausing the playback while the page is alive may result in

Ignore pausing, muting?
Reporter

Comment 4

6 years ago
(In reply to comment #3)
> > Ugh, that's an XPCOM only interface.  :/
> 
> Don't worry, I can easily convert it to webIDL.

Well I was mostly complaining about having to add things to the QueryInterface implementation etc.  But wait.  Is this stuff exposed to content?

> > Hmm, so first things first, this all sort of assumes that there is a central
> 
> I guess there is an 'exit' node. a root node where all the chain of effects,
> generators, things... are connected.
> Can you mute the root node?

The exit node is AudioDestinationNode.  It can definitely be force-muted, but not force-paused.

> > To make things worse, the Web Audio API doesn't have the notion of "pausing"
> > playback, and pausing the playback while the page is alive may result in
> 
> Ignore pausing, muting?

Yeah if we can get away without pausing, then muting should work fine.

Another question: which channel should I be using?   normal or content?
Assignee

Comment 5

6 years ago
> Well I was mostly complaining about having to add things to the
> QueryInterface implementation etc.  But wait.  Is this stuff exposed to
> content?

No. it's not exposed to content.

> Another question: which channel should I be using?   normal or content?

Well.. this depends. You should expose this attribute. For instance in the MediaElement we have mozAudioChannel. I think we should have something like this for WebAudio too.
Reporter

Comment 6

6 years ago
(In reply to comment #5)
> > Well I was mostly complaining about having to add things to the
> > QueryInterface implementation etc.  But wait.  Is this stuff exposed to
> > content?
> 
> No. it's not exposed to content.
> 
> > Another question: which channel should I be using?   normal or content?
> 
> Well.. this depends. You should expose this attribute. For instance in the
> MediaElement we have mozAudioChannel. I think we should have something like
> this for WebAudio too.

So, expose it to content?  I don't like that idea.  This is a non-standard API and we should be exposing less of those where we can.  Can't we just pick a sane value?
Assignee

Comment 7

6 years ago
> So, expose it to content?  I don't like that idea.  This is a non-standard
> API and we should be exposing less of those where we can.  Can't we just
> pick a sane value?

Right now I think you can use 'normal'.
Reporter

Comment 8

6 years ago
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio

Comment 9

6 years ago
Right now the dialer app uses the old audio api to specify a different channel for its dial tones. I don't think we convert the dialer app to using Web Audio until this is supported.
Reporter

Comment 10

6 years ago
Andrea, can you take this please?
Reporter

Comment 11

6 years ago
This needs to block 1.2.
blocking-b2g: --- → koi?
Reporter

Updated

6 years ago
Whiteboard: [blocking-webaudio-]
If there is any Web App tried to use Web Audio API to play any kind of audio then it may need to choose the channel type from content. Ex: One day, dialer app choose ringer as the channel output key tone.

By the way, I think since FxOS targets mobile domain, then audio competing policy and volume control on different audio channel type is mandatory feature. Does mozAudioChannel not plan to propose to W3C? Thanks.
Reporter

Comment 13

6 years ago
(In reply to comment #12)
> If there is any Web App tried to use Web Audio API to play any kind of audio
> then it may need to choose the channel type from content. Ex: One day, dialer
> app choose ringer as the channel output key tone.

I'm not sure what you mean here.

> By the way, I think since FxOS targets mobile domain, then audio competing
> policy and volume control on different audio channel type is mandatory feature.

Sure, hence the blocking-b2g nomination.

> Does mozAudioChannel not plan to propose to W3C? Thanks.

I'm not sure, perhaps Andrea knows?
Blocks: 894856
Blocking+ - audio channels represents a fundamental piece of b2g, so we need this as part of the web audio use case.
Assignee: nobody → mreavy
blocking-b2g: koi? → koi+
Ivan -- Since this a koi blocker, what is the preferred deadline for koi bug fixes?  Is it Oct 28?
Assignee: mreavy → amarchesini
Flags: needinfo?(itsay)
(In reply to Maire Reavy [:mreavy] from comment #15)
> Ivan -- Since this a koi blocker, what is the preferred deadline for koi bug
> fixes?  Is it Oct 28?

That sounds about right - October 28th is the date IOT starts, so we want to get Mozilla desired fixes in before then. After that date, triage becomes more partner driven, so it will be harder to argue Mozilla desired bugs to block unless a partner champions the bug.
(In reply to Jason Smith [:jsmith] from comment #16)
> (In reply to Maire Reavy [:mreavy] from comment #15)
> > Ivan -- Since this a koi blocker, what is the preferred deadline for koi bug
> > fixes?  Is it Oct 28?
> 
> That sounds about right - October 28th is the date IOT starts, so we want to
> get Mozilla desired fixes in before then. After that date, triage becomes
> more partner driven, so it will be harder to argue Mozilla desired bugs to
> block unless a partner champions the bug.

Should note - Oct 28th is a recommended IOT date, not the confirmed IOT date. This is based off the schedule I know about.
Hi Maire,

For the koi blocker, each functional team has 6 weeks (from Sept. 16 - Oct. 25) to triage and fix the koi blocker. We are recommended to fix as much as (or ideally all) koi blockers during the period of time. After October 28, we have another 6 weeks for release management to triage the koi and the bug fixing will keep going. But as mentioned, it will be more partner driven.
Flags: needinfo?(itsay)
Assignee

Comment 19

6 years ago
Posted patch patch (obsolete) — Splinter Review
WIP
Assignee

Comment 20

6 years ago
Posted patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=f3c424799787
Attachment #811921 - Attachment is obsolete: true
Attachment #811999 - Flags: review?(ehsan)
Reporter

Comment 21

6 years ago
Comment on attachment 811999 [details] [diff] [review]
patch

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

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +219,5 @@
>  private:
>    float mVolume;
>  };
>  
> +class AudioChannelAgentCallback : public nsIAudioChannelAgentCallback

Nit: please mark this as MOZ_FINAL.

@@ +225,5 @@
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS(AudioChannelAgentCallback)
> +
> +  AudioChannelAgentCallback(AudioDestinationNode* aNode)

Nit: please make this explicit.

@@ +232,5 @@
> +  }
> +
> +  virtual ~AudioChannelAgentCallback()
> +  {
> +  }

You shouldn't need the dtor.

@@ +245,5 @@
> +private:
> +  nsRefPtr<AudioDestinationNode> mNode;
> +};
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(AudioChannelAgentCallback)

Can't you just use NS_IMPL_CYCLE_COLLECTION_CLASS_1?

@@ +262,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(AudioChannelAgentCallback)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(AudioChannelAgentCallback)
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(AudioDestinationNode)

Can't you just use NS_IMPL_CYCLE_COLLECTION_INHERITED_1?

@@ +388,5 @@
>  {
>    mOfflineRenderingRef.Take(this);
>    mStream->Graph()->StartNonRealtimeProcessing(mFramesToProduce);
> +
> +  if (!mAudioChannelAgent) {

StartRendering and OfflineShutdown are both only used for offline audio contexts, which can never play back audio at all.  This patch should in fact not change the behavior of offline contexts, and only affect non-offline contexts.

You should do the initialization work in the ctor for AudioDestinationNode, if aIsOffline is false.  The shutdown code should be moved to the very end of AudioDestinationNode::DestroyMediaStream(), only run if Context()->IsOffline() returns false.
Attachment #811999 - Flags: review?(ehsan) → review-
Assignee

Comment 22

6 years ago
Posted patch audio.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=c7590316d4ba
Attachment #811999 - Attachment is obsolete: true
Attachment #812535 - Flags: review?(ehsan)
Reporter

Comment 23

6 years ago
Comment on attachment 812535 [details] [diff] [review]
audio.patch

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

Looks great!

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +232,5 @@
> +  }
> +
> +  virtual ~AudioChannelAgentCallback()
> +  {
> +  }

Nit: you don't need the virtual dtor.

@@ +305,5 @@
> +    bool isActive = false;
> +    docshell->GetIsActive(&isActive);
> +    mAudioChannelAgent->SetVisibilityState(isActive);
> +
> +    int32_t state;

Nit: please initialize state to 0.

@@ +312,5 @@
> +    if (state == AudioChannelState::AUDIO_CHANNEL_STATE_NORMAL) {
> +      SetCanPlay(true);
> +    } else {
> +      SetCanPlay(false);
> +    }

I'd rewrite this as SetCanPlay(state == AudioChannelState::AUDIO_CHANNEL_STATE_NORMAL);.
Attachment #812535 - Flags: review?(ehsan) → review+
Assignee

Comment 24

6 years ago
Posted patch audio.patch (obsolete) — Splinter Review
Attachment #812535 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Keywords: checkin-needed
Assignee

Comment 25

6 years ago
Talking with Ehsan, we decided that we can implement this code differently.
Keywords: checkin-needed
Reporter

Comment 26

6 years ago
So the problem here is that we don't want content to be able to detect the mutedness of the stream here.  I _think_ we should be able to use the infrastructure created in bug 868405 in disable the AUDIO_NODE_STREAM_TRACK_ID track by calling MediaStream::SetTrackEnabled.

roc/paul, does that make sense to you guys?
Flags: needinfo?(roc)
Flags: needinfo?(paul)
Assignee

Comment 28

6 years ago
Posted patch audio.patch (obsolete) — Splinter Review
Attachment #812912 - Attachment is obsolete: true
Attachment #813584 - Flags: review?(ehsan)
Reporter

Comment 29

6 years ago
Comment on attachment 813584 [details] [diff] [review]
audio.patch

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

::: content/media/webaudio/AudioContext.cpp
@@ +83,5 @@
> +  // Actually play audio
> +  mDestination = new AudioDestinationNode(MOZ_THIS_IN_INITIALIZER_LIST(),
> +                                          aIsOffline, aNumberOfChannels,
> +                                          aLength, aSampleRate);
> +  mDestination->Stream()->AddAudioOutput(&gWebAudioOutputKey);

Why are you moving these?

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +395,5 @@
>  
> +void
> +AudioDestinationNode::SetCanPlay(bool aCanPlay)
> +{
> +  mStream->SetTrackEnabled(AudioNodeStream::AUDIO_TRACK, aCanPlay);

Hmm, I just realized that we don't actually use AUDIO_TRACK anywhere!  Do you mind replacing the usages of AUDIO_NODE_STREAM_TRACK_ID in AudioNodeStream.cpp with AUDIO_TRACK too?

::: content/media/webaudio/AudioDestinationNode.h
@@ +62,5 @@
>    SelfReference<AudioDestinationNode> mOfflineRenderingRef;
>    uint32_t mFramesToProduce;
> +
> +  nsRefPtr<AudioChannelAgent> mAudioChannelAgent;
> +  nsCOMPtr<nsIDocument> mDocument;

Hmm, you're not using mDocument, right?  Please remove it.
Attachment #813584 - Flags: review?(ehsan)
Assignee

Comment 30

6 years ago
Posted patch audio.patch (obsolete) — Splinter Review
Attachment #813584 - Attachment is obsolete: true
Attachment #813626 - Flags: review?(ehsan)
Reporter

Comment 31

6 years ago
Comment on attachment 813626 [details] [diff] [review]
audio.patch

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

::: content/media/webaudio/AudioDestinationNode.h
@@ +62,5 @@
>    SelfReference<AudioDestinationNode> mOfflineRenderingRef;
>    uint32_t mFramesToProduce;
> +
> +  nsRefPtr<AudioChannelAgent> mAudioChannelAgent;
> +  nsCOMPtr<nsIDocument> mDocument;

Please drop mDocument.
Attachment #813626 - Flags: review?(ehsan) → review+
No idea what this means, but that caused both ICS emulator builds and the Inari device build (but not the JB emulator builds) to fail like https://tbpl.mozilla.org/php/getParsedLog.php?id=28730238&tree=Mozilla-Inbound. All three failing builds were clobbers, so it's not that usual explanation.

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/c7bb40f2578a
(In reply to Andrea Marchesini (:baku) from comment #35)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b607cc162c2c

Hey Guys, had to backout this Patch since this checkin was causing a perma-red orange like https://tbpl.mozilla.org/php/getParsedLog.php?id=28744487&tree=Mozilla-Inbound 

The failure i filed (when i was thinking its intermittent) is bug 923661
Assignee

Comment 37

6 years ago
Posted patch audio.patch (obsolete) — Splinter Review
Attachment #813660 - Attachment is obsolete: true
Attachment #813711 - Attachment is obsolete: true
Assignee

Comment 38

6 years ago
Posted patch interdiff (obsolete) — Splinter Review
Attachment #813792 - Flags: review?(ehsan)
Assignee

Updated

6 years ago
Attachment #813790 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #813792 - Attachment is obsolete: true
Attachment #813792 - Flags: review?(ehsan)
Assignee

Comment 39

6 years ago
Comment on attachment 813711 [details] [diff] [review]
audio.patch

Actually the problem is somewhere else. So this patch is still valid.
The issue is: Bug 923776
Attachment #813711 - Attachment is obsolete: false
Assignee

Updated

6 years ago
Depends on: 923776
Assignee

Updated

6 years ago
Flags: needinfo?(paul)
Duplicate of this bug: 923661
Assignee

Comment 41

6 years ago
Posted patch audio.patchSplinter Review
This patch fixes an issue with CC. Now it's green on try:
https://tbpl.mozilla.org/?tree=Try&rev=9ece134b6264
Attachment #813711 - Attachment is obsolete: true
Attachment #814343 - Flags: review?(ehsan)
Reporter

Comment 42

6 years ago
Comment on attachment 814343 [details] [diff] [review]
audio.patch

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

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +13,5 @@
>  #include "MediaStreamGraph.h"
>  #include "OfflineAudioCompletionEvent.h"
> +#include "nsIInterfaceRequestorUtils.h"
> +#include "nsIDocShell.h"
> +#include "nsIDocument.h"

You're not using nsIDocument.h here as far as I can tell.
Attachment #814343 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/f35bab0828e0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Keywords: verifyme
Is there a way I can manually verify this on Firefox desktop?
Flags: needinfo?(ehsan)
(In reply to Bogdan Maris [QA] [:bogdan_maris] from comment #46)
> Is there a way I can manually verify this on Firefox desktop?

FWIW - the verifyme keyword was added to verify this on Firefox OS, not desktop. Firefox OS uses audio channels to manage sound and volume between different processes (e.g. allow user to change volume for content, handle cases where a ringtone & music file try to play at the same time). This wouldn't apply to desktop, as I don't think audio channels is implemented on desktop right now.
It works if you flip "media.useAudioChannelService" in about:config. I believe running a Web Audio demo and switching away to an other tab, making sure the sound is muted, should do the trick.
Reporter

Comment 49

6 years ago
(In reply to Paul Adenot (:padenot) from comment #48)
> It works if you flip "media.useAudioChannelService" in about:config. I
> believe running a Web Audio demo and switching away to an other tab, making
> sure the sound is muted, should do the trick.

Yes, although as Jason noted, we should verify this on Firefox OS.
Flags: needinfo?(ehsan)
Paul - Can you do some exploratory testing around this to verify this bug?
Flags: needinfo?(pyang)
Yes. leave ni? and I'll go back for this.
Build info:
Gaia:     959ac2f692d85072ffdc3d16a041b5bf4735ae59
Gecko:    http://hg.mozilla.org/mozilla-central/rev/aa986b6ce882
BuildID   20131010040202
Version   27.0a1


Quick test result:

App permission: audio-channel-normal
- Audio tag and dialer: audio can be heard but pretty low
- Audio tag and FM radio: both can be heard
- Audio tag and audio tag: both can be heard

App permission: audio-channel-content
- Audio tag and dialer: audio can be heard but pretty low
- Audio tag and FM radio: both can be heard
- Audio tag and audio tag: both can be heard
- Play same audio for many times: kanon
Flags: needinfo?(pyang)
Paul - that's an audio tag with a stream being used with Web Audio based APIs, right? Can you provide the test pages you used to test this?

FWIW - your initial testing implies this doesn't work at all, which is bad.
Flags: needinfo?(pyang)
Hi Paul, Can you explicitly describe which in the background and which in the foreground?
Eg. FM radio (content, background) + audio tag (normal, foreground) = both.
For content channel can be played in the background and foreground channel is always played.
Which Paul is that? I assume it's not me, but I'm slightly confused.
Flags: needinfo?(jwwang)
Sorry for causing the confusion. It is the Paul in comment 52.
Flags: needinfo?(jwwang)
(In reply to Jason Smith [:jsmith] from comment #53)
> Paul - that's an audio tag with a stream being used with Web Audio based
> APIs, right? Can you provide the test pages you used to test this?
> 
> FWIW - your initial testing implies this doesn't work at all, which is bad.

For test page:
 zapion.github.io/webaudio

(In reply to jwwang from comment #54)
> Hi Paul, Can you explicitly describe which in the background and which in
> the foreground?
> Eg. FM radio (content, background) + audio tag (normal, foreground) = both.
> For content channel can be played in the background and foreground channel
> is always played.

It's pauly | pyang :)

For audio-channel-normal
- Web audio is always foreground

For audio-channel-content
- I tested in both cases and result is the same.
Flags: needinfo?(pyang)
Marking as verified per pyang's testing above.
Depends on: 1044514
You need to log in before you can comment on or make changes to this bug.