997870 - OscillatorNode custom waveform optimizations

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Ralph Giles (:rillian)]

Attachment: osc-bench.html

Testing recently I noticed that Firefox is an order of magnitude slower than chrome at the attached benchmark page, both on mobile and on desktop. We should do better.

Comment 1

[Karl Tomlinson (:karlt)]

This testcase uses a constant fundamental frequency, so in this case the wave data need only be fetched once for the block, which is what blink does, instead of fetching for each sample.

Comment 2

[Paul Adenot (:padenot)]

Attachment: Optimize ComputeCustom a little. r=

Looking at Ralph's benchmark, I noticed it repeatedly creates
OfflineAudioContext that are 10 sample-frames long (the second argument of
OffineAudioContext is in frames, not seconds).

As it happens, we have a greater overhead on OfflineAudioContext
creation/completion than Chrome, and it accounted for most of the slowdown. A
more realistic benchmark for PeriodicWave would be to have an
OfflineAudioContext set to a super-long duration (I used 1000 seconds, that
renders in 2.7 seconds or so), and only have one repetition.

Doing that, we see that we are in the same magnitude (a factor of around 5 in
favor of Chrome). Applying this patch, we are only twice a slow. This patch
basically adds a return value to UpdateParametersIfNeeded so we now know if we
need to refetch the wavetable.

I'm sure we can make it better, but since we are switching basic waveforms to
WaveTables, I'd felt like making sure performance were not regressing a lot.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8534346 [details] [diff] [review]
Optimize ComputeCustom a little. r=

>-      UpdateParametersIfNeeded(ticks, i);

>+      bool needWavetableUpdate = i == aStart ? true : UpdateParametersIfNeeded(ticks, i);

I expect we still need to call UpdateParametersIfNeeded() when i == aStart.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Optimize ComputeCustom a little. r=

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8534972 [details] [diff] [review]
Optimize ComputeCustom a little. r=

>     double frequency, detune;
> 
>     bool simpleFrequency = mFrequency.HasSimpleValue();
>     bool simpleDetune = mDetune.HasSimpleValue();
> 
>     // Shortcut if frequency-related AudioParam are not automated, and we
>     // already have computed the frequency information and related parameters.
>-    if (simpleFrequency && simpleDetune && !mRecomputeParameters) {
>+    if (ConstantFrequencyForThisBlock()) {
>       return;
>     }

Now that simpleFrequency and simpleDetune are unused before the early return,
can you move the early return up, please?

>+  bool ConstantFrequencyForThisBlock()

Please change the function name to clarify that it is also considering whether
things have changed since UpdateParametersIfNeeded() was last called.

Perhaps ParametersMayNeedUpdate(), with inverse meaning of the result.

Comment 6

[Paul Adenot (:padenot)]

Attachment: Optimize ComputeCustom a little. r=

Update to last version, with comments addressed.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Optimize ComputeCustom a little. r=

Comment 8

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/187fa323cec6

Comment 9

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7978b241bf5e

Comment 10

[Paul Adenot (:padenot)]

relanded https://hg.mozilla.org/integration/mozilla-inbound/rev/b7303f56216b

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8536595 [details] [diff] [review]
Optimize ComputeCustom a little. r=

>-      UpdateParametersIfNeeded(ticks, i);

>       mPeriodicWave->waveDataForFundamentalFrequency(mFinalFrequency,
>                                                      lowerWaveData,
>                                                      higherWaveData,
>                                                      tableInterpolationFactor);

>+        mPeriodicWave->waveDataForFundamentalFrequency(mFinalFrequency,
>+                                                       lowerWaveData,
>+                                                       higherWaveData,
>+                                                       tableInterpolationFactor);
>+        UpdateParametersIfNeeded(ticks, i);

Need to do UpdateParametersIfNeeded() before waveDataForFundamentalFrequency() to set mFinalFrequency.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Something in this push made test_periodicWave.html permafail. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/72d7ae169b09

https://treeherder.mozilla.org/logviewer.html#?job_id=5080110&repo=mozilla-inbound

Comment 13

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/894be64b069b

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/894be64b069b