This patch applies on top of the "resample off main thread" and "implement playbackRate" patches. We need to check which AudioBufferSourceNode is connected to the a PannerNode at each connection/deconnection, because it can be than mutating the graph without touching the AudioBufferSourceNode nor the PannerNode results in a different AudioBufferSourceNode connected to a PannerNode: Source1 ---> Gain1 ----> Gain2 ----> Panner1 ----> Destination | Source2 ---> Gain3 ----> Gain4 ----> Panner2 ----------- We swap Gain2 and Gain4: Source1 ---> Gain1 ----> Gain4 ----> Panner2 ----> Destination | Source2 ---> Gain3 ----> Gain2 ----> Panner1 ----------- A rudimentaty graph traversal/cycle detection has been implemented, that seem to work, but it takes quite a bit of node book keeping in the context to get it right. Maybe it can be done in another, cleaner, way. Also, there is a spec issue: What panner should change the |playbackRate| of the source here? There might both a different velocity. Source1 ----> Panner1 ----> Destination | | `---> Panner2 --------' (The other spec issue being the reason why the doppler shift does not apply to sources other than AudioBufferSourceNode, I'm going to notify the group for both questions).

Comment on attachment 733774 [details] [diff] [review] Implement the doppler part of AudioPannerNode. r= Review of attachment 733774 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioBufferSourceNode.cpp @@ +292,5 @@ > } > return mStart + mPosition; > } > > + int32_t ComputeFinalOutSampleRate() Nit: const. @@ +297,5 @@ > + { > + return static_cast<uint32_t>(IdealAudioRate() / (mPlaybackRate * mDopplerShift)); > + } > + > + bool ShouldResample() Nit: I'm a const freak, you should know that by now. ;-) @@ +528,5 @@ > +void > +AudioBufferSourceNode::SendDopplerShiftToStream(double aDopplerShift) > +{ > + AudioNodeStream* ns = static_cast<AudioNodeStream*>(mStream.get()); > + ns->SetDoubleParameter(AudioBufferSourceNodeEngine::DOPPLERSHIFT, aDopplerShift); Nit: use AudioNode::SendDoubleParameterToStream. ::: content/media/webaudio/AudioBufferSourceNode.h @@ +33,5 @@ > { > return true; > } > > + virtual AudioBufferSourceNode* AsAudioBufferSourceNode() { Nit: MOZ_OVERRIDE. ::: content/media/webaudio/AudioContext.cpp @@ +184,5 @@ > +void > +AudioContext::UnregisterAudioBufferSourceNode(AudioBufferSourceNode* aNode) > +{ > + bool rv = mAudioBufferSourceNodes.RemoveElement(aNode); > + MOZ_ASSERT(rv); Hmm, I think you should remove these assertions. nsTArray::RemoveElement will always return true (and we should make it return void at some point.) ::: content/media/webaudio/AudioContext.h @@ +40,5 @@ > > class AudioBuffer; > class AudioBufferSourceNode; > class AudioDestinationNode; > +class AudioPannerNode; We don't have this class! @@ +137,5 @@ > nsRefPtr<AudioDestinationNode> mDestination; > nsRefPtr<AudioListener> mListener; > MediaBufferDecoder mDecoder; > nsTArray<nsAutoPtr<WebAudioDecodeJob> > mDecodeJobs; > + // Two arrays containing all the AudioPannerNodes and AudioBufferSourceNodes, Nit: PannerNodes. ::: content/media/webaudio/AudioListener.h @@ +79,5 @@ > mPosition.z = aZ; > SendThreeDPointParameterToStream(PannerNode::LISTENER_POSITION, mPosition); > } > > + ThreeDPoint& Position() Nit: const. @@ +105,5 @@ > SendThreeDPointParameterToStream(PannerNode::LISTENER_ORIENTATION, mOrientation); > SendThreeDPointParameterToStream(PannerNode::LISTENER_UPVECTOR, mUpVector); > } > > + ThreeDPoint& Velocity() Ditto. ::: content/media/webaudio/AudioNode.h @@ +137,5 @@ > UpdateOutputEnded(); > } > } > > + nsTArray<InputNode>& InputNodes() { Nit: const. ::: content/media/webaudio/PannerNode.cpp @@ +509,5 @@ > + if (!(mListenerVelocity.IsZero() && mVelocity.IsZero())) { > + for(uint32_t i = 0; i < mSources.Length(); i++) { > + mSources[i]->SendDopplerShiftToStream(ComputeDopplerShift()); > + } > + } Please move this block to a helper on PannerNode, and also add a helper to AudioListener which calls this helper on all non-null mPanners weak pointers. Then call it explicitly where needed. ::: content/media/webaudio/PannerNode.h @@ +37,5 @@ > return true; > } > > + virtual void SendThreeDPointParameterToStream(uint32_t aIndex, const ThreeDPoint& aValue); > + virtual void SendDoubleParameterToStream(uint32_t aIndex, double aValue); Note that these AudioNode methods are not virtual so declaring them virtual here just hides them and will not do the right thing. Also note that these methods are meant to be used as convenience methods, and no complicated logic should live in them. They should merely pass stuff down to the AudioNodeStream. @@ +219,5 @@ > + > + ThreeDPoint mListenerVelocity; > + ThreeDPoint mListenerPosition; > + double mListenerDopplerFactor; > + double mListenerSpeedOfSound; Why do you need to store copies of these here? You should just be able to do Listener()->Velocity() etc where needed. ::: content/media/webaudio/WebAudioUtils.h @@ +46,5 @@ > */ > static void ConvertAudioParamToTicks(AudioParamTimeline& aParam, > AudioNodeStream* aDest); > + > + static void FixNaNs(double& aDouble) This name made me think that this method accepts an array. Please use FixNaN.

Does this spec change mean any change to your implementation? (I think no, but just wanted to check) https://dvcs.w3.org/hg/audio/rev/5c21ad646174

No, it does not change anything implementation-wise.

Backed out because of mochitest-1 crashes: https://hg.mozilla.org/integration/mozilla-inbound/rev/d65cf43c4717

This was green on try, so I relanded: https://tbpl.mozilla.org/?tree=Try&rev=ce40dac09c9b https://hg.mozilla.org/integration/mozilla-inbound/rev/1165f67ccfb5

Great, thanks! Do you have a test case for this?

If you are talking about an automated test case, I'm not sure how feasible it is until we have the script node, because the doppler shift is not exposed to content. Maybe using the analyzer node, I'll see what I can do. I also tried to make a loop, and then I noticed we don't have the delay node, so we can't do a loop and have a valid graph. But I agree this needs testing. On the other hand, if you just want to check how cool it is, go to [1], press "start", open the console, and do a `ctx.listener.setVelocity(x, y, z)` or a `node.setVelocity(x1, y1, z1)`, for different value of x, y, and z, hear the playback rate change. [1]: http://paul.cx/public/panner.html

(In reply to comment #10) > If you are talking about an automated test case, I'm not sure how feasible it > is until we have the script node, Yeah, I meant a manual test case. > because the doppler shift is not exposed to > content. Maybe using the analyzer node, I'll see what I can do. I also tried to > make a loop, and then I noticed we don't have the delay node, so we can't do a > loop and have a valid graph. We do have DelayNode! (Although I haven't tested the looping quite well yet.) > But I agree this needs testing. > > On the other hand, if you just want to check how cool it is, go to [1], press > "start", open the console, and do a `ctx.listener.setVelocity(x, y, z)` or a > `node.setVelocity(x1, y1, z1)`, for different value of x, y, and z, hear the > playback rate change. > > [1]: http://paul.cx/public/panner.html I think I'll steal this test case and extend it a bit... Thanks!

Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.