[webkit-reviews] review denied: [Bug 48012] Add AudioBufferSourceNode files : [Attachment 71344] 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 48012: Add AudioBufferSourceNode files
https://bugs.webkit.org/show_bug.cgi?id=48012

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=71344&action=review

Overall the code basically looks good to a layman not experienced in audio
processing. r- for a few minor issues, and a couple of larger / higher-level
comments.

> WebCore/webaudio/AudioBufferSourceNode.cpp:83
> +    // Careful - this is a tryLock() and not an autolocker, so we must
unlock() before every return.

Please consider adding a TryLocker class and MutexTryLocker typedef to WTF. The
TryLocker class could have a bool holdsLock() method, and would only unlock the
mutex in its destructor if it successfully acquired it in its constructor. This
would clean up this class and I'm pretty sure a few others. This can be a
separate bug following this one.

> WebCore/webaudio/AudioBufferSourceNode.cpp:145
> +    bool channelCountGood = numberOfChannels == busNumberOfChannels &&
(numberOfChannels == 1 || numberOfChannels == 2);

How about a FIXME here as below? This is the first case of this test in this
file reading from the top down.

> WebCore/webaudio/AudioBufferSourceNode.cpp:264
> +    // FIXME: we can add support for sources with more than two channels,
but this is not a common case.

Is it expected that it may be more of a common case in the future? Is there any
real problem with generalizing this code to arrays of channels, source
pointers, destination pointers, etc.? It seems it might cut in half the amount
of code in certain places.

> WebCore/webaudio/AudioBufferSourceNode.cpp:280
> +    sourceR += m_readIndex;

Please add assertions that you're not walking off the end of the source, and
won't when accessing framesToProcess frames past these pointers.

> WebCore/webaudio/AudioBufferSourceNode.cpp:405
> +void AudioBufferSourceNode::noteOff(double /*when*/)

Just remove the commented-out variable name.

> WebCore/webaudio/AudioBufferSourceNode.cpp:411
> +    // FIXME: when is ignored.

Put quotes around "when". Confusing to read otherwise.

> WebCore/webaudio/AudioBufferSourceNode.h:48
> +    static PassRefPtr<AudioBufferSourceNode> create(AudioContext* context,
double sampleRate)

Is there a particular reason to inline this creation method in the header? If
not please move its body to the .cpp.