Closed
Bug 874508
Opened 12 years ago
Closed 11 years ago
Reconcile Web Audio with audio channels
Categories
(Core :: Web Audio, defect)
Tracking
()
People
(Reporter: ehsan.akhgari, Assigned: baku)
References
Details
(Whiteboard: [blocking-webaudio-])
Attachments
(1 file, 10 obsolete files)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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•12 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•12 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•12 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•12 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•12 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•12 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•12 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•11 years ago
|
||
Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.
Component: Video/Audio → Web Audio
Comment 9•11 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•11 years ago
|
||
Andrea, can you take this please?
Reporter | ||
Updated•11 years ago
|
Whiteboard: [blocking-webaudio-]
Comment 12•11 years ago
|
||
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•11 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?
Comment 14•11 years ago
|
||
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+
Comment 15•11 years ago
|
||
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)
Comment 16•11 years ago
|
||
(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.
Comment 17•11 years ago
|
||
(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.
Comment 18•11 years ago
|
||
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•11 years ago
|
||
WIP
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #811921 -
Attachment is obsolete: true
Attachment #811999 -
Flags: review?(ehsan)
Reporter | ||
Comment 21•11 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•11 years ago
|
||
Attachment #811999 -
Attachment is obsolete: true
Attachment #812535 -
Flags: review?(ehsan)
Reporter | ||
Comment 23•11 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•11 years ago
|
||
Attachment #812535 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 25•11 years ago
|
||
Talking with Ehsan, we decided that we can implement this code differently.
Keywords: checkin-needed
Reporter | ||
Comment 26•11 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)
Yes!
Flags: needinfo?(roc)
Assignee | ||
Comment 28•11 years ago
|
||
Attachment #812912 -
Attachment is obsolete: true
Attachment #813584 -
Flags: review?(ehsan)
Reporter | ||
Comment 29•11 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•11 years ago
|
||
Attachment #813584 -
Attachment is obsolete: true
Attachment #813626 -
Flags: review?(ehsan)
Reporter | ||
Comment 31•11 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+
Assignee | ||
Comment 32•11 years ago
|
||
Attachment #813626 -
Attachment is obsolete: true
Comment 33•11 years ago
|
||
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
Assignee | ||
Comment 34•11 years ago
|
||
Assignee | ||
Comment 35•11 years ago
|
||
Comment 36•11 years ago
|
||
(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•11 years ago
|
||
Attachment #813660 -
Attachment is obsolete: true
Attachment #813711 -
Attachment is obsolete: true
Assignee | ||
Comment 38•11 years ago
|
||
Attachment #813792 -
Flags: review?(ehsan)
Assignee | ||
Updated•11 years ago
|
Attachment #813790 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #813792 -
Attachment is obsolete: true
Attachment #813792 -
Flags: review?(ehsan)
Assignee | ||
Comment 39•11 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•11 years ago
|
Flags: needinfo?(paul)
Assignee | ||
Comment 41•11 years ago
|
||
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•11 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+
Assignee | ||
Comment 43•11 years ago
|
||
nsIDocument.h is now removed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f35bab0828e0
Comment 44•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment 45•11 years ago
|
||
status-b2g-v1.2:
--- → fixed
status-firefox25:
--- → wontfix
status-firefox26:
--- → fixed
status-firefox27:
--- → fixed
Comment 46•11 years ago
|
||
Is there a way I can manually verify this on Firefox desktop?
Flags: needinfo?(ehsan)
Comment 47•11 years ago
|
||
(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.
Comment 48•11 years ago
|
||
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•11 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)
Comment 50•11 years ago
|
||
Paul - Can you do some exploratory testing around this to verify this bug?
Flags: needinfo?(pyang)
Comment 51•11 years ago
|
||
Yes. leave ni? and I'll go back for this.
Comment 52•11 years ago
|
||
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)
Comment 53•11 years ago
|
||
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)
Comment 54•11 years ago
|
||
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.
Comment 55•11 years ago
|
||
Which Paul is that? I assume it's not me, but I'm slightly confused.
Flags: needinfo?(jwwang)
Comment 56•11 years ago
|
||
Sorry for causing the confusion. It is the Paul in comment 52.
Flags: needinfo?(jwwang)
Comment 57•11 years ago
|
||
(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)
You need to log in
before you can comment on or make changes to this bug.
Description
•