Bug 82414 – Add Oscillator/WaveTable implementation and tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Add Oscillator/WaveTable implementation and tests

[Chris Rogers]

Created attachment 134197 [details]
Patch

[Chris Rogers]

Ray could you please provide a technical review here?

You can skip the Oscillator::calculateSampleAccuratePhaseIncrements() method, since I have further cleanup there...

Thanks!

[Raymond Toy]

Comment on attachment 134197 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134197&action=review

> Source/WebCore/ChangeLog:14
> +        * DerivedSources.make:

Should the changelog actually say something more?

> Source/WebCore/Modules/webaudio/AudioNode.h:61
> +        NodeTypeOscillator,

Why here and not at the end?  This causes all of the node types to be renumbered which is annoying if you're debugging and have the node type numbers memorized.

> Source/WebCore/Modules/webaudio/Oscillator.cpp:59
> +    m_frequency = AudioParam::create("frequency", 440, 0, 100000);

Why 440 Hz?  Why is the upper limit 100kHz?

> Source/WebCore/Modules/webaudio/Oscillator.cpp:65
> +    setType(SINE);

I think this is better as setType(m_type), so if the default changes in the constructor, we don't have to fix it here.

> Source/WebCore/Modules/webaudio/Oscillator.cpp:200
> +    float rateScale = m_waveTable->rateScale();

m_waveTable->rateScale() is a double.  To preserve accuracy, rateScale should be a double.  Same for invRateScale on the line below.

> Source/WebCore/Modules/webaudio/Oscillator.cpp:231
> +            frequency = invRateScale * incr;

invRateScale is a float. This computation loses accuracy.  But see comment for line 200.  But incr only has float accuracy here (because *phaseIncrements is a float), so maybe it doesn't matter.

> Source/WebCore/Modules/webaudio/Oscillator.cpp:234
> +

Maybe add comment to say we're linearly interpolating within each table and then linearly interpolate between the tables?

> Source/WebCore/Modules/webaudio/Oscillator.cpp:236
> +        double sample2 = waveData[readIndex2];

Maybe name these sample1Upper and sample2Upper to be consistent with sampleUpper on line 239 and to match the sample1Lower/sample2Lower/sampleLower names in the lines below.

> Source/WebCore/Modules/webaudio/Oscillator.cpp:246
> +        virtualReadIndex += incr;

virtualReadIndex is a double, but incr only has float accuracy because it comes from the float *phaseIncrements.  This basically limits the accuracy of virtualReadIndex to a float.  Don'd know if that matters or not.

> Source/WebCore/Modules/webaudio/Oscillator.cpp:247
> +        virtualReadIndex -= floor(virtualReadIndex * invWaveTableSize) * waveTableSize;

There's less roundoff if you compute virtualReadIndex / waveTableSize instead of virtualReadIndex * invWaveTableSize because invWaveTableSize already has one round off error in computing it.

> Source/WebCore/Modules/webaudio/Oscillator.cpp:268
> +}

Should this set the type to CUSTOM?  Otherwise, I saw no way to set the type to CUSTOM.  Well, setType would work, but the FIXME says throw an error if the type is CUSTOM.

> Source/WebCore/Modules/webaudio/Oscillator.h:51
> +        CUSTOM = 4

Maybe prefix (or suffix) these with OSC or OSCILLATOR to make it more explicit?  I guess the corresponding change needs to be done in the idl file.

> Source/WebCore/Modules/webaudio/Oscillator.h:76
> +    unsigned short m_type;

What is m_type?  One of the enum values indicating the type of oscillator?

> Source/WebCore/Modules/webaudio/Oscillator.h:78
> +    RefPtr<AudioParam> m_frequency;

Add comment on what m_frequency (and m_detune) mean.

> Source/WebCore/Modules/webaudio/Oscillator.h:90
> +    AudioFloatArray m_tempBuffer;

