I have a set of patches which implement looping on AudioBufferSourceNode. I'm in the process of testing and cleaning them up.

Comment on attachment 723581 [details] [diff] [review] Part 2: Accept the loop parameters on AudioBufferSourceNodeEngine Review of attachment 723581 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioBufferSourceNode.cpp @@ +132,5 @@ > nsRefPtr<ThreadSharedFloatArrayBufferList> mBuffer; > int32_t mOffset; > int32_t mDuration; > + // 0 for false, <0 if we will loop, >0 if we are looping > + int32_t mLoop; If this is really a tristate, can we use an enum?

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6) > Comment on attachment 723581 [details] [diff] [review] > Part 2: Accept the loop parameters on AudioBufferSourceNodeEngine > > Review of attachment 723581 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/webaudio/AudioBufferSourceNode.cpp > @@ +132,5 @@ > > nsRefPtr<ThreadSharedFloatArrayBufferList> mBuffer; > > int32_t mOffset; > > int32_t mDuration; > > + // 0 for false, <0 if we will loop, >0 if we are looping > > + int32_t mLoop; > > If this is really a tristate, can we use an enum? Sure. Gimme a few minutes.

Done :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/3551877d9b92 https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6552161eff https://hg.mozilla.org/integration/mozilla-inbound/rev/26aa58e87d5d https://hg.mozilla.org/integration/mozilla-inbound/rev/e811d4258d45 https://hg.mozilla.org/integration/mozilla-inbound/rev/151b845b0fa9 (I made the mistake to think that part 5 is reviewed too..., so I backed that part out for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ae88e9925b)

Comment on attachment 723594 [details] [diff] [review] Part 5: Implement the looping logic in AudioBufferSourceNodeEngine Review of attachment 723594 [details] [diff] [review]: ----------------------------------------------------------------- This code seems more complex than necessary. It might be simpler to structure the code as a while loop that runs until all samples have been assembled into a WEBAUDIO_BLOCK_SIZE buffer. At each step of the loop we figure out where we should be reading from --- either the "before loop" audio segment or the "in loop" audio segment, or else we've reached the end --- and how much we can read sequentially. If in the first iteration it turns out we can read the entire WEBAUDIO_BLOCK_SIZE buffer, then we can do the borrowing optimization. Otherwise we copy the data into our buffer and advance by the length. Grab me on IRC if that doesn't make sense. Also the memcpys that extract parts of channels should be abstracted into a helper function. ::: content/media/webaudio/AudioBufferSourceNode.cpp @@ +100,5 @@ > aOutput->SetNull(WEBAUDIO_BLOCK_SIZE); > return; > } > + > + // Loop until stop() has not been called This comment doesn't seem right. We loop until stop() has been called, right?

This version of the patch is definitely not shorter, but it covers some edge cases that the previous version doesn't, and having had to debug both versions, it's much simpler to debug since the logic is a lot easier to follow at the cost of being a bit more verbose.

Comment on attachment 723795 [details] [diff] [review] Part 5: Implement the looping logic in AudioBufferSourceNodeEngine Review of attachment 723795 [details] [diff] [review]: ----------------------------------------------------------------- typo: "write silende" I still think this can be simplified a lot. How about something like this: FillWithZeroes(AudioChunk*, uint32_t aOffsetWithinBlock, TrackTicks* aCurrentPos, TrackTicks aMaxPos) { // fill AudioChunk with zeroes at offset aOffsetWithinBlock, using SetNull if possible // otherwise allocating a block if necessary. Advance *aOffsetWithinBlock and *aCurrentPos // by the amount written; don't advance *aOffsetWithinBlock beyond end of // WEBAUDIO_BLOCK_SIZE or *aCurrentPos beyond aMaxPos. } CopyFromBuffer(AudioChunk*, uint32_t aOffsetWithinBlock, TrackTicks* aCurrentPos, uint32_t aBufferOffset, uint32_t aBufferMax) { // fill AudioChunk with the data from the sample buffer, borrowing contents if possible // otherwise allocating a block if necessary. Advance *aOffsetWithinBlock and *aCurrentPos // by the amount written; don't advance *aOffsetWithinBlock beyond end of // WEBAUDIO_BLOCK_SIZE. Take data from buffer starting at aBufferOffset, and don't // take data beyond aBufferMax. } uint32_t written = 0; // within the block TrackTicks currentPosition = ...; while (written < WEBAUDIO_BLOCK_SIZE) { if (mStop is valid && currentPosition >= mStop) { FillWithZeroes(aOutput, &written, &currentPosition, INT_MAX); continue; } if (currentPosition < mStart) { FillWithZeroes(aOutput, &written, &currentPosition, mStart); continue; } TrackTicks t = currentPosition - mStart; if (!loopingEnabled) { if (offset + t < mDuration) { CopyFromBuffer(aOutput, &written, &currentPosition, offset + t, mDuration); } else { FillWithZeroes(aOutput, &written, &currentPosition, INT_MAX); } } else { if (offset + t < mLoopEnd) { CopyFromBuffer(aOutput, &written, &currentPosition, offset + t, mLoopEnd); } else { uint32_t offsetInLoop = (offset + t - mLoopEnd) % (mLoopEnd - mLoopStart); CopyFromBuffer(aOutput, &written, &currentPosition, offsetInLoop + mLoopStart, mLoopEnd); } } } if this works, then we don't even need to track whether we "will loop" or "are looping", we can tell from the current time. ::: content/media/webaudio/AudioBufferSourceNode.cpp @@ +116,5 @@ > return; > } > > + uint32_t written = 0; > + bool wroteAllBeforeStop = false; It's not really clear what this means. @@ +136,5 @@ > + sizeToWrite = WEBAUDIO_BLOCK_SIZE; > + fillWithZeroes = true; > + findSourceOffset = false; > + } else { > + TrackTicks endTime = std::min(mStart + mDuration, mStop); This shadows endTime above.

In that pseudocode, FillWithZeroes and CopyFromBuffer are responsible for figuring out how much data can be copied/zeroed. One thing I forgot to mention is that CopyFromBuffer needs to make sure it doesn't advance beyond mStop, in addition to its other constraints.

At the end of that while loop you'd want to explicitly determine whether we've finished or not, by checking whether we've advanced beyond mStop or looping is disabled and we've reached the end of mDuration.

New part 2. In this version, mLoop is a simple boolean member.

Comment on attachment 724444 [details] [diff] [review] Part 5: Implement the looping logic in AudioBufferSourceNodeEngine Review of attachment 724444 [details] [diff] [review]: ----------------------------------------------------------------- Yay!

https://hg.mozilla.org/integration/mozilla-inbound/rev/f74273a94c5b https://hg.mozilla.org/integration/mozilla-inbound/rev/3df9da356bdc https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb8aa2f50e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/ac78a9192951 https://hg.mozilla.org/integration/mozilla-inbound/rev/c7382fcb77a4

https://hg.mozilla.org/mozilla-central/rev/f74273a94c5b https://hg.mozilla.org/mozilla-central/rev/3df9da356bdc https://hg.mozilla.org/mozilla-central/rev/fdb8aa2f50e9 https://hg.mozilla.org/mozilla-central/rev/ac78a9192951 https://hg.mozilla.org/mozilla-central/rev/c7382fcb77a4

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