noteGrainOn is too long

• Component: Web Audio
• Status: NEW ---

[Raymond Toy]

As discovered in the note-grain-on-timing test in bug 76659, noteGrainOn generates a signal that is longer than the specified duration.  This is caused by AudioBufferSourceNode::renderFromBuffer always adding 512 samples to the end to allow for HRTF tail-time, according to the fixme comment.  This should be fixed.

[Chris Rogers]

This should be handled more generally.

[Raymond Toy]

(In reply to comment #1)
> This should be handled more generally.

Can you describe this a little bit more?

[Chris Rogers]

Sorry, I'll try to explain a bit better.

This is a work-around for the latency in HRTFPanner processing (assuming that there is an AudioPannerNode in the processing chain after AudioBufferSourceNode).

If there is a panner and the extra 512 sample-frames are not generated in AudioBufferSourceNode, then the nodes get torn down immediately when the grain finishes playing.  But, because the HRTFPanner incurs a processing latency, it still has unprocessed sample-data which has not yet been output.  And so the very end of the rendered signal is truncated and can cause glitches.

Ideally, the handling would be more general than just HRTFPanner, and would handle *any* node which incurs processing latency.  We should talk about this offline to consider the best solution.

[Raymond Toy]

Adding dependency to 74750.  Once the tailTime feature is added, we can build on that to remove the extra 512 samples from noteGrainOn, and fix the HRTF panner to continue to output samples after the input has been disconnected.

[Raymond Toy]

Add dependency on 74533.

[Brady Eidson]

(In reply to comment #5)
> Add dependency on 74533.

A very curious dependency, as 74533 was a crasher in code not at all related to Web Audio, and has already been resolved.

[Raymond Toy]

Oops.  That's 74553, not 74533.

[Raymond Toy]

Created attachment 135899 [details]
Patch

[Raymond Toy]

Please review this preliminary patch.  Ignore the debugging stuff for now. I want to try this patch on another platform and having the debugging stuff will help if I find a problem.

This patch depends on the patch for 74553; it won't work without 74553.

Also ignore the patch for revolver which was needed so that I could run in debug mode.  (That bug is fixed in another patch, already.)

[Raymond Toy]

One additional thing.  This patch implements the tail time processing for all nodes that have a non-zero tail time or latency time, in addition to fixing noteGrainOn() being too long.

[Chris Rogers]

Comment on attachment 135899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135899&action=review

Hi Ray, thanks for the early patch.  I'll have to have Jer's patch landed first before I can go deeper here.  Also, we should probably talk about the logic offline to work this out more quickly.

> Source/WebCore/platform/audio/ReverbConvolverStage.cpp:77
> +    }

Please update sources.  This change in ReverbConvolverStage.cpp should not be part of this patch.

> Source/WebCore/webaudio/AudioNode.cpp:40
> +#endif

Please remove debugging code.  For your own testing purposes, you can keep these locally in a git branch (for example).

> Source/WebCore/webaudio/AudioNode.cpp:192
> +            endTailProcessingIfNeeded();

I'm going to have to wait until Jer's patch lands before fully being able to understand this logic.

> Source/WebCore/webaudio/AudioNode.cpp:370
> +        // Establish a ref to ourselves so we don't get removed when the inputs go away. We are now

"when the inputs go away" -> "when all nodes have disconnected from us"

> Source/WebCore/webaudio/AudioNode.cpp:375
> +#endif

Please remove debugging code.  For your own testing purposes, you can keep these locally in a git branch (for example).

> Source/WebCore/webaudio/AudioNode.cpp:391
> +#endif

Please remove debugging code.  For your own testing purposes, you can keep these locally in a git branch (for example).

> Source/WebCore/webaudio/AudioNode.cpp:405
> +#endif

Please remove debugging code.  For your own testing purposes, you can keep these locally in a git branch (for example).

> Source/WebCore/webaudio/AudioNode.cpp:407
> +        deref(RefTypeConnection);

If you have a method that calls deref() directly then you should have (at the top of the method:
    ASSERT(context()->isGraphOwner());

> Source/WebCore/webaudio/AudioNode.h:34
> +#define DEBUG_AUDIONODE_TAIL 0

Please remove debugging code.  For your own testing purposes, you can keep these locally in a git branch (for example).


DEBUG_AUDIONODE_REFERENCES also should probably not be in AudioNode.h - let's not pile on more debugging conditionals

> Source/WebCore/webaudio/AudioNode.h:159
> +    // one at least input is outputting silence. This will create an additional connection ref to

typo: grammar/wording is funny here.

"input is outputting silence" -> "input is receiving silence"

Shouldn't the logic be when ALL inputs are receiving silence, not "at least one"?

> Source/WebCore/webaudio/AudioNodeInput.cpp:67
> +    node()->resetTailProcessingIfNeeded();

I don't understand why this logic is here.  Why should silent processing logic be affected if somebody connects to us?

> Source/WebCore/webaudio/AudioNodeInput.cpp:113
> +    // If an input to a node is disabled, we want to enable tail processing for the nodes that have

nit: "inputs" are not disabled, instead "connections" (from AudioNodeOutputs) are disabled.  There's a difference between an "input" to a node and connections (which can be multiple) to that input.

I don't understand why we should start tail processing just because one particular AudioNodeOutput is calling our disable() method.  What if there are ten other AudioNodeOutputs still connected to us?

[Raymond Toy]

Comment on attachment 135899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135899&action=review

Just a few comments that might clarify things.  If not, we'll discuss this in person which will be easier.

>> Source/WebCore/platform/audio/ReverbConvolverStage.cpp:77
>> +    }
> 
> Please update sources.  This change in ReverbConvolverStage.cpp should not be part of this patch.

I didn't do that due to the move of the webaudio files to the Modules directory.  Otherwise I would have.

>> Source/WebCore/webaudio/AudioNodeInput.cpp:67
>> +    node()->resetTailProcessingIfNeeded();
> 
> I don't understand why this logic is here.  Why should silent processing logic be affected if somebody connects to us?

Yes, you're right.  In the original version where I had a countdown for tail processing, I wanted to reset the counter and state.  With the current implementation, it doesn't matter because we don't count down the tail frames anymore.  (It wasn't needed in the original version either, I think.)

