Closed
Bug 1100349
Opened 9 years ago
Closed 9 years ago
Implement StereoPannerNode
Categories
(Core :: Web Audio, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: padenot, Assigned: padenot)
References
(Depends on 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(4 files, 4 obsolete files)
1.44 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
6.98 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
28.09 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
10.46 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
There is a new node in the Web Audio API, that is a simple equal-power stereo panner, for musical applications rather than games/simulation. Spec: http://webaudio.github.io/web-audio-api/#the-stereopannernode-interface
Updated•9 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•9 years ago
|
||
Ehsan, this is a new node that's been added to the spec to try to make the panning situation in Web Audio API better. It's basically like the PannerNode with equal-power, but with a [-1; +1] AudioParam (full-left -> full-right), that is better for musical applications (PannerNode is better for 3d games). A bit more background on why it's needed can be found on the intent-to-implement and ship on blink-dev: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/IVDmh6ubHZM, where I outlined the why and how we are fixing this. The spec is there: http://webaudio.github.io/web-audio-api/#the-stereopannernode-interface I clarified the spec a bit (it was enough to implement, but could have use some clarification, especially that pan is an a-rate AudioParam), and it might not have landed in the spec when you look at this patch, so here the PR that fixes it: https://github.com/WebAudio/web-audio-api/pull/440
Attachment #8524692 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8524692 [details] [diff] [review] Implement StereoPannerNode. r= smaug, I need a DOM reviewer for this. This is adding a new interface, StereoPannerNode. The spec is here: http://webaudio.github.io/web-audio-api/#the-stereopannernode-interface
Attachment #8524692 -
Flags: review?(bugs)
Assignee | ||
Comment 3•9 years ago
|
||
Some tests, testing the DOM bits and the different branches that existe: stereo/mono input, automated/non-automated.
Attachment #8524694 -
Flags: review?(ehsan.akhgari)
Comment 4•9 years ago
|
||
Comment on attachment 8524692 [details] [diff] [review] Implement StereoPannerNode. r= (I believe ehsan will become a DOM peer real soon ;) )
Attachment #8524692 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 5•9 years ago
|
||
This fixes an issue caused by the small refactoring on the panning function.
Attachment #8525301 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Updated•9 years ago
|
Attachment #8524692 -
Attachment is obsolete: true
Attachment #8524692 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 6•9 years ago
|
||
It appears this is necessary when adding a new interface.
Attachment #8525303 -
Flags: review?(bugs)
Comment 7•9 years ago
|
||
Comment on attachment 8525303 [details] [diff] [review] imported patch stereo-panner-test-interfaces Yup, to indicate patch authors that a new thing is exposed in the global scope.
Attachment #8525303 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 8•9 years ago
|
||
Apparently, the previous version busted non-unified builds.
Attachment #8525404 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Updated•9 years ago
|
Attachment #8525301 -
Attachment is obsolete: true
Attachment #8525301 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 9•9 years ago
|
||
And this removes a issue with the test on try.
Attachment #8525407 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Updated•9 years ago
|
Attachment #8524694 -
Attachment is obsolete: true
Attachment #8524694 -
Flags: review?(ehsan.akhgari)
Comment 10•9 years ago
|
||
Comment on attachment 8525404 [details] [diff] [review] Implement StereoPannerNode. r= Review of attachment 8525404 [details] [diff] [review]: ----------------------------------------------------------------- Please send an intent to ship email to dev-platform about this if you haven't already (sorry, I'm way behind my email!) ::: dom/media/webaudio/AudioNodeEngine.cpp @@ +246,5 @@ > + float aOutputL[WEBAUDIO_BLOCK_SIZE], > + float aOutputR[WEBAUDIO_BLOCK_SIZE]) > +{ > +#ifdef BUILD_ARM_NEON > + // No NEON version yet Please file a follow-up for this and include the bug# here. @@ +256,5 @@ > + *aOutputL++ = *aInputL++ + *aInputR * *aGainL++; > + *aOutputR++ = *aInputR++ * *aGainR++; > + } else { > + *aOutputL++ = *aInputL * *aGainL++; > + *aOutputR++ = *aInputR++ + *aInputL++ * *aGainR++; Can you please index all of these arrays using i instead of incrementing the pointers? This is super error prone. :-) ::: dom/media/webaudio/AudioNodeEngine.h @@ -158,5 @@ > - * Vector copy-scaled operation. > - */ > -void AudioBlockCopyChannelWithScale(const float aInput[WEBAUDIO_BLOCK_SIZE], > - const float aScale[WEBAUDIO_BLOCK_SIZE], > - float aOutput[WEBAUDIO_BLOCK_SIZE]); Couldn't you keep using the array version? ::: dom/media/webaudio/PanningUtils.h @@ +16,5 @@ > +template<typename T> > +void > +GainMonoToStereo(const AudioChunk& aInput, AudioChunk* aOutput, > + T aGainL, T aGainR) > +{ Nit: please MOZ_ASSERT the expected channel counts on aInput and aOutput. @@ +27,5 @@ > + > +template<typename T, typename U> > +void > +GainStereoToStereo(const AudioChunk& aInput, AudioChunk* aOutput, > + T aGainL, T aGainR, U aOnLeft) Please document that U can be expected to be either a bool or an array of bools here and further down. @@ +28,5 @@ > +template<typename T, typename U> > +void > +GainStereoToStereo(const AudioChunk& aInput, AudioChunk* aOutput, > + T aGainL, T aGainR, U aOnLeft) > +{ Ditto. ::: dom/media/webaudio/StereoPannerNode.cpp @@ +29,5 @@ > + > +class StereoPannerNodeEngine : public AudioNodeEngine > +{ > +public: > + explicit StereoPannerNodeEngine(AudioNode* aNode, Nit: explicit only makes sense if the ctor can be called with one argument. @@ +37,5 @@ > + , mDestination(static_cast<AudioNodeStream*>(aDestination->Stream())) > + // Keep the default value in sync with the default value in > + // StereoPannerNode::StereoPannerNode. > + , mPan(0.f) > + Nit: please remove the empty line! @@ +70,5 @@ > + float& aLeftGain, > + float& aRightGain) > + { > + // Clamp and normalize the panning in [0; 1] > + aPanning = std::min(std::max(aPanning, -1.f), 1.f); Please make sure that this normalization is spec'ed. I couldn't find the spec text for this. @@ +76,5 @@ > + if (aMonoToStereo) { > + aPanning += 1; > + aPanning /= 2; > + } else { > + if (aPanning <= 0) { Nit: else if. @@ +94,5 @@ > + MOZ_ASSERT(mSource == aStream, "Invalid source stream"); > + > + // The output of this node is always stereo, no matter what the inputs are. > + AllocateAudioBlock(2, aOutput); > + bool monoToStereo = aInput.ChannelCount() == 1; Please MOZ_ASSERT that aInput's channels cannot be greater than 2. @@ +98,5 @@ > + bool monoToStereo = aInput.ChannelCount() == 1; > + > + if (aInput.IsNull()) { > + // If input is silent, so is the output > + aOutput->SetNull(WEBAUDIO_BLOCK_SIZE); If the output is always meant to be stereo, then this is incorrect, since AudioChunk::SetNull creates a silent mono chunk. Please test the desired behavior here too. @@ +104,5 @@ > + float panning = mPan.GetValue(); > + // If the panning is 0.0, we can simply copy the input to the > + // output: this node is a no-op > + if (panning == 0.0f) { > + *aOutput = aInput; This will only be correct if aInput is stereo, right? Please add a test to cover the case when it's not. @@ +110,5 @@ > + // Optimize the case where the panning is constant for this processing > + // block: we can just apply a constant gain on the left and right > + // channel > + float gainL, gainR; > + float value = mPan.GetValue(); Use |panning| instead? @@ +112,5 @@ > + // channel > + float gainL, gainR; > + float value = mPan.GetValue(); > + GetGainValuesForPanning(value, monoToStereo, gainL, gainR); > + ApplyStereoPanning(aInput, aOutput, gainL, gainR, value <= 0); Hmm, you're losing aInput.mVolume here, aren't you? Please add a test that would have caught this. @@ +119,5 @@ > + float computedGain[2][WEBAUDIO_BLOCK_SIZE]; > + bool onLeft[WEBAUDIO_BLOCK_SIZE]; > + > + for (size_t counter = 0; counter < WEBAUDIO_BLOCK_SIZE; ++counter) { > + TrackTicks tick = aStream->GetCurrentPosition(); Nit: StreamTime. @@ +137,5 @@ > + > + virtual size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE > + { > + return AudioNodeEngine::SizeOfExcludingThis(aMallocSizeOf); > + } Is there any reason to override this function? @@ +171,5 @@ > +size_t > +StereoPannerNode::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const > +{ > + size_t amount = AudioNode::SizeOfExcludingThis(aMallocSizeOf); > + amount += mPan->SizeOfExcludingThis(aMallocSizeOf); Did you mean to use SizeOfIncludingThis here? ::: dom/webidl/AudioContext.webidl @@ +41,5 @@ > optional unsigned long numberOfInputChannels = 2, > optional unsigned long numberOfOutputChannels = 2); > > [NewObject] > + StereoPannerNode createStereoPanner(); Sigh... I really wish we'd start to look into using constructors for new audio nodes at least. (Not minusing because of this!) ::: dom/webidl/StereoPannerNode.webidl @@ +9,5 @@ > + * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C > + * liability, trademark and document use rules apply. > + */ > + > +interface StereoPannerNode : AudioNode { Please make this node implement AudioNodePassThrough, and add a test to that effect. See bug 1007778 where I implemented that.
Attachment #8525404 -
Flags: review?(ehsan.akhgari) → review-
Comment 11•9 years ago
|
||
Comment on attachment 8525407 [details] [diff] [review] Tests for the StereoPannerNode. r= Review of attachment 8525407 [details] [diff] [review]: ----------------------------------------------------------------- (Note that the test needs to be extended to cover the issues I found in the patch...) ::: dom/media/webaudio/test/test_stereoPannerNode.html @@ +55,5 @@ > +is(stereoPanner.channelCount, 2, "StereoPannerNode node has 2 input channels by default"); > +is(stereoPanner.channelCountMode, "clamped-max", "Correct channelCountMode for the StereoPannerNode"); > +is(stereoPanner.channelInterpretation, "speakers", "Correct channelCountInterpretation for the StereoPannerNode"); > +expectException(function() { > + stereoPanner.channelCount = 3; So, what happens if you set the channelCount to 1 instead? Shouldn't we be throwing for all values expect 2? @@ +166,5 @@ > + var panner = ac.createStereoPanner(); > + panner.connect(ac.destination); > + var expected = f(ac, panner); > + ac.oncomplete = function(e) { > + ok(true, f.name); Nit: use info() instead.
Attachment #8525407 -
Flags: review?(ehsan.akhgari) → review+
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10) > @@ +70,5 @@ > > + float& aLeftGain, > > + float& aRightGain) > > + { > > + // Clamp and normalize the panning in [0; 1] > > + aPanning = std::min(std::max(aPanning, -1.f), 1.f); > > Please make sure that this normalization is spec'ed. I couldn't find the > spec text for this. We are addressing this at a global level in the spec: each AudioParam will have a "nominal range", and values will be clamped to this nominal range (I think the PR has not landed yet). In practice, this is what we do (note that the range can be [-Infinity; +Infinity], for example for the GainNode.gain AudioParam) most of the time, I'll audit and fix Gecko if it's not the case.
Comment 13•9 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #12) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #10) > > @@ +70,5 @@ > > > + float& aLeftGain, > > > + float& aRightGain) > > > + { > > > + // Clamp and normalize the panning in [0; 1] > > > + aPanning = std::min(std::max(aPanning, -1.f), 1.f); > > > > Please make sure that this normalization is spec'ed. I couldn't find the > > spec text for this. > > We are addressing this at a global level in the spec: each AudioParam will > have a "nominal range", and values will be clamped to this nominal range (I > think the PR has not landed yet). Fantastic! > In practice, this is what we do (note that > the range can be [-Infinity; +Infinity], for example for the GainNode.gain > AudioParam) most of the time, I'll audit and fix Gecko if it's not the case. Thanks for doing this. This is indeed one of the old issues with the spec, so it's very nice to see some traction here!
Assignee | ||
Comment 14•9 years ago
|
||
Addressed review comments.
Attachment #8530932 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Updated•9 years ago
|
Attachment #8525404 -
Attachment is obsolete: true
Assignee | ||
Comment 15•9 years ago
|
||
Added tests for the bugs fixed in this iteration of the patch. I'm not entirely sure how to test the mono/stereo silence issue in a mochitest, it would get upmixed at input, so it's not really observable, right?
Attachment #8530941 -
Flags: review?(ehsan.akhgari)
Comment 16•9 years ago
|
||
Comment on attachment 8530932 [details] [diff] [review] Implement StereoPannerNode. r= Review of attachment 8530932 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below fixed. ::: dom/media/webaudio/AudioNodeEngine.cpp @@ +253,5 @@ > + uint32_t i; > + for (i = 0; i < WEBAUDIO_BLOCK_SIZE; i++) { > + if (aIsOnTheLeft[i]) { > + aOutputL[i] = aInputL[i] + *aInputR * *aGainL++; > + aOutputR[i] = aInputR[i] * *aGainR++; Perhaps do the same for aGainL and aGainR referenced above too? ::: dom/media/webaudio/StereoPannerNode.cpp @@ +84,5 @@ > + } > + > + void SetToSilentStereoBlock(AudioChunk* aChunk) > + { > + AllocateAudioBlock(2, aChunk); The buffer is already allocated here, so you can remove this. @@ +134,5 @@ > + // Optimize the case where the panning is constant for this processing > + // block: we can just apply a constant gain on the left and right > + // channel > + float gainL, gainR; > + float panning = mPan.GetValue(); There is a panning variable in the outer scope :) That's what I meant, sorry for not being specific. ::: dom/webidl/StereoPannerNode.webidl @@ +17,5 @@ > +// Mozilla extension > +partial interface StereoPannerNode { > + [ChromeOnly] > + attribute boolean passThrough; > +}; Instead of this, you can just say: StereoPannerNode implements AudioNodePassThrough;
Attachment #8530932 -
Flags: review?(ehsan.akhgari) → review+
Comment 17•9 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #15) > Created attachment 8530941 [details] [diff] [review] > Tests for the StereoPannerNode. r= > > Added tests for the bugs fixed in this iteration of the patch. > > I'm not entirely sure how to test the mono/stereo silence issue in a > mochitest, > it would get upmixed at input, so it's not really observable, right? Perhaps use a ChannelSplitterNode? (That part can be done in a separate bug if you want.)
Updated•9 years ago
|
Attachment #8530941 -
Flags: review?(ehsan.akhgari) → review+
Assignee | ||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/61277c6b999e https://hg.mozilla.org/integration/mozilla-inbound/rev/d112575f39f9
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d112575f39f9 https://hg.mozilla.org/mozilla-central/rev/61277c6b999e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 20•8 years ago
|
||
I've documented this feature: https://developer.mozilla.org/en-US/docs/Web/API/StereoPannerNode https://developer.mozilla.org/en-US/docs/Web/API/StereoPannerNode.pan https://developer.mozilla.org/en-US/docs/Web/API/AudioContext.createStereoPanner A quick tech review would be great when you get the chance. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
Comment 21•8 years ago
|
||
Also there is a note in: https://developer.mozilla.org/en-US/Firefox/Releases/37#Interfaces.2FAPIs.2FDOM
Summary: Implement StereoPanner → Implement StereoPannerNode
Updated•8 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•