Does the comment on line 88 apply to m_tempBuffer?  If it's temporary, does it need to be a member of the class?  It seems that it's not really temporary but holds the detune values.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:163
> +        for (i = 0; i < numberOfComponents; ++i) {

Use vsmul instead of loop?

> Source/WebCore/Modules/webaudio/WaveTable.cpp:200
> +            for (i = 0; i < m_waveTableSize; ++i)

Use vmaxmgv instead of loop?

> Source/WebCore/Modules/webaudio/WaveTable.cpp:201
> +                maxValue = std::max(maxValue, data[i]);

Can't data[i] be negative?  Do we want the max of the absolute value instead of the max of the positive values?

> Source/WebCore/Modules/webaudio/WaveTable.cpp:206
> +        // Apply normalization scale.

Micro optimization:  If maxValue is 0, don't do normalization because all of the data is zero.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:207
> +        for (i = 0; i < m_waveTableSize; ++i)

Use vsmul instead of loop?

> Source/WebCore/Modules/webaudio/WaveTable.cpp:230
> +        double a;

What do a and b mean here?  Looks like it's a[n]*cos(omega) + b[n]*sin(omega)?

> Source/WebCore/Modules/webaudio/WaveTable.cpp:233
> +        // Calculate Fourier coefficients depending on the shape.

It would be nice to include comments for the formulas for the fourier series for each waveform.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:241
> +            b = invOmega * (1 - ((n & 1) == 1 ? -1 : 1));

Why not make this easier to read and say b = invOmega*2 for n odd and 0 for n even?

But depending on where I look, the b[n] coefficient for a square wave is 2/pi/n for n odd and zero otherwise.  Wikipedia gives 4/pi/n.  This formula is off by scale factor. Does that matter?

> Source/WebCore/Modules/webaudio/WaveTable.cpp:244
> +            a = invOmega * 0.5 * sin(0.5 * omega) + invOmega * invOmega * cos(0.5 * omega)

Wikipedia gives the fourier series for a sawtooth as a=0 and b[n] = 2/pi*(-1)^(n+1)/n.  What exactly is the sawtooth wave form here?  Is it 0 at time 0?  Wikipedia's sawtooth is -1 at time 0.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:250
> +        case Oscillator::TRIANGLE:

What is the triangle wave here?  Mathworld gives the series as b = 8/pi^2*(-1)^((n-1)/2)/n^2 for n odd.  The triangle wave there is 0 at time 0.

> Source/WebCore/Modules/webaudio/WaveTable.h:44
> +    static PassRefPtr<WaveTable> createSine(double sampleRate);

Are these static so that you don't have to create a WaveTable object first?

> Source/WebCore/Modules/webaudio/WaveTable.h:64
> +    double sampleRate() const { return m_sampleRate; }

This is inconsistent with almost all other audio classes where the sampleRate is a float.  Should it be made consistent?  Sample rates are almost always exactly representable by a float, though. The only trouble is if you divide by it to compute something else.  (I prefer sampleRate as a double for this reason.)

> Source/WebCore/Modules/webaudio/WaveTable.idl:26
> +    // Oscillator is an audio generator of periodic waveforms.

Comment is wrong?

> LayoutTests/webaudio/resources/oscillator-testing.js:1
> +// Notes about generated waveforms:

Excellent notes!

> LayoutTests/webaudio/resources/oscillator-testing.js:5
> +// Since an audio signal must be band-limited based on the nyquist frequency (half the sample-rate)

You probably mean digital audio signal, not just "audio signal".

> LayoutTests/webaudio/resources/oscillator-testing.js:26
> +};

This seems slightly error prone if you have to keep this synchronized with the actual idl definitions.

[Chris Rogers]

Created attachment 134884 [details]
Patch

[Chris Rogers]

Created attachment 134889 [details]
Patch

[Chris Rogers]

Comment on attachment 134197 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134197&action=review

Raymond, thanks very much for your review.  I hope I've addressed all of your comments.

>> Source/WebCore/ChangeLog:14
>> +        * DerivedSources.make:
> 
> Should the changelog actually say something more?

Yes, I've added more comments.

>> Source/WebCore/Modules/webaudio/AudioNode.h:61
>> +        NodeTypeOscillator,
> 
> Why here and not at the end?  This causes all of the node types to be renumbered which is annoying if you're debugging and have the node type numbers memorized.