>> Source/WebCore/webaudio/AudioNodeInput.cpp:113
>> +    // If an input to a node is disabled, we want to enable tail processing for the nodes that have
> 
> nit: "inputs" are not disabled, instead "connections" (from AudioNodeOutputs) are disabled.  There's a difference between an "input" to a node and connections (which can be multiple) to that input.
> 
> I don't understand why we should start tail processing just because one particular AudioNodeOutput is calling our disable() method.  What if there are ten other AudioNodeOutputs still connected to us?

Perhaps the name is bad.  It's really more like enable processing.  The actual tail processing doesn't happen until processIfNecessary is passing silence through.  This just establishes a reference to ourselves so we don't go away before processing the tail.  I don't think we can wait to until processIfNecessary to establish the reference because the node will have already been disabled by then.

[Raymond Toy]

Comment on attachment 135899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135899&action=review

> Source/WebCore/webaudio/AudioNodeInput.cpp:114
> +    // tail or latency time.

Fixed.  It shouldn't be here and it works just fine running it in processIfNecessary.

[Raymond Toy]

Created attachment 136738 [details]
Patch

[Raymond Toy]

Comment on attachment 135899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135899&action=review

>>> Source/WebCore/platform/audio/ReverbConvolverStage.cpp:77
>>> +    }
>> 
>> Please update sources.  This change in ReverbConvolverStage.cpp should not be part of this patch.
> 
> I didn't do that due to the move of the webaudio files to the Modules directory.  Otherwise I would have.

