Bug 77515 – Modify RealtimeAnalyserNode pull mechanism

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond]

Hi 

Per description on https://bugs.webkit.org/show_bug.cgi?id=76431 and webaudio spec : 4.17. The RealtimeAnalyserNode Interface, it will be good that some inspector Node like RealtimeAnalyserNode can pull without connect to downstream node which ultimately connect to an audio destination.

As Chris have mentioned at https://bugs.webkit.org/show_bug.cgi?id=76431#c5 , this task must be done carefully to handle audionode's life time well.

Here I propose an approaching as below :

--------------------------

= Goal: =

1. Eliminate the need of RealtimeAnalyserNode to be connected to (direcly or indirectly) the DestinationNode in order to make itself get pumped and retrieve the data. Actually this apply to other potential nodes with similar requirement ( which don't necessary need to have a NodeOutput  and wish to inspect the audio stream)

2. Try to minimize the impact on current APP already using RealtimeAnalyserNode.

= Overall Design =

Provide a base node class who's numberOfOutputs might be 0 or 1.
Subclass RealtimeAnalyserNode from it.
When this kind of node's numberOfOutputs is 0, add it to extraPullNodes, otherwise, remove it from extraPullNodes. And these node with go through normal process path.
In Render process, have the context process these nodes in just before handlePostRenderTasks.
The node will also be removed from the pull list when it's uninitialized, since a node only get uninitialized at the end of each render quantum,  so it won't be a issue.

= current status =

I will attach a patch which do work without issue.

= Todo  =

This first patch is just try to show the conception, Makefile related files other then chromium build and Layout test case not yet added.


----------------------------

Any comments on this approaching is welcome.

[Raymond]

Created attachment 124890 [details]
patch for design review

This Patch just for review the conception. If the idea is ok, I will update it with Layout Test case together.

[Raymond]

(In reply to comment #0)

Oh, sorry that the overall design on comment #0 is somehow out of date compare with the patch. Please ignore it and here's the more accurate version :

-----------

Overall Design

Provide a base node class who's numberOfOutputs is 1, while it does not necessary require an output connection to make a pull.  Subclass RealtimeAnalyserNode from it.

When this kind of node's numberOfOutputConnections is 0, add it to extraPullNodes list. In audio thread, have the context process these nodes in just before handlePostRenderTasks.  

If  numberOfOutputConnections is not 0,  remove it from extraPullNodes. And these node with go through normal process path.

The extraPullNodes will be maintained as current rending list, and target list which will be sync with the rendering one upon handle pre/post task in audio thread. Just like AudioNodeInput handle it's rendering connections.

The node will also be removed from the list when it's marked for delete.

[Raymond]

Created attachment 125083 [details]
sample layout test for design review

And attach a sample layout test here also for design review.
It should be point out that, this layout test will crash on debug mode due to another bug I reported at:

https://bugs.webkit.org/show_bug.cgi?id=77609

[Raymond]

Created attachment 125229 [details]
Patch

[Raymond]

Hi

I have updated patches to include the code and layout test in one single patch. Please help to take a review ;) And btw, it won't crash the layout test anymore

[Raymond]

Created attachment 125560 [details]
Patch

[Raymond]

Hi Chris

I update the patch again to include the changes for Build related files on various platform. Now this should be a complete patch for this issue. Please help to review on this :) Thanks.

Btw, where is those HASH like string(FD315FF612B0267600C1A359 etc.) in project.pbxproj come from ? Is there any doc on this?

[Raymond]

Hi Chris

I noticed that in the latest minutes of w3c audio group. It is agreed that RealTimeAnalyserNode should be able to work without an output, So, Come back to this patch, what do you think about the overall idea? If the overall idea looks good, I can rebase this patch to the latest trunk.

[Chris Rogers]

