My plan to implement this is to special case AudioParam's which have AudioNode inputs by giving them an AudioNodeStream which takes the AudioNodeStream of all of their input nodes as inputs. We need to add the AudioParam stream in this way in order to get proper stream ordering. The stream for the AudioParam would be an input to the node that the AudioParam belongs to. We need to do this in order to get proper stream ordering. With the above, all that we should need to do is to special case AudioNodeStream::ObtainInputBlock to ignore the streams belonging to AudioParams, and in the engine code where we want to sample an AudioParam, we will just mix its stream's mLastChunk with the value read from the AudioParam. Does this make sense to you roc?

Yes, that is exactly what I was thinking.

Demo: http://people.mozilla.org/~eakhgari/webaudio/nodeToParam.html Be warned. This sounds absolutely terrible! ;-)

Comment on attachment 744419 [details] [diff] [review] Part 1: Make it possible to connect an AudioNode to an AudioParam Review of attachment 744419 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioParam.h @@ +131,5 @@ > CallbackType mCallback; > const float mDefaultValue; > + // For every InputNode, there is a corresponding entry in mOutputParams of the > + // InputNode's mInputNode. > + nsTArray<AudioNode::InputNode> mInputNodes; Move this up next to mNode so all pointer-things are together

Comment on attachment 744420 [details] [diff] [review] Part 2: Give each AudioParam that is connected to an AudioNode an AudioNodeStream Review of attachment 744420 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioNode.cpp @@ +215,5 @@ > + static_cast<ProcessedMediaStream*>(audioParamNodeStream); > + > + // Setup the AudioParam's stream as an input to the owner AudioNode's stream > + input->mNodeStreamPort = audioParamNodePS->AllocateInputPort(stream, > + MediaInputPort::FLAG_BLOCK_INPUT); So this means if there are N connections into the AudioParam, the AudioParam's stream is also connected N times into its AudioNode. That seems unnecessary. ::: content/media/webaudio/AudioParam.cpp @@ +87,5 @@ > + > +MediaStream* > +AudioParam::Stream() > +{ > + if (!mStream) { Just write if (mStream) { return; } to save indenting @@ +91,5 @@ > + if (!mStream) { > + AudioNodeEngine* engine = new AudioNodeEngine(nullptr); > + nsRefPtr<AudioNodeStream> stream = mNode->Context()->Graph()->CreateAudioNodeStream(engine, MediaStreamGraph::INTERNAL_STREAM); > + > + // Force the input to have only one channels, and make it down-mix using "channel"

Comment on attachment 744422 [details] [diff] [review] Part 3: Mix in the value generated by AudioNode inputs to AudioParams when getting their values during audio processing Review of attachment 744422 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioParamTimeline.h @@ +44,5 @@ > + return BaseClass::HasSimpleValue() && !mStream; > + } > + > + template<class TimeType> > + float GetValueAtTime(TimeType aTime, size_t aCounter = 0) const Need to document what aCounter means here. @@ +50,5 @@ > + MOZ_ASSERT(aCounter < WEBAUDIO_BLOCK_SIZE); > + MOZ_ASSERT(!aCounter || !HasSimpleValue()); > + > + // Mix the value of the AudioParam itself with that of the AudioNode inputs. > + return BaseClass::GetValueAtTime(aTime) + Hang on ... shouldn't you be adding aCounter to aTime here? The changes you made to DelayNode/GainNode suggest so.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > @@ +50,5 @@ > > + MOZ_ASSERT(aCounter < WEBAUDIO_BLOCK_SIZE); > > + MOZ_ASSERT(!aCounter || !HasSimpleValue()); > > + > > + // Mix the value of the AudioParam itself with that of the AudioNode inputs. > > + return BaseClass::GetValueAtTime(aTime) + > > Hang on ... shouldn't you be adding aCounter to aTime here? The changes you > made to DelayNode/GainNode suggest so. Oops, great catch! Filed bug 867876 to help me save face next time! :-)

Rebased.

Comment on attachment 744445 [details] [diff] [review] Part 2: Give each AudioParam that is connected to an AudioNode an AudioNodeStream Review of attachment 744445 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioNode.cpp @@ +210,5 @@ > + input->mStreamPort = ps->AllocateInputPort(mStream, MediaInputPort::FLAG_BLOCK_INPUT); > + > + // Setup the AudioParam's stream as an input to the owner AudioNode's stream > + // if this is the first connection to the AudioParam. > + if (aDestination.InputNodes().Length() == 1) { Won't this check mean that if we add a connection into the AudioParam, then remove it, then add another connection into the AudioParam, we'll have the AudioParam's stream feeding into the AudioNode twice? Maybe AudioParam::Stream() could take responsibility for connecting a newly-created stream to the AudioNode?