Updated (and therefore removed).

>> Source/WebCore/webaudio/AudioNode.cpp:40
>> +#endif
> 
> Please remove debugging code.  For your own testing purposes, you can keep these locally in a git branch (for example).

Removed.

>> Source/WebCore/webaudio/AudioNode.cpp:370
>> +        // Establish a ref to ourselves so we don't get removed when the inputs go away. We are now
> 
> "when the inputs go away" -> "when all nodes have disconnected from us"

Fixed.

>> Source/WebCore/webaudio/AudioNode.cpp:375
>> +#endif
> 
> Please remove debugging code.  For your own testing purposes, you can keep these locally in a git branch (for example).

Done.

>> Source/WebCore/webaudio/AudioNode.cpp:391
>> +#endif
> 
> Please remove debugging code.  For your own testing purposes, you can keep these locally in a git branch (for example).

Done.

>> Source/WebCore/webaudio/AudioNode.cpp:405
>> +#endif
> 
> Please remove debugging code.  For your own testing purposes, you can keep these locally in a git branch (for example).

Done.

>> Source/WebCore/webaudio/AudioNode.cpp:407
>> +        deref(RefTypeConnection);
> 
> If you have a method that calls deref() directly then you should have (at the top of the method:
>     ASSERT(context()->isGraphOwner());

As discussed, we can't do that. We call addDeferredFinishDeref() instead.

>> Source/WebCore/webaudio/AudioNode.h:34
>> +#define DEBUG_AUDIONODE_TAIL 0
> 
> Please remove debugging code.  For your own testing purposes, you can keep these locally in a git branch (for example).
> 
> 
> DEBUG_AUDIONODE_REFERENCES also should probably not be in AudioNode.h - let's not pile on more debugging conditionals

Removed.

>> Source/WebCore/webaudio/AudioNode.h:159
>> +    // one at least input is outputting silence. This will create an additional connection ref to
> 
> typo: grammar/wording is funny here.
> 
> "input is outputting silence" -> "input is receiving silence"
> 
> Shouldn't the logic be when ALL inputs are receiving silence, not "at least one"?

Fixed.

[Chris Rogers]

Comment on attachment 136738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136738&action=review

Looks pretty good overall, but I have one question about AudioNode lifetime.  Since an AudioNode can now ref() itself, what happens in the case when the AudioContext is uninitialized (when the page unloads), but there are still some AudioNodes sitting around still processing tails.  In this case AudioNode::processIfNecessary() will never have a chance to get called again enough times to fully empty the tails, so endTailProcessingIfNeeded() will never get called and the derefs() will never happen.

So I think we're going to have to keep track of these "special" nodes and add a final cleanup method which we call in AudioContext::uninitialize().

Looking at this a little closer, I think you may be able to leverage AudioContext::refNode() and AudioContext::derefUnfinishedSourceNodes().  Then the only tricky thing left is that you can't *directly* call AudioContext::refNode() in the processIfNecessaryMethod()

> Source/WebCore/Modules/webaudio/AudioNode.cpp:216
> +        allowTailProcessingIfNeeded();

WebKit style: avoid redundant comments - I would just remove the comments on lines 214:215 since the name of the method itself says the same thing.
The comment in the 2nd sentence is already included in the comments in the method impl itself

Also, shouldn't this line be moved down to just above line 223 (call to process())?  This is because we don't want to call this *until* we have received non-silent input

> Source/WebCore/Modules/webaudio/AudioNode.cpp:398
> +        // Establish a ref to ourselves if we haven't already so we don't get removed when all nodes

"ourselves" -> "ourself"

> Source/WebCore/Modules/webaudio/AudioNode.cpp:400
> +        // actually processing the tail. That happens when all of the inputs become silent.

Part of this comment can be made a little more clear:

"We can process the tail, but that does not imply we are actually processing the tail. That happens when all of the inputs become silent."

-> "We hold this ref during the time when we are receiving non-silent input, or during the time after first receiving silent input when we empty the processing pipeline to account for latency and tail."

> Source/WebCore/Modules/webaudio/AudioNode.cpp:411
> +        // Stop tail processing. Just need to deref ourselves, but we can't do that while the graph

"ourselves" -> "ourself"

> Source/WebCore/Modules/webaudio/AudioNode.h:156
> +    bool allowTailProcessing() const { return m_allowTailProcessing; }

As a simplification, I would simply remove this method and access m_allowTailProcessing directly in the .cpp file.
In both uses of this method, m_allowTailProcessing is already accessed directly within two or three lines.
So in these cases its simpler and more direct to avoid creating methods like this.

> Source/WebCore/Modules/webaudio/AudioNode.h:159
> +    // additional connection ref to ourselves so we don't get removed from the graph before the tail

ourselves -> ourself

> Source/WebCore/Modules/webaudio/AudioNode.h:164
> +    // end tail processing. The connection ref to ourselves will also be removed.

ourselves -> ourself

"The connection ref to ourselves will also be removed" -> "This balances the reference we took in allowTailProcessingIfNeeded()"

> LayoutTests/webaudio/resources/note-grain-on-testing.js:128
> +        // The end point is the duration

nit: period at end of sentence

[Raymond Toy]

Created attachment 139673 [details]
Patch

[Raymond Toy]

Comment on attachment 136738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136738&action=review

>> Source/WebCore/Modules/webaudio/AudioNode.cpp:216
>> +        allowTailProcessingIfNeeded();
> 
> WebKit style: avoid redundant comments - I would just remove the comments on lines 214:215 since the name of the method itself says the same thing.
> The comment in the 2nd sentence is already included in the comments in the method impl itself
> 
> Also, shouldn't this line be moved down to just above line 223 (call to process())?  This is because we don't want to call this *until* we have received non-silent input

Done.

>> Source/WebCore/Modules/webaudio/AudioNode.cpp:398
>> +        // Establish a ref to ourselves if we haven't already so we don't get removed when all nodes
> 
> "ourselves" -> "ourself"

Done.

>> Source/WebCore/Modules/webaudio/AudioNode.cpp:400
>> +        // actually processing the tail. That happens when all of the inputs become silent.
> 
> Part of this comment can be made a little more clear:
> 
> "We can process the tail, but that does not imply we are actually processing the tail. That happens when all of the inputs become silent."
> 
> -> "We hold this ref during the time when we are receiving non-silent input, or during the time after first receiving silent input when we empty the processing pipeline to account for latency and tail."

Done.

>> Source/WebCore/Modules/webaudio/AudioNode.cpp:411
>> +        // Stop tail processing. Just need to deref ourselves, but we can't do that while the graph
> 
> "ourselves" -> "ourself"

Done.

>> Source/WebCore/Modules/webaudio/AudioNode.h:156
>> +    bool allowTailProcessing() const { return m_allowTailProcessing; }
> 
> As a simplification, I would simply remove this method and access m_allowTailProcessing directly in the .cpp file.
> In both uses of this method, m_allowTailProcessing is already accessed directly within two or three lines.
> So in these cases its simpler and more direct to avoid creating methods like this.

Removed.

>> Source/WebCore/Modules/webaudio/AudioNode.h:159
>> +    // additional connection ref to ourselves so we don't get removed from the graph before the tail
> 
> ourselves -> ourself

Done.

>> Source/WebCore/Modules/webaudio/AudioNode.h:164
>> +    // end tail processing. The connection ref to ourselves will also be removed.
> 
> ourselves -> ourself
> 
> "The connection ref to ourselves will also be removed" -> "This balances the reference we took in allowTailProcessingIfNeeded()"

Done.

>> LayoutTests/webaudio/resources/note-grain-on-testing.js:128
>> +        // The end point is the duration
> 
> nit: period at end of sentence

Done.

[Raymond Toy]

Created attachment 139676 [details]
Patch

[Raymond Toy]

I've tested this patch on Safari where I can see the audiocontext being destroyed when the page is torn down.  The atexit function also shows that there are no AudioNodes left dangling.  I ran this test on several of the audio demos successfully.

There is a slight difference in approach from what we discussed (offline).  Instead of call addDeferredFinishDeref, I call addDeferredDerefNode because we need to remove the node from m_referencedNodes list.  Otherwise the list grows forever, holding on to nodes that have actually stopped.

[Chris Rogers]

Comment on attachment 139676 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139676&action=review

I notice there are many fprintfs in the code.  Please make sure to carefully examine any patches that you're uploading with "webkit-patch upload" to make sure they look good to you before asking for review.  "webkit-patch upload" normally should bring up the patch/diff in a browser so you can catch these obvious mistakes before uploading -- thanks!

> Source/WebCore/Modules/webaudio/AudioContext.cpp:197
> +    fprintf(stderr, "AudioContext::~AudioContext(): %p\n", this);

Please remove stray fprintf

> Source/WebCore/Modules/webaudio/AudioContext.cpp:581
>      // Don't allow regular lock in real-time audio thread.

This comment needs to change to match your code change on line 582
Probably best to simply remove the comment at this point

> Source/WebCore/Modules/webaudio/AudioContext.cpp:658
> +}