I'm just trying to keep the similar types of nodes (sources in this case) grouped together.  I feel this is important.

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:59
>> +    m_frequency = AudioParam::create("frequency", 440, 0, 100000);
> 
> Why 440 Hz?  Why is the upper limit 100kHz?

There has to be some default frequency and probably doesn't really matter since people will change it anyway.  I happened to choose A440 (musical pitch standard):
http://en.wikipedia.org/wiki/A440_(pitch_standard)

I've added a comment about this.

The upper limit isn't really enforced, but some "high" value (much higher than the highest pitch humans can hear)

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:65
>> +    setType(SINE);
> 
> I think this is better as setType(m_type), so if the default changes in the constructor, we don't have to fix it here.

Fixed

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:200
>> +    float rateScale = m_waveTable->rateScale();
> 
> m_waveTable->rateScale() is a double.  To preserve accuracy, rateScale should be a double.  Same for invRateScale on the line below.

Following our offline discussions, I'm leaving this float since it's going to be combined with float values from sample-accurate param values.

In general I've switched as much as possible of the code for Oscillator and WaveTable to single-precision float for performance reasons.

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:231
>> +            frequency = invRateScale * incr;
> 
> invRateScale is a float. This computation loses accuracy.  But see comment for line 200.  But incr only has float accuracy here (because *phaseIncrements is a float), so maybe it doesn't matter.

For performance, I'm trying to avoid division (much slower than multiply) and double-precision arithmetic (except m_virtualReadIndex where it's accumulating)

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:234
>> +
> 
> Maybe add comment to say we're linearly interpolating within each table and then linearly interpolate between the tables?

Fixed - added comment

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:236
>> +        double sample2 = waveData[readIndex2];
> 
> Maybe name these sample1Upper and sample2Upper to be consistent with sampleUpper on line 239 and to match the sample1Lower/sample2Lower/sampleLower names in the lines below.

Fixed - renamed to be more consistent

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:246
>> +        virtualReadIndex += incr;
> 
> virtualReadIndex is a double, but incr only has float accuracy because it comes from the float *phaseIncrements.  This basically limits the accuracy of virtualReadIndex to a float.  Don'd know if that matters or not.

Because it's accumulating values with double precision, it could make a difference in long-term drift.  I'm erring on the cautious side because I've had problems in the past (time drift over long periods of time) in very similar situations (sample-rate converters)

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:247
>> +        virtualReadIndex -= floor(virtualReadIndex * invWaveTableSize) * waveTableSize;
> 
> There's less roundoff if you compute virtualReadIndex / waveTableSize instead of virtualReadIndex * invWaveTableSize because invWaveTableSize already has one round off error in computing it.

For performance reasons,  I'm trying to avoid an explicit division (more expensive).  Most of the time the wrapping doesn't occur since the floor() value is 0.  We only have a very small potential precision loss when it occasionally wraps around from the end of the large table to the beginning.  So the precision loss only occurs once per table-wrap and not once per sample-frame which is much more frequent.  Also, we're using double-precision here for:
floor(), virtualReadIndex, and invWaveTableSize

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:268
>> +}
> 
> Should this set the type to CUSTOM?  Otherwise, I saw no way to set the type to CUSTOM.  Well, setType would work, but the FIXME says throw an error if the type is CUSTOM.

Fixed

>> Source/WebCore/Modules/webaudio/Oscillator.h:51
>> +        CUSTOM = 4
> 
> Maybe prefix (or suffix) these with OSC or OSCILLATOR to make it more explicit?  I guess the corresponding change needs to be done in the idl file.

I'd prefer to keep these constants simple in the API.

>> Source/WebCore/Modules/webaudio/Oscillator.h:76
>> +    unsigned short m_type;
> 
> What is m_type?  One of the enum values indicating the type of oscillator?

Added comment.

>> Source/WebCore/Modules/webaudio/Oscillator.h:78
>> +    RefPtr<AudioParam> m_frequency;
> 
> Add comment on what m_frequency (and m_detune) mean.

Fixed

>> Source/WebCore/Modules/webaudio/Oscillator.h:90
>> +    AudioFloatArray m_tempBuffer;
> 
> Does the comment on line 88 apply to m_tempBuffer?  If it's temporary, does it need to be a member of the class?  It seems that it's not really temporary but holds the detune values.

