I have a set of patches which hook up GainNode to the MSG back-end. They depend on the patches in bug 804387.

Comment on attachment 707858 [details] [diff] [review] Part 2: Make it possible to send AudioEventTimeline objects as commands to the MSG thread Review of attachment 707858 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioNodeEngine.h @@ +122,5 @@ > { > NS_ERROR("Invalid SetInt32Parameter index"); > } > + virtual void SetTimelineParameter(uint32_t aIndex, > + const dom::AudioEventTimeline<ErrorResult>& aValue) Can't we typedef this so that we don't have an explicit ErrorResult parameter flying around?

Comment on attachment 707859 [details] [diff] [review] Part 3: Make AudioParams call back into their owning node when they're modified Review of attachment 707859 [details] [diff] [review]: ----------------------------------------------------------------- Instead of making the callback parameter a run-time parameter, how about making it a template parameter to AudioParam?

Also I think the callback should pass the AudioParam in so the callee can know which param was modified.

Comment on attachment 707860 [details] [diff] [review] Part 4 - Hook up GainNode to the MediaStreams back-end to enable controlling the volume of the audio being played back Review of attachment 707860 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioNodeEngine.cpp @@ +70,5 @@ > > +void > +AudioBlockCopyChannelWithVectorScale(const float aInput[WEBAUDIO_BLOCK_SIZE], > + const float aScale[WEBAUDIO_BLOCK_SIZE], > + float aOutput[WEBAUDIO_BLOCK_SIZE]) Don't need the word "Vector" in this name. ::: content/media/webaudio/GainNode.cpp @@ +43,5 @@ > + { > + *aOutput = aInput; > + if (mGain.HasSimpleValue()) { > + // Optimize the case where we only have a single value set as the volume > + aOutput->mVolume = mGain.GetValue(); You need to multiply by the original volume @@ +51,5 @@ > + // the same way we do in AudioNodeStream::SetStreamTimeParameterImpl > + // in fact you should probably convert all the times to track-ticks for speedier processing > + // that code converts times from "relative to the reference stream > + // (destination node of the audio context)" to graph time, then to > + // TrackTicks for the target stream This comment is confusing. You could cleanly separate what we currently do from what we should do. @@ +54,5 @@ > + // (destination node of the audio context)" to graph time, then to > + // TrackTicks for the target stream > + > + // Make sure to set the output volume to 1.0 > + aOutput->mVolume = 1.f; You might as well use the original volume here. Certainly it needs to be taken into account. @@ +64,5 @@ > + float computedGain[WEBAUDIO_BLOCK_SIZE]; > + for (size_t counter = 0; counter < WEBAUDIO_BLOCK_SIZE; ++counter) { > + TrackTicks tick = aStream->GetCurrentPosition() + counter; > + StreamTime streamTime = TicksToTimeRoundDown(sampleRate, tick); > + computedGain[counter] = mGain.GetValueAtTime(MediaTimeToSeconds(streamTime)); This doesn't account for the case where this stream's 0 time is different from the context's destination node's 0 time. I think it would be best to have the engine's timeline use units of TrackTicks and convert to TrackTicks in a new method AudioNodeStream::SetTimelineParameterImpl. @@ +103,5 @@ > + uint32_t aInput, ErrorResult& aRv) > +{ > + AudioNode::Connect(aDestination, aOutput, aInput, aRv); > + > + SendGainToStream(this); Why do you need to do this here?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8) > Also I think the callback should pass the AudioParam in so the callee can > know which param was modified. Actually I guess this is not needed.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6) > Comment on attachment 707858 [details] [diff] [review] > Part 2: Make it possible to send AudioEventTimeline objects as commands to > the MSG thread > > Review of attachment 707858 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/AudioNodeEngine.h > @@ +122,5 @@ > > { > > NS_ERROR("Invalid SetInt32Parameter index"); > > } > > + virtual void SetTimelineParameter(uint32_t aIndex, > > + const dom::AudioEventTimeline<ErrorResult>& aValue) > > Can't we typedef this so that we don't have an explicit ErrorResult > parameter flying around? Sure, there's already that typedef in AudioParam.h, as it turns out!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7) > Comment on attachment 707859 [details] [diff] [review] > Part 3: Make AudioParams call back into their owning node when they're > modified > > Review of attachment 707859 [details] [diff] [review]: > ----------------------------------------------------------------- > > Instead of making the callback parameter a run-time parameter, how about > making it a template parameter to AudioParam? That won't work. I talked to Boris about this today. The summary is that we have to have the same prototype chain for all AudioParams on the JS side, which means that the polymorphism to determine what kind of AudioParam we're dealing with has to happen on the C++ side at runtime. So the options are either using a virtual function or a callback function. The advantage of using callback functions over virtual functions (which is why I picked up this solution) is that for node types which hold on to multiple AudioParams, we won't need to create weird inheritance chains just to be able to override the callback virtual function. In terms of cost, the callback is pretty much a virtual function implemented by me instead of the compiler, so the cost is a dynamic function call. Additionally, I don't like the idea of templatizing AudioParam for other reasons such as code bloat, unneeded ugliness, etc. but they don't matter anyway I guess. What do you think?