addDeferredDerefNode() should not be needed.  Please see comment about this method in your changes to AudioContext.h

> Source/WebCore/Modules/webaudio/AudioContext.cpp:748
> +}

handleDeferredDerefNodes() should not be needed (see my comment about this in AudioContext.h)

> Source/WebCore/Modules/webaudio/AudioContext.h:220
> +    void handleDeferredDerefNodes();

I don't understand why you've added these two new methods:
addDeferredDerefNode()
handleDeferredDerefNodes()

We already have the mechanism in place with: addDeferredFinishDeref() and handleDeferredFinishDerefs()
Why are these being added in addition?  Your old patch was even calling addDeferredFinishDeref() in AudioNode::endTailProcessingIfNeeded()

> Source/WebCore/Modules/webaudio/AudioNode.cpp:42
> +

Please remove

> Source/WebCore/Modules/webaudio/AudioNode.cpp:71
> +    fprintf(stderr, "%p: %d: AudioNode::~AudioNode() %d %d %d\n", this, nodeType(), m_normalRefCount, m_connectionRefCount, m_disabledRefCount);

Please remove stray fprintf

> Source/WebCore/Modules/webaudio/AudioNode.cpp:293
> +    fprintf(stderr, "%p: %d: AudioNode::ref(%d) %d %d %d\n", this, nodeType(), refType, m_normalRefCount, m_connectionRefCount, m_disabledRefCount);

