Bug 47941 - Add ConvolverNode files
Summary: Add ConvolverNode files
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2010-10-19 15:47 PDT by Chris Rogers
Modified: 2010-11-02 18:00 PDT (History)
8 users (show)

See Also:

Patch (10.79 KB, patch)
2010-10-19 15:49 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (10.91 KB, patch)
2010-11-02 12:43 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2010-10-19 15:47:19 PDT
Add ConvolverNode files
Comment 1 Chris Rogers 2010-10-19 15:49:37 PDT
Created attachment 71214 [details]
Comment 2 Chris Rogers 2010-10-19 16:04:49 PDT
This is the implementation for ConvolverNode as described in the web audio specification:
Comment 3 Kenneth Russell 2010-11-01 18:35:36 PDT
Comment on attachment 71214 [details]

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.
Comment 4 Kenneth Russell 2010-11-01 18:47:05 PDT
Comment on attachment 71214 [details]

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.
Comment 5 Chris Rogers 2010-11-02 12:43:47 PDT
Created attachment 72715 [details]
Comment 6 Chris Rogers 2010-11-02 12:46:13 PDT
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.
Comment 7 Kenneth Russell 2010-11-02 12:47:52 PDT
Comment on attachment 72715 [details]

Comment 8 WebKit Commit Bot 2010-11-02 18:00:31 PDT
Comment on attachment 72715 [details]

Clearing flags on attachment: 72715

Committed r71195: <http://trac.webkit.org/changeset/71195>
Comment 9 WebKit Commit Bot 2010-11-02 18:00:37 PDT
All reviewed patches have been landed.  Closing bug.