Bug 47941 – Add ConvolverNode files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add ConvolverNode files

[Chris Rogers]

Created attachment 71214 [details]
Patch

[Chris Rogers]

This is the implementation for ConvolverNode as described in the web audio specification:
http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#ConvolverNode-section

[Kenneth Russell]

Comment on attachment 71214 [details]
Patch

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

Overall the code looks okay though I have one high-level comment. r- only for the lack of an assertion in process().

> WebCore/webaudio/ConvolverNode.cpp:62
> +void ConvolverNode::process(size_t framesToProcess)

Please add an assertion about being on the audio thread in this method.

> WebCore/webaudio/ConvolverNode.cpp:133
> +    OwnPtr<Reverb> reverb = adoptPtr(new Reverb(&bufferBus, AudioNode::ProcessingSizeInFrames, MaxFFTSize, 2, true));

It is really hard just by looking at the code to prove to oneself that passing this pointer to the temporary AudioBus is safe. The Reverb and ReverbConvolver classes reference the AudioChannels in the AudioBus and then the ReverbConvolver seems to store a persistent pointer to the AudioChannel's data. Maybe I don't understand the lifetimes of these classes and I necessarily leave this to your discretion, but I would suggest rethinking how these objects are maintained, possibly by using reference counting.

[Kenneth Russell]

Comment on attachment 71214 [details]
Patch

After looking back through some of the other node implementations and noting that they do not have assertions about process() being called on the audio thread, I'm changing this to r+. My comment about the pointer to the AudioBus temporary variable still stands, though.

[Chris Rogers]

Created attachment 72715 [details]
Patch

[Chris Rogers]

This was previously R+.  I simply added a comment about the memory use for the AudioBus passed into the Reverb constructor.  It is memory only used in the constructor, and no persistent pointers are kept to it in that class.

[Kenneth Russell]

Comment on attachment 72715 [details]
Patch

OK.

[WebKit Commit Bot]

Comment on attachment 72715 [details]
Patch

Clearing flags on attachment: 72715

Committed r71195: <http://trac.webkit.org/changeset/71195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.