Please remove stray fprintf

> Source/WebCore/Modules/webaudio/AudioNode.cpp:365
> +    fprintf(stderr, "%p: %d: AudioNode::deref(%d) %d %d %d\n", this, nodeType(), refType, m_normalRefCount, m_connectionRefCount, m_disabledRefCount);

Please remove stray fprintf

> Source/WebCore/Modules/webaudio/AudioNode.cpp:416
> +        context()->addDeferredDerefNode(this);

Your previous approach was correct in calling addDeferredFinishDeref().  I don't understand why you've changed this here.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:430
> +    fprintf(stderr, "===========================\n");

Please remove stray fprintf

> Source/WebCore/Modules/webaudio/AudioNode.cpp:433
> +        fprintf(stderr, "%d: %d\n", i, s_nodeCount[i]);

Please remove stray fprintf

> Source/WebCore/Modules/webaudio/AudioNode.cpp:435
> +    fprintf(stderr, "===========================\n\n\n");

Please remove stray fprintf

[Raymond Toy]

(In reply to comment #21)
> (From update of attachment 139676 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139676&action=review
> 
> I notice there are many fprintfs in the code.  Please make sure to carefully examine any patches that you're uploading with "webkit-patch upload" to make sure they look good to you before asking for review.  "webkit-patch upload" normally should bring up the patch/diff in a browser so you can catch these obvious mistakes before uploading -- thanks!

I changed the original printfs to fprintfs because they were getting interleaved with my other debugging prints (that have been removed).

Do you really want to remove all of the debugging prints that you left in the code?


> addDeferredDerefNode() should not be needed.  Please see comment about this method in your changes to AudioContext.h

As we had originally discussed, we just had addDeferredRef and handleDeferredRef, and just used addDeferredFinishDeref.

The problem with this approach is that addDeferredRef and handleDeferredRef would just keep adding nodes to m_referencedNodes.  There was no way to remove them when the tail processing is done.  This list would hang on to the nodes forever until we tried to deref unfinished nodes.  This would get an error on debug builds because we would decrement the ref too many times. (Once in handleDeferredFinishDeref and once in derefUnfinishedNodes.)

I didn't see any way for addDeferredFinishDeref to do this without adding another list anyway.  (Because we can't modify m_referencedNodes until we reach a safe point when we run handleDeferredFinishDerefs.)

Is this analysis wrong?  If so, please correct my understanding.

[Chris Rogers]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 139676 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=139676&action=review
> > 
> > I notice there are many fprintfs in the code.  Please make sure to carefully examine any patches that you're uploading with "webkit-patch upload" to make sure they look good to you before asking for review.  "webkit-patch upload" normally should bring up the patch/diff in a browser so you can catch these obvious mistakes before uploading -- thanks!
> 
> I changed the original printfs to fprintfs because they were getting interleaved with my other debugging prints (that have been removed).
> 
> Do you really want to remove all of the debugging prints that you left in the code?

No, please just leave all of this debugging code alone and don't add anything more as you are in this patch.

> 
> 
> > addDeferredDerefNode() should not be needed.  Please see comment about this method in your changes to AudioContext.h
> 
> As we had originally discussed, we just had addDeferredRef and handleDeferredRef, and just used addDeferredFinishDeref.
> 
> The problem with this approach is that addDeferredRef and handleDeferredRef would just keep adding nodes to m_referencedNodes.  There was no way to remove them when the tail processing is done.  This list would hang on to the nodes forever until we tried to deref unfinished nodes.  This would get an error on debug builds because we would decrement the ref too many times. (Once in handleDeferredFinishDeref and once in derefUnfinishedNodes.)
> 
> I didn't see any way for addDeferredFinishDeref to do this without adding another list anyway.  (Because we can't modify m_referencedNodes until we reach a safe point when we run handleDeferredFinishDerefs.)
> 
> Is this analysis wrong?  If so, please correct my understanding.