Changed name to m_detuneValues and fixed comment.

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:163
>> +        for (i = 0; i < numberOfComponents; ++i) {
> 
> Use vsmul instead of loop?

Fixed

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:200
>> +            for (i = 0; i < m_waveTableSize; ++i)
> 
> Use vmaxmgv instead of loop?

Fixed

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:201
>> +                maxValue = std::max(maxValue, data[i]);
> 
> Can't data[i] be negative?  Do we want the max of the absolute value instead of the max of the positive values?

Yes, good point.  Using vmaxmgv() solves this.

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:206
>> +        // Apply normalization scale.
> 
> Micro optimization:  If maxValue is 0, don't do normalization because all of the data is zero.

Because this case is an anomaly (condition should never happen - no useful wavetable will have this property) and also because it makes the code slightly harder to follow, I'm avoiding this.

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:207
>> +        for (i = 0; i < m_waveTableSize; ++i)
> 
> Use vsmul instead of loop?

Fixed

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:230
>> +        double a;
> 
> What do a and b mean here?  Looks like it's a[n]*cos(omega) + b[n]*sin(omega)?

Added comment.

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:233
>> +        // Calculate Fourier coefficients depending on the shape.
> 
> It would be nice to include comments for the formulas for the fourier series for each waveform.

Comments added.

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:241
>> +            b = invOmega * (1 - ((n & 1) == 1 ? -1 : 1));
> 
> Why not make this easier to read and say b = invOmega*2 for n odd and 0 for n even?
> 
> But depending on where I look, the b[n] coefficient for a square wave is 2/pi/n for n odd and zero otherwise.  Wikipedia gives 4/pi/n.  This formula is off by scale factor. Does that matter?

The scaling factor is accounted for by the normalization in createBandLimitedTables()
Added comment about this.

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:244
>> +            a = invOmega * 0.5 * sin(0.5 * omega) + invOmega * invOmega * cos(0.5 * omega)
> 
> Wikipedia gives the fourier series for a sawtooth as a=0 and b[n] = 2/pi*(-1)^(n+1)/n.  What exactly is the sawtooth wave form here?  Is it 0 at time 0?  Wikipedia's sawtooth is -1 at time 0.

Yes, it's zero at time 0.  I've added a comment describing all the waveform types.

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:250
>> +        case Oscillator::TRIANGLE:
> 
> What is the triangle wave here?  Mathworld gives the series as b = 8/pi^2*(-1)^((n-1)/2)/n^2 for n odd.  The triangle wave there is 0 at time 0.

Added comment describing the shape.

>> Source/WebCore/Modules/webaudio/WaveTable.h:44
>> +    static PassRefPtr<WaveTable> createSine(double sampleRate);
> 
> Are these static so that you don't have to create a WaveTable object first?

This is just the ordinary WebKit way of creating objects (static methods called create(), etc.).  The constructors are made private using this style.

>> Source/WebCore/Modules/webaudio/WaveTable.h:64
>> +    double sampleRate() const { return m_sampleRate; }
> 
> This is inconsistent with almost all other audio classes where the sampleRate is a float.  Should it be made consistent?  Sample rates are almost always exactly representable by a float, though. The only trouble is if you divide by it to compute something else.  (I prefer sampleRate as a double for this reason.)

Fixed

>> Source/WebCore/Modules/webaudio/WaveTable.idl:26
>> +    // Oscillator is an audio generator of periodic waveforms.
> 
> Comment is wrong?

Fixed

>> LayoutTests/webaudio/resources/oscillator-testing.js:5
>> +// Since an audio signal must be band-limited based on the nyquist frequency (half the sample-rate)
> 
> You probably mean digital audio signal, not just "audio signal".

Fixed

>> LayoutTests/webaudio/resources/oscillator-testing.js:26
>> +};
> 
> This seems slightly error prone if you have to keep this synchronized with the actual idl definitions.

Yes, but if it ever gets out of sync it will be quite apparent in layout test failures so I'm pretty confident it won't slip through the cracks unnoticed.
The alternative is to create a temporary AudioContext and Oscillator in each of the testing files, which is quite ugly and harder to read.

