Hook up DelayNode to the Media Streams backend

RESOLVED FIXED in mozilla23

Status

()

Core
Web Audio
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla23
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 728040 [details] [diff] [review]
Part 1: Refactor the code to convert GainNode's AudioParamTimeline to ticks into WebAudioUtils
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #728040 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
(Assignee)

Comment 2

5 years ago
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
Attachment #728040 - Flags: checkin+
(Assignee)

Comment 4

5 years ago
Created attachment 729337 [details] [diff] [review]
Part 2: Hook up delay nodes to the MSG

I plan on testing and make sure that we handle cycles correctly in a separate bug...
Attachment #729337 - Flags: review?(roc)
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.
(Assignee)

Comment 7

5 years ago
(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!
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
Created attachment 733043 [details] [diff] [review]
Part 2: Hook up delay nodes to the MSG

Addressed the previous review comments.  Also I was incorrectly doing k-rate sampling, switched to a-rate.
Attachment #729337 - Attachment is obsolete: true
Attachment #729337 - Flags: review?(roc)
Attachment #733043 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/5d1887ea9d43
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Assignee)

Comment 13

5 years ago
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.