I'm not sure I follow you.  It looks like your addDeferredRefNode() method *does* actually create a new vector called "m_deferredRefList" which is then emptied in handleDeferredRefNodes() which you are calling in the post-render tasks.  So this part seems fine.  There shouldn't be any list which "keeps growing".

The part you don't need is the addDeferredDerefNode(), handleDeferredDerefNodes() methods and the associated m_deferredDerefList

[Raymond Toy]

Created attachment 140353 [details]
Patch

[Raymond Toy]

Comment on attachment 139676 [details]
Patch

All fprintf's reverted to printf's, and simplified handleDeferredDerefNodes().  As discussed (offline), this patch should be applied after 85681 which will clean things up and help us remove the parallel deferredDerefNodes list.

[Raymond Toy]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (From update of attachment 139676 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=139676&action=review
> > > 
> > > I notice there are many fprintfs in the code.  Please make sure to carefully examine any patches that you're uploading with "webkit-patch upload" to make sure they look good to you before asking for review.  "webkit-patch upload" normally should bring up the patch/diff in a browser so you can catch these obvious mistakes before uploading -- thanks!
> > 
> > I changed the original printfs to fprintfs because they were getting interleaved with my other debugging prints (that have been removed).
> > 
> > Do you really want to remove all of the debugging prints that you left in the code?
> 
> No, please just leave all of this debugging code alone and don't add anything more as you are in this patch.
> 
> > 
> > 
> > > addDeferredDerefNode() should not be needed.  Please see comment about this method in your changes to AudioContext.h
> > 
> > As we had originally discussed, we just had addDeferredRef and handleDeferredRef, and just used addDeferredFinishDeref.
> > 
> > The problem with this approach is that addDeferredRef and handleDeferredRef would just keep adding nodes to m_referencedNodes.  There was no way to remove them when the tail processing is done.  This list would hang on to the nodes forever until we tried to deref unfinished nodes.  This would get an error on debug builds because we would decrement the ref too many times. (Once in handleDeferredFinishDeref and once in derefUnfinishedNodes.)
> > 
> > I didn't see any way for addDeferredFinishDeref to do this without adding another list anyway.  (Because we can't modify m_referencedNodes until we reach a safe point when we run handleDeferredFinishDerefs.)
> > 
> > Is this analysis wrong?  If so, please correct my understanding.
> 
> I'm not sure I follow you.  It looks like your addDeferredRefNode() method *does* actually create a new vector called "m_deferredRefList" which is then emptied in handleDeferredRefNodes() which you are calling in the post-render tasks.  So this part seems fine.  There shouldn't be any list which "keeps growing".
> 
> The part you don't need is the addDeferredDerefNode(), handleDeferredDerefNodes() methods and the associated m_deferredDerefList