[Philippe Normand]

Comment on attachment 134889 [details]
Patch

Attachment 134889 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12288454

[Chris Rogers]

Created attachment 134909 [details]
Patch

[Chris Rogers]

Latest patch attempts to fix GTK build.  Also removes expected results since they're large .wav files and exceed the bugzilla limit.  "webkit-patch upload" fails silently so I can land those separately.

[Raymond Toy]

Comment on attachment 134889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134889&action=review

> Source/WebCore/Modules/webaudio/Oscillator.cpp:110
> +    bool hasSampleAccurateChanges = false;

Would hasTimeLineValues be more accurate?  When I read and look at the code, I keep thinking that if hasSampleAccurateChanges = false then the result is not sample accurate.

> Source/WebCore/Modules/webaudio/Oscillator.cpp:205
> +    bool hasSampleAccurateChanges = calculateSampleAccuratePhaseIncrements(framesToProcess);

See comment on line 110.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:41
> +const unsigned WaveTableSize = 4096;

Add comment that WaveTableSize must be a power of two.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:252
> +            b = invOmega * (1 - ((n & 1) == 1 ? -1 : 1));

This is easier to read and understand when written as (n & 1) == 1 ? 2 : 0.  Or maybe (n&1) == 1 ? 2*invOmega : 0.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:257
> +                -(invOmega * -0.5 * sin(-0.5 * omega) + invOmega * invOmega * cos(-0.5 * omega));

sin(0.5*omega) = = sin(pi*n) = 0, ignoring roundoff.  That leaves invOmega^2*cos(0.5*omega) - invOmega^2*cos(-0.5*omega).  How is this not zero?

> Source/WebCore/Modules/webaudio/WaveTable.idl:26
> +    // WaveTable represents a periodic audio waveform given Fourier coefficients.

given -> "given by its"

[Raymond Toy]

Comment on attachment 134909 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134909&action=review

> Source/WebCore/Modules/webaudio/WaveTable.cpp:255
> +            // Sawtooth-shaped waveform with the first half ramping from zero to maximum and the second half from minimum to zero.

The sawtooth as defined here is an odd function.  Hence a = 0.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:263
> +            // Triangle-shaped waveform going from its maximum value to its minimum value then back to the maximum value.

The triangle defined here is an even function.  Hence b must be 0.

[Chris Rogers]

Hi James, I'm addressing Ray's last comments.  He's handling the mathy audio tech review, but hoped you could look at the most recent patch for anything that stands out to you.  Thanks!

[Kenneth Russell]

Comment on attachment 134909 [details]
Patch

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.

[Chris Rogers]

Comment on attachment 134889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134889&action=review

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:110
>> +    bool hasSampleAccurateChanges = false;
> 
> Would hasTimeLineValues be more accurate?  When I read and look at the code, I keep thinking that if hasSampleAccurateChanges = false then the result is not sample accurate.

Fixed

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:205
>> +    bool hasSampleAccurateChanges = calculateSampleAccuratePhaseIncrements(framesToProcess);
> 
> See comment on line 110.

Fixed

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:41
>> +const unsigned WaveTableSize = 4096;
> 
> Add comment that WaveTableSize must be a power of two.

Comment added.

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:252
>> +            b = invOmega * (1 - ((n & 1) == 1 ? -1 : 1));
> 
> This is easier to read and understand when written as (n & 1) == 1 ? 2 : 0.  Or maybe (n&1) == 1 ? 2*invOmega : 0.

Good point - simplified.

>> Source/WebCore/Modules/webaudio/WaveTable.cpp:257
>> +                -(invOmega * -0.5 * sin(-0.5 * omega) + invOmega * invOmega * cos(-0.5 * omega));
> 
> sin(0.5*omega) = = sin(pi*n) = 0, ignoring roundoff.  That leaves invOmega^2*cos(0.5*omega) - invOmega^2*cos(-0.5*omega).  How is this not zero?

Good catch - thanks for finding these redundant/zero terms!

I've cleaned up the redundant sin() terms here and in the triangle below.

