Comment on attachment 728040 [details] [diff] [review] Part 1: Refactor the code to convert GainNode's AudioParamTimeline to ticks into WebAudioUtils https://hg.mozilla.org/integration/mozilla-inbound/rev/b42e344b20a9

I plan on testing and make sure that we handle cycles correctly in a separate bug...

Testcase/demo: http://people.mozilla.org/~eakhgari/webaudio/delay.html

Comment on attachment 729337 [details] [diff] [review] Part 2: Hook up delay nodes to the MSG Review of attachment 729337 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/DelayNode.cpp @@ +91,5 @@ > + { > + MOZ_ASSERT(mSource == aStream, "Invalid source stream"); > + > + if (aInput.IsNull()) { > + *aOutput = aInput; This isn't right. There could be nonzero buffered data we have to use here. @@ +100,5 @@ > + const uint32_t numChannels = aInput.mChannelData.Length(); > + MOZ_ASSERT(numChannels > 0); > + > + if (!EnsureBuffer(numChannels)) { > + aOutput->SetNull(0); This doesn't handle the number of channels changing between chunks. @@ +114,5 @@ > + if (mDelay.HasSimpleValue()) { > + delayTime = std::max(0.0, std::min(mMaxDelay, double(mDelay.GetValue()))); > + if (firstTime) { > + mCurrentDelayTime = delayTime; > + } Unclear why this is conditional on firstTime @@ +150,5 @@ > + readPosition -= bufferLength; > + } > + MOZ_ASSERT(readPosition >= 0.0, "Why are we reading before the beginning of the buffer?"); > + int readIndex1 = int(readPosition); > + int readIndex2 = (readIndex1 + 1) % bufferLength; I'm afraid of one or both of these values ending up out-of-bounds, referring to a wrapped-around sample, especially when readPosition is not an integer or the delay is changing. Can you prove that's not the case? @@ +164,5 @@ > + // processing each channel. If we're processing the last channel, let > + // the values persist for future calls to this method. > + if (channel < numChannels - 1) { > + mCurrentDelayTime = oldCurrentDelayTime; > + mWriteIndex = oldWriteIndex; This save-and-restore is a hack surely? We should not update these values until after this loop has finished. @@ +178,5 @@ > + // Circular buffer for capturing delayed samples. > + AutoFallibleTArray<FallibleTArray<float>, 2> mBuffer; > + // Write index for the buffer, to write the frames to the correct index of the buffer > + // given the current delay. > + uint32_t mWriteIndex; document the buffer setup better @@ +190,5 @@ > { > + DelayNodeEngine* engine = new DelayNodeEngine(aContext->Destination()); > + mStream = aContext->Graph()->CreateAudioNodeStream(engine, MediaStreamGraph::INTERNAL_STREAM); > + engine->SetSourceStream(static_cast<AudioNodeStream*> (mStream.get())); > + AudioNodeStream* ns = static_cast<AudioNodeStream*>(mStream.get()); catch the result of CreateAudioNodeStream in a local so we don't need to do these casts @@ +191,5 @@ > + DelayNodeEngine* engine = new DelayNodeEngine(aContext->Destination()); > + mStream = aContext->Graph()->CreateAudioNodeStream(engine, MediaStreamGraph::INTERNAL_STREAM); > + engine->SetSourceStream(static_cast<AudioNodeStream*> (mStream.get())); > + AudioNodeStream* ns = static_cast<AudioNodeStream*>(mStream.get()); > + ns->SetDoubleParameter(DelayNodeEngine::MAX_DELAY, aMaxDelay); Why is MAX_DELAY a settable parameter? you can just pass in aMaxDelay when we create the engine.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6) > Comment on attachment 729337 [details] [diff] [review] > Part 2: Hook up delay nodes to the MSG > > Review of attachment 729337 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/webaudio/DelayNode.cpp > @@ +91,5 @@ > > + { > > + MOZ_ASSERT(mSource == aStream, "Invalid source stream"); > > + > > + if (aInput.IsNull()) { > > + *aOutput = aInput; > > This isn't right. There could be nonzero buffered data we have to use here. Oh, you're right. > @@ +100,5 @@ > > + const uint32_t numChannels = aInput.mChannelData.Length(); > > + MOZ_ASSERT(numChannels > 0); > > + > > + if (!EnsureBuffer(numChannels)) { > > + aOutput->SetNull(0); > > This doesn't handle the number of channels changing between chunks. Yeah, sorry I was planning to fix this before submitting the patch. > @@ +114,5 @@ > > + if (mDelay.HasSimpleValue()) { > > + delayTime = std::max(0.0, std::min(mMaxDelay, double(mDelay.GetValue()))); > > + if (firstTime) { > > + mCurrentDelayTime = delayTime; > > + } > > Unclear why this is conditional on firstTime Further down we have this statement to handle smooth delay transitions: mCurrentDelayTime += (delayTime - mCurrentDelayTime) * smoothingRate; In order to make sure that code is valid for all cases, we need to make sure that mCurrentDelayTime is set the first time around to make that statement not increment mCurrentDelayTime. > @@ +150,5 @@ > > + readPosition -= bufferLength; > > + } > > + MOZ_ASSERT(readPosition >= 0.0, "Why are we reading before the beginning of the buffer?"); > > + int readIndex1 = int(readPosition); > > + int readIndex2 = (readIndex1 + 1) % bufferLength; > > I'm afraid of one or both of these values ending up out-of-bounds, referring > to a wrapped-around sample, especially when readPosition is not an integer > or the delay is changing. Can you prove that's not the case? The maximum value for bufferLength is 180*48000. The maximum value for mCurrentDelay is 180.0, so initially readPosition cannot be more than bufferLength + a fraction less than 1. Then we take care of that case by subtracting bufferLength from it if needed. So, if bufferLength-readPosition<1.0, readIndex1 will end up being zero. If 1.0<=bufferLength-readPosition<2.0, readIndex1 will be bufferLength-1 and readIndex2 will be 0. > @@ +164,5 @@ > > + // processing each channel. If we're processing the last channel, let > > + // the values persist for future calls to this method. > > + if (channel < numChannels - 1) { > > + mCurrentDelayTime = oldCurrentDelayTime; > > + mWriteIndex = oldWriteIndex; > > This save-and-restore is a hack surely? We should not update these values > until after this loop has finished. Hmm, I can modify the code to just increment local copies and assign them to the real member values with a similar condition. Would you prefer me to do that? > @@ +191,5 @@ > > + DelayNodeEngine* engine = new DelayNodeEngine(aContext->Destination()); > > + mStream = aContext->Graph()->CreateAudioNodeStream(engine, MediaStreamGraph::INTERNAL_STREAM); > > + engine->SetSourceStream(static_cast<AudioNodeStream*> (mStream.get())); > > + AudioNodeStream* ns = static_cast<AudioNodeStream*>(mStream.get()); > > + ns->SetDoubleParameter(DelayNodeEngine::MAX_DELAY, aMaxDelay); > > Why is MAX_DELAY a settable parameter? you can just pass in aMaxDelay when > we create the engine. Sure.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #7) > Hmm, I can modify the code to just increment local copies and assign them to > the real member values with a similar condition. Would you prefer me to do > that? Yes!

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #7) > > > + const uint32_t numChannels = aInput.mChannelData.Length(); > > > + MOZ_ASSERT(numChannels > 0); > > > + > > > + if (!EnsureBuffer(numChannels)) { > > > + aOutput->SetNull(0); > > > > This doesn't handle the number of channels changing between chunks. Actually we talked about this in the F2F and it's not clear what we want to do in that case yet, so I'd rather wait for https://www.w3.org/Bugs/Public/show_bug.cgi?id=21426 to be resolved before addressing this. I've filed bug 857610 to track that.

Addressed the previous review comments. Also I was incorrectly doing k-rate sampling, switched to a-rate.

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