(In reply to comment #15) > Comment on attachment 744445 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=744445 > Part 2: Give each AudioParam that is connected to an AudioNode an > AudioNodeStream > > Review of attachment 744445 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/webaudio/AudioNode.cpp > @@ +210,5 @@ > > + input->mStreamPort = ps->AllocateInputPort(mStream, MediaInputPort::FLAG_BLOCK_INPUT); > > + > > + // Setup the AudioParam's stream as an input to the owner AudioNode's stream > > + // if this is the first connection to the AudioParam. > > + if (aDestination.InputNodes().Length() == 1) { > > Won't this check mean that if we add a connection into the AudioParam, then > remove it, then add another connection into the AudioParam, we'll have the > AudioParam's stream feeding into the AudioNode twice? I don't think so. AudioParam::InputNode's destructor removes the input port, so when you remove the connection, the port will get destroyed, and when you get a new connection a new one will be created. Unless I'm missing something! > Maybe AudioParam::Stream() could take responsibility for connecting a > newly-created stream to the AudioNode? I would really like to not do that. That is super fragile, since it would mean that if somebody calls AudioParam::Stream() _before_ calling AudioParam::AppendInputNode, then things will blow up in our face. Plus, it makes AudioParam::Stream() contain more "random" logic than it already does. I don't see what's wrong with the current setup.

Comment on attachment 744445 [details] [diff] [review] Part 2: Give each AudioParam that is connected to an AudioNode an AudioNodeStream I think roc is away until next week, but I'd like to make progress here. Paul, do you mind reviewing this? I will happily address any other comments that roc has here post landing.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #16) > I don't think so. AudioParam::InputNode's destructor removes the input > port, so when you remove the connection, the port will get destroyed, and > when you get a new connection a new one will be created. Unless I'm missing > something! Well then, if you add two AudioNodes feeding into the AudioParam, then remove the first one, there will be no connection between the AudioParam's MediaStream and the destination AudioNode's MediaStream. Right? > > Maybe AudioParam::Stream() could take responsibility for connecting a > > newly-created stream to the AudioNode? > > I would really like to not do that. That is super fragile, since it would > mean that if somebody calls AudioParam::Stream() _before_ calling > AudioParam::AppendInputNode, then things will blow up in our face. Plus, it > makes AudioParam::Stream() contain more "random" logic than it already does. > I don't see what's wrong with the current setup. I think of AudioParam has having a lazily-created MediaStream which is connected as an input to it's AudioNode's MediaStream. That connection isn't used for anything other than ordering the stream graph. So whenever we create the AudioParam's MediaStream, it makes sense to connect it to the AudioNode's MediaStream. I don't see the problem here.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #16) > > I don't think so. AudioParam::InputNode's destructor removes the input > > port, so when you remove the connection, the port will get destroyed, and > > when you get a new connection a new one will be created. Unless I'm missing > > something! > > Well then, if you add two AudioNodes feeding into the AudioParam, then > remove the first one, there will be no connection between the AudioParam's > MediaStream and the destination AudioNode's MediaStream. Right? Hmm, yeah you're right. > > > Maybe AudioParam::Stream() could take responsibility for connecting a > > > newly-created stream to the AudioNode? > > > > I would really like to not do that. That is super fragile, since it would > > mean that if somebody calls AudioParam::Stream() _before_ calling > > AudioParam::AppendInputNode, then things will blow up in our face. Plus, it > > makes AudioParam::Stream() contain more "random" logic than it already does. > > I don't see what's wrong with the current setup. > > I think of AudioParam has having a lazily-created MediaStream which is > connected as an input to it's AudioNode's MediaStream. That connection isn't > used for anything other than ordering the stream graph. So whenever we > create the AudioParam's MediaStream, it makes sense to connect it to the > AudioNode's MediaStream. I don't see the problem here. OK, I guess I can do that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dde95c320e08 https://hg.mozilla.org/integration/mozilla-inbound/rev/6c0be12d00aa https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0cd25b6611 https://hg.mozilla.org/integration/mozilla-inbound/rev/42ced245b6f8

https://hg.mozilla.org/mozilla-central/rev/dde95c320e08 https://hg.mozilla.org/mozilla-central/rev/6c0be12d00aa https://hg.mozilla.org/mozilla-central/rev/8d0cd25b6611 https://hg.mozilla.org/mozilla-central/rev/42ced245b6f8

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