>> Source/WebCore/Modules/webaudio/WaveTable.idl:26
>> +    // WaveTable represents a periodic audio waveform given Fourier coefficients.
> 
> given -> "given by its"

Fixed

[Chris Rogers]

Created attachment 134920 [details]
Patch

[Kenneth Russell]

Comment on attachment 134920 [details]
Patch

r=me with the same caveats as before. I really think the maintainability issues with this code should be fixed now.

[Raymond Toy]

Comment on attachment 134920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134920&action=review

So sorry I didn't see this earlier.  I just have a couple of nits that simplify the formulas quite a bit.  Otherwise, it looks very good.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:257
> +            b = -invOmega * 0.5 * cos(0.5 * omega) - invOmega * 0.5 * cos(-0.5 * omega);

nit:  cos(x) is even, so this could be simplified to -invOmega*0.5*cos(0.5*omega).  But cos(0.5*omega) = cos(pi*n) = (-1)^n so b = (-1)^n/omega.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:260
> +            // Triangle-shaped waveform going from its maximum value to its minimum value then back to the maximum value.

nit: The triangle wave here is an even function so b must be 0.  And it is because cos(omega) = cos(2*pi*n) = 1.

> Source/WebCore/Modules/webaudio/WaveTable.cpp:261
> +            a = (2 - 4 * cos(0.5 * omega) + 2 * cos(omega)) / (n * n * piFloat * piFloat);

nit: cos(omega) = 1.  cos(0.5*omega) = cos(pi*n) = (-1)^n.  So a = (4-4*(-1)^n)/(n^2*pi^2).  So a = 8/(n^2*pi^2) for n odd and a = 0 for n even.

[Chris Rogers]

(In reply to comment #13)
> (From update of attachment 134909 [details])
> 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

I agree, and as we discussed offline will put in a patch for AutoTryLocker separately.


> 
> > 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?

Fixed.

> 
> > 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?

This is something used a few other places in the code - will put in separate patch for inline function into AudioUtilities.

> 
> > 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.

Fixed.

> 
> > 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.

Currently that's the nature of the FFTFrame class.

[Chris Rogers]

(In reply to comment #17)
> (From update of attachment 134920 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134920&action=review
> 
> So sorry I didn't see this earlier.  I just have a couple of nits that simplify the formulas quite a bit.  Otherwise, it looks very good.
> 
> > Source/WebCore/Modules/webaudio/WaveTable.cpp:257
> > +            b = -invOmega * 0.5 * cos(0.5 * omega) - invOmega * 0.5 * cos(-0.5 * omega);
> 
> nit:  cos(x) is even, so this could be simplified to -invOmega*0.5*cos(0.5*omega).  But cos(0.5*omega) = cos(pi*n) = (-1)^n so b = (-1)^n/omega.
> 
> > Source/WebCore/Modules/webaudio/WaveTable.cpp:260
> > +            // Triangle-shaped waveform going from its maximum value to its minimum value then back to the maximum value.
> 
> nit: The triangle wave here is an even function so b must be 0.  And it is because cos(omega) = cos(2*pi*n) = 1.
> 
> > Source/WebCore/Modules/webaudio/WaveTable.cpp:261
> > +            a = (2 - 4 * cos(0.5 * omega) + 2 * cos(omega)) / (n * n * piFloat * piFloat);
> 
> nit: cos(omega) = 1.  cos(0.5*omega) = cos(pi*n) = (-1)^n.  So a = (4-4*(-1)^n)/(n^2*pi^2).  So a = 8/(n^2*pi^2) for n odd and a = 0 for n even.

Thanks, I've updated to simplify this math very close to your suggestions.

[Chris Rogers]

Committed r112938: <http://trac.webkit.org/changeset/112938>

[Simon Fraser (smfr)]

This broke a test:
http://build.webkit.org/results/Lion%20Release%20(Tests)/r112950%20(7151)/fast/dom/Window/window-properties-pretty-diff.html

[Chris Rogers]

(In reply to comment #21)
> This broke a test:
> http://build.webkit.org/results/Lion%20Release%20(Tests)/r112950%20(7151)/fast/dom/Window/window-properties-pretty-diff.html

Simon, sorry about that - I just landed the fix...