Gah, I promised to CC Boris... Please see the last comment.

Comment on attachment 707860 [details] [diff] [review] Part 4 - Hook up GainNode to the MediaStreams back-end to enable controlling the volume of the audio being played back Review of attachment 707860 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/GainNode.cpp @@ +43,5 @@ > + { > + *aOutput = aInput; > + if (mGain.HasSimpleValue()) { > + // Optimize the case where we only have a single value set as the volume > + aOutput->mVolume = mGain.GetValue(); Oh, right... @@ +51,5 @@ > + // the same way we do in AudioNodeStream::SetStreamTimeParameterImpl > + // in fact you should probably convert all the times to track-ticks for speedier processing > + // that code converts times from "relative to the reference stream > + // (destination node of the audio context)" to graph time, then to > + // TrackTicks for the target stream lol, I just pasted in what you told me on IRC so that I don't forget it the next day, and then I forgot to replace it with an actual comment. Embarrassing! @@ +64,5 @@ > + float computedGain[WEBAUDIO_BLOCK_SIZE]; > + for (size_t counter = 0; counter < WEBAUDIO_BLOCK_SIZE; ++counter) { > + TrackTicks tick = aStream->GetCurrentPosition() + counter; > + StreamTime streamTime = TicksToTimeRoundDown(sampleRate, tick); > + computedGain[counter] = mGain.GetValueAtTime(MediaTimeToSeconds(streamTime)); I'm not sure if I quite understand the second sentence... @@ +103,5 @@ > + uint32_t aInput, ErrorResult& aRv) > +{ > + AudioNode::Connect(aDestination, aOutput, aInput, aRv); > + > + SendGainToStream(this); Hmm, I guess I don't. I added this to capture the case where the gain AudioParam is not touched before we connect the node, but in that case the default value will be 1.0, so we can just not do anything here.

(In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #13) > In terms of cost, the callback is pretty > much a virtual function implemented by me instead of the compiler, so the > cost is a dynamic function call. OK, sounds reasonable.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9) > I think it would be best to have the engine's timeline use units of > TrackTicks and convert to TrackTicks in a new method > AudioNodeStream::SetTimelineParameterImpl. What I mean is that on the engine side, an event timeline should have all its time measurements in TrackTicks instead of doubles-relative-to-the-context-destination-time.

(In reply to comment #16) > (In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #13) > > In terms of cost, the callback is pretty > > much a virtual function implemented by me instead of the compiler, so the > > cost is a dynamic function call. > > OK, sounds reasonable. So, r+? ;-)

(In reply to comment #17) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9) > > I think it would be best to have the engine's timeline use units of > > TrackTicks and convert to TrackTicks in a new method > > AudioNodeStream::SetTimelineParameterImpl. > > What I mean is that on the engine side, an event timeline should have all its > time measurements in TrackTicks instead of > doubles-relative-to-the-context-destination-time. Hmm, is that really worth spending time on now? I will very soon rewrite all of this to just precompute the buffer inside AudioEventTimeline...

What you have there currently is completely wrong if the GainNode is added after the AudioContext has started playing. Do you really want to land in that state?

Does this correctly fix the graph time issue?

No. You've converted to graph time but you really need to be converting to the time relative to the destination node. But it's definitely going to be easier to convert the timeline to be in ticks, or at least GainNode's stream time.

(In reply to comment #22) > No. You've converted to graph time but you really need to be converting to the > time relative to the destination node. But it's definitely going to be easier > to convert the timeline to be in ticks, or at least GainNode's stream time. I don't think that I have a clear idea on how to implement either of these two solutions... I think at this point I'm quite confused by the different time coordinates, and how to convert between them correctly. A question about changing the timeline to be in ticks: is that enough to just loop through the events and call some magic function which takes an AudioEventTimeline time and returns a tick and store that in the timeline object somehow? If yes, do we have all of the information necessary to do this when passing that object to the MSG thread? That I think would be the ideal time to do this, to make sure that we don't needlessly complicate the logic inside GainNodeEngine::ProduceAudioBlock.

(In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #23) > I don't think that I have a clear idea on how to implement either of these > two solutions... I think at this point I'm quite confused by the different > time coordinates, and how to convert between them correctly. There are three time coordinate systems: 1) The MediaStreamGraph. Zero is the time when the graph was created (from the point of view of the MSG thread). 2) The AudioContext destination node's stream. Zero is the time that node was created (from the point of view of the MSG thread). 3) The GainNode's stream. Zero is the time that node was created (from the point of view of the MSG thread). > A question about changing the timeline to be in ticks: is that enough to > just loop through the events and call some magic function which takes an > AudioEventTimeline time and returns a tick and store that in the timeline > object somehow? If yes, do we have all of the information necessary to do > this when passing that object to the MSG thread? That I think would be the > ideal time to do this, to make sure that we don't needlessly complicate the > logic inside GainNodeEngine::ProduceAudioBlock. Yes, that's what I want us to do :-).

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24) > (In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #23) > > I don't think that I have a clear idea on how to implement either of these > > two solutions... I think at this point I'm quite confused by the different > > time coordinates, and how to convert between them correctly. > > There are three time coordinate systems: > 1) The MediaStreamGraph. Zero is the time when the graph was created (from > the point of view of the MSG thread). > 2) The AudioContext destination node's stream. Zero is the time that node > was created (from the point of view of the MSG thread). > 3) The GainNode's stream. Zero is the time that node was created (from the > point of view of the MSG thread). I assume that #3 is what aStream->GetCurrentPosition() returns, right? What is the correct way to get times #1 and #2, and convert between them from inside ProduceAudioBlock? > > A question about changing the timeline to be in ticks: is that enough to > > just loop through the events and call some magic function which takes an > > AudioEventTimeline time and returns a tick and store that in the timeline > > object somehow? If yes, do we have all of the information necessary to do > > this when passing that object to the MSG thread? That I think would be the > > ideal time to do this, to make sure that we don't needlessly complicate the > > logic inside GainNodeEngine::ProduceAudioBlock. > > Yes, that's what I want us to do :-). Great. I'll look into this when I get back from my vacation, I guess.

Pushed parts 1-3: https://hg.mozilla.org/integration/mozilla-inbound/rev/901c735274bc https://hg.mozilla.org/integration/mozilla-inbound/rev/51f7c9e31f9a https://hg.mozilla.org/integration/mozilla-inbound/rev/d567b1bb34ad

And backed out the patches since bug 804387 is getting backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/7f08319059b3

https://hg.mozilla.org/integration/mozilla-inbound/rev/7841a1527c4f https://hg.mozilla.org/integration/mozilla-inbound/rev/f12bd589624f https://hg.mozilla.org/integration/mozilla-inbound/rev/69b18d52cfdc

https://hg.mozilla.org/mozilla-central/rev/7841a1527c4f https://hg.mozilla.org/mozilla-central/rev/f12bd589624f https://hg.mozilla.org/mozilla-central/rev/69b18d52cfdc

createGainNode seems to be the last mile for getting things bleeping?

If you flip the Web Audio pref you should be able to play sound already just using AudioBufferSourceNodes.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31) > If you flip the Web Audio pref you should be able to play sound already just > using AudioBufferSourceNodes. Got that far, which is all kinds of awesome! I was also looking at a number of libs / examples and they tend to depend on createGainNode as a first step, eg Howler: https://github.com/goldfire/howler.js/blob/master/howler.js#L28 Really stoked about this stuff, thanks for all the hard work :)

(In reply to comment #32) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31) > > If you flip the Web Audio pref you should be able to play sound already just > > using AudioBufferSourceNodes. > > Got that far, which is all kinds of awesome! I was also looking at a number of > libs / examples and they tend to depend on createGainNode as a first step, eg > Howler: > > https://github.com/goldfire/howler.js/blob/master/howler.js#L28 > > Really stoked about this stuff, thanks for all the hard work :) Yeah, createGainNode works just fine, but it's not yet hooked up to anything. This is on the top of my todo list as soon as I find a few hours away from the other stuff that keeps coming up. :-)

(In reply to :Ehsan Akhgari from comment #33) ... > Yeah, createGainNode works just fine, but it's not yet hooked up to > anything. This is on the top of my todo list as soon as I find a few hours > away from the other stuff that keeps coming up. :-) Ah, nice. I'm really looking forward to all sort of JS Audio libraries suddenly working great on Firefox.

With an additional test fix

Comment on attachment 720826 [details] [diff] [review] Part 5 - Provide an API for converting event times to tick values Review of attachment 720826 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioEventTimeline.h @@ +88,5 @@ > > +template <> > +inline double AudioTimelineEvent::Time<double>() const > +{ > + return mTime; I think you should add a DebugOnly flag to track which format the time is in and assert if we ask for the wrong one.

Comment on attachment 720788 [details] [diff] [review] Part 6 - Hook up GainNode to the MediaStreams back-end to enable controlling the volume of the audio being played back Review of attachment 720788 [details] [diff] [review]: ----------------------------------------------------------------- Very nice.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39) > Comment on attachment 720826 [details] [diff] [review] > Part 5 - Provide an API for converting event times to tick values > > Review of attachment 720826 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/AudioEventTimeline.h > @@ +88,5 @@ > > > > +template <> > > +inline double AudioTimelineEvent::Time<double>() const > > +{ > > + return mTime; > > I think you should add a DebugOnly flag to track which format the time is in > and assert if we ask for the wrong one. Good idea, will do.

https://hg.mozilla.org/integration/mozilla-inbound/rev/db2247a7ad09 https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c12a1fd1dd https://hg.mozilla.org/integration/mozilla-inbound/rev/9539c3f2112b

https://hg.mozilla.org/mozilla-central/rev/db2247a7ad09 https://hg.mozilla.org/mozilla-central/rev/f6c12a1fd1dd https://hg.mozilla.org/mozilla-central/rev/9539c3f2112b

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