(In reply to comment #8)
> Hi Chris
> 
> I noticed that in the latest minutes of w3c audio group. It is agreed that RealTimeAnalyserNode should be able to work without an output, So, Come back to this patch, what do you think about the overall idea? If the overall idea looks good, I can rebase this patch to the latest trunk.

Hi Raymond, yes I think the overall idea is good.  I've already had a quick look.  Thanks!

[Raymond]

Created attachment 137287 [details]
Patch

[Build Bot]

Comment on attachment 137287 [details]
Patch

Attachment 137287 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12409835

[Raymond]

(In reply to comment #9)
> (In reply to comment #8)
> > Hi Chris
> > 
> > I noticed that in the latest minutes of w3c audio group. It is agreed that RealTimeAnalyserNode should be able to work without an output, So, Come back to this patch, what do you think about the overall idea? If the overall idea looks good, I can rebase this patch to the latest trunk.
> 
> Hi Raymond, yes I think the overall idea is good.  I've already had a quick look.  Thanks!

Hi Chris,

Patch updated. However, for mac build. I have no idea how those HEX ID in .pbxproj is generated? Since I duplicate them from the nearest neighbor AudioBasicProcessor, it lead to a build failure. Could you help to address this issue? Which ID I should put in there for the new file? I don't have a Mac machine which can run say XCode?

[Raymond]

(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Hi Chris
> > > 
> > > I noticed that in the latest minutes of w3c audio group. It is agreed that RealTimeAnalyserNode should be able to work without an output, So, Come back to this patch, what do you think about the overall idea? If the overall idea looks good, I can rebase this patch to the latest trunk.
> > 
> > Hi Raymond, yes I think the overall idea is good.  I've already had a quick look.  Thanks!
> 
> Hi Chris,
> 
> Patch updated. However, for mac build. I have no idea how those HEX ID in .pbxproj is generated? Since I duplicate them from the nearest neighbor AudioBasicProcessor, it lead to a build failure. Could you help to address this issue? Which ID I should put in there for the new file? I don't have a Mac machine which can run say XCode?

Hi Chris.

Or I can just ignore Mac build. and adding an expected fail on mac's layout test. So that you or someone with right tools/IDE can fix the .pbxproj file later?

[Chris Rogers]

Comment on attachment 137287 [details]
Patch

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

Thanks Raymond.  I'm sorry it's taken so long to get to this review (it's a big one).  It looks reasonable, but I have a number of comments.  Once the patch is ready, then I can help fix the Xcode project...

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:2
> + * Copyright (C) 2012, Google Inc. All rights reserved.

I think you may wish to change "Google" -> "Intel"

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:39
> +    , m_needExtraPull(false)

I think "automatic" might be a better name than "extra"

m_needExtraPull -> m_needsAutomaticPull

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:59
> +    AudioContext::AutoLocker locker(context());

I would move the locker to just after the ASSERT on line 56.  I think the code is a little cleaner that way, and since the AutoLocker uses a recursive lock, it's a little more
efficient since it doesn't have to acquire and release the lock twice in a row.

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:69
> +    AudioContext::AutoLocker locker(context());

I would move the locker to just after the ASSERT on line 66.  I think the code is a little cleaner that way, and since the AutoLocker uses a recursive lock, it's a little more
efficient since it doesn't have to acquire and release the lock twice in a row.

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:100
> +        // downstream node, thus remove it from the context's extra pull list.

extra -> automatic

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:107
> +        // some upstream nodes, add it to the context's extra pull list.

"still be connected by some upstream" -> "still connected *from* upstream node(s)"

extra -> automatic

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:108
> +        unsigned numberOfInputConnections = input(0)->numberOfRenderingConnections();

numberOfRenderingConnections() -> numberOfConnections()

since numberOfRenderingConnections() is only meant to be called from inside the audio thread while processing and is a less up-to-date version of numberOfConnections()

The one part I still don't understand is how you're handling the case when before some input connections existed for this node, but those connections no longer exist (since the upstream nodes disconnected from us)
It seems like nobody is calling this method (updatePullStatus()) when that case happens.

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.h:2
> + * Copyright (C) 2012, Google Inc. All rights reserved.

I think you may wish to change "Google" -> "Intel"

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.h:32
> +// AudioBasicInspectorNode is an AudioNode with one input and one output where the output might not necesary connect to another node's input.

"might not necesary" -> "might not necessarily"

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.h:33
> +// If the output is not connect to any input, then the AudioBasicInspectorNode's processIfNecessary function will be called by AudioContext before

"output is not connect to any input" -> "not connected to any other node"

"will be called by AudioContext" -> "will be called automatically by AudioContext"

> Source/WebCore/Modules/webaudio/AudioContext.cpp:717
> +    // the renderingExtraPullNodes.

Ideally we would be able to remove it from the pull list earlier than this time (when there are no more up-stream nodes connected to the inspector node).
Otherwise, it could be quite some time before garbage collection happens and this node continues to get pulled.

> Source/WebCore/Modules/webaudio/AudioContext.h:151
> +

Extra -> Automatic

> Source/WebCore/Modules/webaudio/AudioContext.h:290
> +    HashSet<AudioNode*> m_extraPullNodes;

I would add a comment describing that for thread safety, we maintain a separate Vector for rendering which is copied from here when it changes in updateExtraPullNodes().

> Source/WebCore/Modules/webaudio/AudioDestinationNode.cpp:84
> +    context()->processExtraPullNodes(numberOfFrames);

"Process extra nodes which need to pull data by themselves." -> "Process nodes which need a little extra help because they are not connected to anything, but still need to process."
"Extra" -> "Automatic"

> Source/WebCore/Modules/webaudio/AudioNodeOutput.h:75
> +    bool isConnected() { return fanOutCount() > 0; }

Needs to be updated to be:
{ return fanOutCount() > 0 || paramFanOutCount() > 0; }

> LayoutTests/webaudio/extra-pull-node.html:15
> +description("This test verifies that the AudioBasicInspectorNode based nodes working correctly.");

"working correctly" -> "work correctly"

> LayoutTests/webaudio/extra-pull-node.html:18
> +// We carefully arrange the renderLengthInFrames to be multi times of the audio processing frame,

"be multi times" -> "be a multiple"

"audio processing frame" -> "AudioNode rendering quantum (AudioNode::ProcessingSizeInFrames)"

> LayoutTests/webaudio/extra-pull-node.html:28
> +function createDataBuffer(context, length) {

audio-testing.js now has a cool new function called "createConstantBuffer()" you can use instead of this one...

> LayoutTests/webaudio/extra-pull-node.html:56
> +    var arr = new Uint8Array(fftSize);

can we rename all of these variables called "arr" -> "timeDomainData"

> LayoutTests/webaudio/extra-pull-node.html:60
> +        testPassed("RealtimeAnalyser got pulled when connected to upstream node but not downstream node.");

"RealtimeAnalyser" -> "RealtimeAnalyserNode" here and elsewhere

"to upstream node" -> "from upstream node"

> LayoutTests/webaudio/extra-pull-node.html:62
> +        testFailed("RealtimeAnalyser failed to get pulled when connected to upstream node but not downstream node.");

ditto

> LayoutTests/webaudio/extra-pull-node.html:67
> +// To verify the realtimeAnalyser can pull data when there is upstream node connect to it

RealtimeAnalyserNode

"there is upstream node connect" -> "there is an upstream node connected"

> LayoutTests/webaudio/extra-pull-node.html:68
> +// at the same time it don't connect to a downstream node.

"at the same time it don't connect to a downstream node" -> "but it's not connected to a downstream node"

> LayoutTests/webaudio/extra-pull-node.html:83
> +        testPassed("RealtimeAnalyser got pulled when connected by upstream node and to destination node.");

"by upstream" -> "from upstream"

> LayoutTests/webaudio/extra-pull-node.html:91
> +// at the same time it also connect to a downstream node which ultimately connect to audio destination

Similar changes to wording as for test1

> LayoutTests/webaudio/extra-pull-node.html:107
> +    // If realtimeAnalyser hadn't pulled any data, the data here will be 128.

RealtimeAnalyserNode hasn't

> LayoutTests/webaudio/extra-pull-node.html:130
> +

nit: extra blank line

[Raymond]

Created attachment 138496 [details]
Patch

[Raymond]

(In reply to comment #14)
Hi Chris
Thanks for the review in great detail. Patch updated.

> 
> > Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:108
> > +        unsigned numberOfInputConnections = input(0)->numberOfRenderingConnections();
> 
> numberOfRenderingConnections() -> numberOfConnections()
> 
> since numberOfRenderingConnections() is only meant to be called from inside the audio thread while processing and is a less up-to-date version of numberOfConnections()

Because numberOfRenderingConnections is public one and numberOfConnections is private one ;) And when this branch is called numberOfRenderingConnections is already synced with numberOfConnections. So It doesn't matter.

> 
> The one part I still don't understand is how you're handling the case when before some input connections existed for this node, but those connections no longer exist (since the upstream nodes disconnected from us)
> It seems like nobody is calling this method (updatePullStatus()) when that case happens.
> 

I don't quite catch you here, do you mean the case of this node is disconnected by other nodes? then it will be called from audionodeinput::updateRenderingState -> audiobasicinspectornode::checkNumberOfChannelsForInput -> updatePullStatus

[Build Bot]

Comment on attachment 138496 [details]
Patch

Attachment 138496 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12523149

[Chris Rogers]

Comment on attachment 138496 [details]
Patch

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

Thanks Raymond, this is starting to look pretty good.

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:99
> +        // When a AudioBasicInspectorNode is connected to a downstream node, it will get pulled by the

"When a" -> "When an"

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:106
> +        // When a AudioBasicInspectorNode is not connected to any downstream node while still connected from

"When a" -> "When an"

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:112
> +        }

I'm still not sure how we handle the case when numberOfInputConnections==0 (where all our input connections have gone away).  Don't we need something like:

if (!numberOfInputConnections && m_needAutomaticPull) {
    context()->removeAutomaticPullNode(this);
    m_needAutomaticPull = false;
}

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.h:33
> +// If the output is not connect to any other node, then the AudioBasicInspectorNode's processIfNecessary function will be called automatically by

"not connect" -> "not connected"

"processIfNecessary function" -> "processIfNecessary() method"

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.h:34
> +// AudioContext before the end of each render quantum to pull data from its input(s) so that it can inspect the audio stream.

I would just remove the text "to pull data from its input(s)"

> Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.h:46
> +    bool m_needAutomaticPull;

I would move this bool variable after the method updatePullStatus() and add a short comment for this instance variable describing what it is.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:726
> +    removeAutomaticPullNode(node);

I believe it should be impossible for this node to still be in the m_automaticPullNodes list because AudioBasicInspectorNode::updatePullStatus() should have already removed it (since all input connections to it should be gone by now).
So we don't need this code - does this make sense? 

Also, please check the ASSERTS that we have in AudioContext::~AudioContext() -- I think we need to add an extra ASSERT for m_automaticPullNodes and m_renderingAutomaticPullNodes

> Source/WebCore/Modules/webaudio/AudioContext.h:151
> +

Please add short comment describing these two methods (how they're used with inspector nodes, etc.)

> Source/WebCore/Modules/webaudio/AudioContext.h:155
> +    // Called right before handlePostRenderTasks();

No need for ";" at end of line :)

Also, I'd make this comment a little longer saying"

// Called right before handlePostRenderTasks() to handle inspector nodes which aren't connected to anything.

> Source/WebCore/Modules/webaudio/AudioContext.h:291
> +    // It will be copied from m_automaticPullNodes by updateAutomaticPullNodes at the very start or end of the rendering quantum.

"updateAutomaticPullNodes" -> "updateAutomaticPullNodes()"

> LayoutTests/webaudio/automatic-pull-node.html:31
> +    toneBuffer = createConstantBuffer(context, renderLengthInFrames, 1);

nit: this isn't really a "tone" since it's a constant buffer, so I would rename this "constantBuffer", otherwise people might get confused

> LayoutTests/webaudio/automatic-pull-node.html:46
> +    if (timeDomainData[0] >= 255)

255 is a "magic" constant.  Can we define a constant variable for this after line 21, with a comment describing why its value is 255.

> LayoutTests/webaudio/automatic-pull-node.html:95
> +    if (timeDomainData[0] == 128)

Magic constant value needs to be defined around line 21 along with comment describing why its value is 128

> LayoutTests/webaudio/automatic-pull-node.html:103
> +// To verify the realtimeAnalyser will stop pull if it is connected to a downstream node

"realtimeAnalyser will stop pull" ->"RealtimeAnalyserNode will stop pulling"

> LayoutTests/webaudio/automatic-pull-node.html:104
> +// which do not ultimatly connect to any audio destination

"do not ultimatly connect" -> "is not ultimately connected"

nit: Period "." after end of sentence

[Raymond]

Created attachment 138945 [details]
Patch

[Raymond]

(In reply to comment #18)
Hi Chris, patch updated.

> 
> > Source/WebCore/Modules/webaudio/AudioBasicInspectorNode.cpp:112
> > +        }
> 
> I'm still not sure how we handle the case when numberOfInputConnections==0 (where all our input connections have gone away).  Don't we need something like:
> 
> if (!numberOfInputConnections && m_needAutomaticPull) {
>     context()->removeAutomaticPullNode(this);
>     m_needAutomaticPull = false;
> }

You are right. I have thought that when this is no input nor output connection, the node will be removed and markForDeletion will handle this case. But now I realized that there are case that it's disabled and might be connected again. So this case should be handled here.

> 
> > Source/WebCore/Modules/webaudio/AudioContext.cpp:726
> > +    removeAutomaticPullNode(node);
> 
> I believe it should be impossible for this node to still be in the m_automaticPullNodes list because AudioBasicInspectorNode::updatePullStatus() should have already removed it (since all input connections to it should be gone by now).
> So we don't need this code - does this make sense? 

Hmm, I think we do need this code, for updatePullStatus might be triggered later, since in handlePostRenderTasks(), derefFinishedSoruceNodes come before handleDirtyAudioNodeInputs(). And then in audioNodeInput::updateRenderingState() if the node is mark for deletion, it will do nothing and return, thus updatePullStatus() won't be called at all.

[Build Bot]

Comment on attachment 138945 [details]
Patch

Attachment 138945 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12520777

[Raymond]

Created attachment 139316 [details]
Patch

[Raymond]

Hi Chris

Thanks for the help on fix .pbxproj file. Patch updated.

[Build Bot]

Comment on attachment 139316 [details]
Patch

Attachment 139316 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12550431

[Raymond]

Created attachment 139321 [details]
Patch

[Raymond]

finally, pass mac build.

[Chris Rogers]

Comment on attachment 139321 [details]
Patch

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

Raymond, looks great.  I have a couple of tiny nits.  Also, you can see that the "cr-linux" and "efl" EWS bots are purple.  This because the patch is out-of-date.  You need to update your source, and try to fix the merge problems, then re-upload the patch.  I also tried to apply your latest patch to a fresh checkout and could see the patch did not apply cleanly.  Once this is done, I can approve the patch.

> Source/WebCore/ChangeLog:10
> +        This commit also includes a patch from Chris Rogers for WebCore.xcodeproj/project.pbxproj.

Raymond, no need to credit me for this small part of the patch in the ChangeLog...

> Source/WebCore/Modules/webaudio/AudioContext.cpp:725
> +    // since all connections are gone and we hold the graph lock. Then when handlePostRenderTasks

handlePostRenderTasks -> handlePostRenderTasks()

> Source/WebCore/Modules/webaudio/AudioContext.cpp:726
> +    // get chance to schedule the deletion work, the updateAutomaticPullNodes() also get chance to modify

"get chance" -> "gets a chance"   in two places in this sentence

> Source/WebCore/Modules/webaudio/AudioContext.cpp:727
> +    // the renderingAutomaticPullNodes.

"the renderingAutomaticPullNodes " -> "m_renderingAutomaticPullNodes"

[Raymond]

Created attachment 139721 [details]
Patch

[Raymond]

Hi Chris
Thanks for the review. Patch is updated ;)

[Chris Rogers]

Comment on attachment 139721 [details]
Patch

Thanks Raymond!

[WebKit Review Bot]

Comment on attachment 139721 [details]
Patch

Clearing flags on attachment: 139721

Committed r115787: <http://trac.webkit.org/changeset/115787>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.