[webkit-reviews] review denied: [Bug 47941] Add ConvolverNode files : [Attachment 71214] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Chris Rogers <crogers at google.com>'s
request for review:
Bug 47941: Add ConvolverNode files
https://bugs.webkit.org/show_bug.cgi?id=47941

Attachment 71214: Patch
https://bugs.webkit.org/attachment.cgi?id=71214&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
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.