849713 - Implement looping of AudioBufferSourceNode

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[(no longer active)]

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

Comment 1

[(no longer active)]

Attachment: Part 1: Add DOM bindings for AudioBufferSourceNode loop attributes

Comment 2

[(no longer active)]

Attachment: Part 2: Accept the loop parameters on AudioBufferSourceNodeEngine

Comment 3

[(no longer active)]

Attachment: Part 3: Set the loop parameters on AudioBufferSourceNodeEngine if looping is turned on when start() gets called

Comment 4

[(no longer active)]

Attachment: Part 4: Refactor the logic for borrowing the output buffer from the input buffer

Comment 5

[(no longer active)]

Attachment: Part 5: Implement the looping logic in AudioBufferSourceNodeEngine

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 7

[(no longer active)]

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

Comment 8

[(no longer active)]

(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.

Comment 9

[(no longer active)]

Attachment: Part 2: Accept the loop parameters on AudioBufferSourceNodeEngine

Comment 10

[(no longer active)]

Attachment: Part 5: Implement the looping logic in AudioBufferSourceNodeEngine

Done :-)

Comment 11

[(no longer active)]

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 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 13

[(no longer active)]

Attachment: Part 5: Implement the looping logic in AudioBufferSourceNodeEngine

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 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 17

[(no longer active)]

Attachment: Part 2: Accept the loop parameters on AudioBufferSourceNodeEngine

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

Comment 18

[(no longer active)]

Attachment: Part 5: Implement the looping logic in AudioBufferSourceNodeEngine

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 724444 [details] [diff] [review]
Part 5: Implement the looping logic in AudioBufferSourceNodeEngine

Review of attachment 724444 [details] [diff] [review]:
-----------------------------------------------------------------

Yay!

Comment 20

[(no longer active)]

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

Comment 21

[Ed Morley [:emorley]]

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

Comment 22

[(no longer active)]

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