For the record, we discussed this offline and the approach is correct, but could be simplified.  The simplification will happen after 85681 is implemented.

[Raymond Toy]

Created attachment 142096 [details]
Proposed patch

Tentative patch assuming 85681 has been applied.

[Chris Rogers]

Comment on attachment 142096 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142096&action=review

> LayoutTests/ChangeLog:46
> +

Please re-upload patch with the ChangeLog fixed

> Source/WebCore/ChangeLog:145
> +        (WebCore::AudioPannerNode::process):

Please re-upload patch with the ChangeLog fixed

> Source/WebCore/Modules/webaudio/AudioContext.cpp:577
> +    }

Please remove changes to this method.

> Source/WebCore/Modules/webaudio/AudioContext.h:217
> +    void addDeferredDerefNode(AudioNode* node) { derefNode(node, true); }

Please call addDeferredFinishDeref() directly here, instead of going indirectly through this hacked derefNode()

> Source/WebCore/Modules/webaudio/AudioContext.h:273
> +    void derefNode(AudioNode*, bool deferred = false);

Please revert change to derefNode()

[Raymond Toy]

Comment on attachment 142096 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142096&action=review

Sorry for the bad Changelog.  I wasn't ready for this to be reviewed yet because it needs the dependent bug to be landed before this can be applied.  (I thought setting no review flags would prevent any email notifications.)

>> Source/WebCore/Modules/webaudio/AudioContext.h:217
>> +    void addDeferredDerefNode(AudioNode* node) { derefNode(node, true); }
> 
> Please call addDeferredFinishDeref() directly here, instead of going indirectly through this hacked derefNode()

No, I cannot.  derefNode removes the node from m_referencedNodes, but addDeferredFinishDeref does not.  Thus, the m_referencedNodes grows forever, holding on to the nodes, which won't get removed until context is closed.

[Raymond Toy]

Created attachment 146104 [details]
WIP

Work in progress.  chrome crashes running the granular demo, trying to read from renderingOutput(i) for some renderingOutput that has been apparently deleted.

[Raymond Toy]

Created attachment 149457 [details]
WIP