Currently we don't do this for many params, such as the buffer, looping attributes, etc.

Comment on attachment 740351 [details] [diff] [review] Part 1: Send the AudioBufferSourceNode loop parameter changes to the stream Review of attachment 740351 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/test/test_audioBufferSourceNodeLoop.html @@ +36,5 @@ > + source.connect(sp); > + sp.connect(context.destination); > + sp.onaudioprocess = function(e) { > + is(e.inputBuffer.numberOfChannels, 1, "input buffer must have only one channel"); > + dump(e.inputBuffer.getChannelData(0)[2047]); Maybe this is a leftover dump that should be removed? ::: content/media/webaudio/test/test_audioBufferSourceNodeLoopStartEnd.html @@ +44,5 @@ > + source.connect(sp); > + sp.connect(context.destination); > + sp.onaudioprocess = function(e) { > + is(e.inputBuffer.numberOfChannels, 1, "input buffer must have only one channel"); > + dump(e.inputBuffer.getChannelData(0)[2047]); Same here.

Yeah, both of these were debugging dumps, sorry, forgot to take them out!

Comment on attachment 740351 [details] [diff] [review] Part 1: Send the AudioBufferSourceNode loop parameter changes to the stream https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac63545817c

I think this is all that I needed to do here.

Comment on attachment 740460 [details] [diff] [review] Part 2: Send the AudioBufferSourceNode buffer parameter changes to the stream Review of attachment 740460 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioBufferSourceNode.cpp @@ +492,5 @@ > +AudioBufferSourceNode::SendOffsetAndDurationParametersToStream(AudioNodeStream* aStream, > + double aOffset, > + double aDuration) > +{ > + MOZ_ASSERT(mBuffer, "How come we don't have a buffer here?"); Consider this javascript code: var ctx = new AudioContext(); var source = ctx.createBufferSource(); source.start(100, 100, 100); source.buffer = null; This assert blows up. ::: content/media/webaudio/AudioBufferSourceNode.h @@ +82,3 @@ > { > mBuffer = aBuffer; > + SendBufferParameterToStream(aCx); Maybe: if (mBuffer) { SendBufferParameterToStream(aCx); }

You're completely right, good catch! Patch + test for this case coming up.

(In reply to comment #7) > ::: content/media/webaudio/AudioBufferSourceNode.h > @@ +82,3 @@ > > { > > mBuffer = aBuffer; > > + SendBufferParameterToStream(aCx); > > Maybe: > > if (mBuffer) { > SendBufferParameterToStream(aCx); > } No, that would make it a no-op to set the buffer to null, wouldn't it?

There is a problem I did not catch during the review in the first patch, if we set the params and then the buffer, and then calling start, because |SendLoopParametersToStream| is no-op when |mBuffer| is null. This patch fixes the issue by evaluating the parameters before sending them really lazy.

Comment on attachment 740779 [details] [diff] [review] followup - Also send the params to the stream when setting the buffer. This is addressed in you last patch, nevermind :-)

Comment on attachment 740779 [details] [diff] [review] followup - Also send the params to the stream when setting the buffer. Man, I can't read. This is still needed.

Now rebased on top of your patches, and with a test.

Comment on attachment 740847 [details] [diff] [review] followup - Also send the params to the stream when setting the buffer. Review of attachment 740847 [details] [diff] [review]: ----------------------------------------------------------------- Very good catch! ::: content/media/webaudio/test/webaudio.js @@ +41,5 @@ > var difference = 0; > var maxDifference = 0; > var firstBadIndex = -1; > for (var i = offset || 0; i < Math.min(buf1.length, (offset || 0) + length); ++i) { > + dump("("+i+")[" + buf1[i+sourceOffset] + " " + buf2[i+destOffset] + "]\n"); Gah, I meant to take this console.log out! Can you please take the dump out too? Doing this will make things very sad on our test machine since you'll get one line of test spew per frame examined here!

Comment on attachment 740779 [details] [diff] [review] followup - Also send the params to the stream when setting the buffer. This is obsolete, AFAICT.

Comment on attachment 740777 [details] [diff] [review] Part 2: Send the AudioBufferSourceNode buffer parameter changes to the stream Feel free to land this if you r+ it without comments with your own patch here. Thanks!

Landed both patches: https://hg.mozilla.org/integration/mozilla-inbound/rev/05cec4050393 https://hg.mozilla.org/integration/mozilla-inbound/rev/979da3f33fa3

https://hg.mozilla.org/mozilla-central/rev/05cec4050393 https://hg.mozilla.org/mozilla-central/rev/979da3f33fa3

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