[webkit-reviews] review granted: [Bug 82414] Add Oscillator/WaveTable implementation and tests : [Attachment 134909] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Chris Rogers
<crogers at google.com>'s request for review:
Bug 82414: Add Oscillator/WaveTable implementation and tests
https://bugs.webkit.org/show_bug.cgi?id=82414

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

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


I trust Ray's review of the math but the overall structure looks OK. I strongly
feel you should add an AutoTryLocker class as the number of places in the Web
Audio implementation where you're manually doing tryLock()...unlock() is
increasing, but I won't block the review for it. r=me

> Source/WebCore/Modules/webaudio/Oscillator.cpp:108
> +bool Oscillator::calculateSampleAccuratePhaseIncrements(size_t
framesToProcess)

I gather that the caller ensures that framesToProcess is less than or equal to
the size of the m_detune and m_phaseIncrements arrays, but is it worth adding
some more ASSERTS here, closer to the vsmul calls?

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

You should really add an AutoTryLocker class which you can query to see whether
the tryLock succeeded. I'd practically insist on this at this point.

> Source/WebCore/Modules/webaudio/Oscillator.cpp:225
> +    while (n--) {

WebKit style is to prefer preincrement and predecrement, if it's convenient to
restructure the loop to use it. If not it's fine; this is concise.

> Source/WebCore/Modules/webaudio/Oscillator.cpp:247
> +	   float sampleHigher = (1 - interpolationFactor) * sample1Higher +
interpolationFactor * sample2Higher;

How about a lerp() function?

> Source/WebCore/Modules/webaudio/WaveTable.cpp:148
> +void WaveTable::createBandLimitedTables(const float* realData, const float*
imagData, unsigned numberOfComponents)

Too bad it isn't possible to have assertions about the size of the storage
pointed to by realData and imagData, and numberOfComponents. However, I
verified that the callers are correct.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:190
> +	   // Clear packed-nyquist if necessary

Missing period.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:203
> +	   frame.doInverseFFT(data);

Unfortunate that it isn't possible to assert inside the FFTFrame class that
data points to enough storage.