| Summary: | Add ConvolverNode files | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Rogers <crogers> | ||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cmarrin, commit-queue, dglazkov, eric.carlson, jamesr, jer.noble, kbr, simon.fraser | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Other | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
Chris Rogers
2010-10-19 15:47:19 PDT
Created attachment 71214 [details]
Patch
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 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. 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.
Created attachment 72715 [details]
Patch
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 on attachment 72715 [details]
Patch
OK.
Comment on attachment 72715 [details] Patch Clearing flags on attachment: 72715 Committed r71195: <http://trac.webkit.org/changeset/71195> All reviewed patches have been landed. Closing bug. |