Closed Bug 849713 Opened 7 years ago Closed 7 years ago

Implement looping of AudioBufferSourceNode

Categories

(Core :: Web Audio, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(5 files, 5 obsolete files)

3.96 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.95 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.61 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.51 KB, patch
Details | Diff | Splinter Review
8.74 KB, patch
roc
: review+
Details | Diff | Splinter Review
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?
(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.
Attachment #723581 - Attachment is obsolete: true
Attachment #723581 - Flags: review?(roc)
Attachment #723592 - Flags: review?(roc)
Done :-)
Attachment #723588 - Attachment is obsolete: true
Attachment #723588 - Flags: review?(roc)
Attachment #723594 - Flags: review?(roc)
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.
Attachment #723594 - Attachment is obsolete: true
Attachment #723594 - Flags: review?(roc)
Attachment #723795 - Flags: review?(roc)
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.
Attachment #723592 - Attachment is obsolete: true
Attachment #723795 - Attachment is obsolete: true
Attachment #723795 - Flags: review?(roc)
Attachment #724444 - Flags: review?(roc)
Whiteboard: [leave open]
Comment on attachment 724444 [details] [diff] [review]
Part 5: Implement the looping logic in AudioBufferSourceNodeEngine

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

Yay!
Attachment #724444 - Flags: review?(roc) → review+
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
You need to log in before you can comment on or make changes to this bug.