Bug 93194 – Creating "basic waveform" Oscillator nodes is not efficient

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Srikumar K. Subramanian]

New Oscillator nodes need to be created for each "voice" since their noteOn/noteOff methods can be used only once. Therefore efficient creation of these nodes is desirable. Currently, new wavetables are created and setup for the "basic waveform" types every time a new Oscillator node is created and its "type" field is set (to SINE, SQUARE, etc.), which is an unnecessary overhead to Oscillator node creation.

[Chris Rogers]

It looks like we could easily optimize to share the WaveTable objects between Oscillator instances through the use of lazily-created WaveTables (static member vars of Oscillator) used in the Oscillator::setType() methods for the simple waveform types...

This would avoid redundant creation of WaveTable objects and their associated overhead.

[Raymond Toy]

Created attachment 156986 [details]
Patch

[Raymond Toy]

(In reply to comment #2)
> Created an attachment (id=156986) [details]
> Patch

Added the static member vars to cache the wave tables.

This patch also fixes a bug where the oscillator type was always returning CUSTOM instead of the specified type.  Test added for this case.

[Chris Rogers]

Comment on attachment 156986 [details]
Patch

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

Thanks Ray:

> Source/WebCore/Modules/webaudio/Oscillator.cpp:48
> +RefPtr<WaveTable> Oscillator::s_waveTableTriangle;

It's strictly forbidden in WebKit to have static initialization code run, so these can't be RefPtr<>, but instead should be raw pointers.
You'll then need to call release() on the RefPtr's below to "convert" them into a raw pointer.
These are shared WaveTables which will be lazily initialized and live for the life of the process.

Ask someone to help if this doesn't make sense.

[Raymond Toy]

Created attachment 157039 [details]
Patch

[Raymond Toy]

Comment on attachment 156986 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:48
>> +RefPtr<WaveTable> Oscillator::s_waveTableTriangle;
> 
> It's strictly forbidden in WebKit to have static initialization code run, so these can't be RefPtr<>, but instead should be raw pointers.
> You'll then need to call release() on the RefPtr's below to "convert" them into a raw pointer.
> These are shared WaveTables which will be lazily initialized and live for the life of the process.
> 
> Ask someone to help if this doesn't make sense.

Fixed as suggested.

[Chris Rogers]

Comment on attachment 157039 [details]
Patch

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

> Source/WebCore/Modules/webaudio/Oscillator.cpp:92
> +            s_waveTableSine = WaveTable::createSine(sampleRate).leakRef();

I am not familiar with this leakRef() method and don't see it used very much in the WebKit code, so you'll either have to ask somebody who knows more about this (abarth, for example).
Or, change to a pattern like this:

RefPtr<WaveTable> table = WaveTable::createSine(sampleRate);
s_waveTableSine = table.release();

[Raymond Toy]

Comment on attachment 157039 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/Oscillator.cpp:92
>> +            s_waveTableSine = WaveTable::createSine(sampleRate).leakRef();
> 
> I am not familiar with this leakRef() method and don't see it used very much in the WebKit code, so you'll either have to ask somebody who knows more about this (abarth, for example).
> Or, change to a pattern like this:
> 
> RefPtr<WaveTable> table = WaveTable::createSine(sampleRate);
> s_waveTableSine = table.release();

I started this way, but s_waveTableSine is a WaveTable* and table.release() is a PassRefPtr<WaveTable> so you can't do that assignment.

http://www.webkit.org/coding/RefPtr.html says to convert a RefPtr to a raw pointer, use table.release().leakRef().

I'll ask abarth.

[Adam Barth]

Comment on attachment 157039 [details]
Patch

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

>>> Source/WebCore/Modules/webaudio/Oscillator.cpp:92
>>> +            s_waveTableSine = WaveTable::createSine(sampleRate).leakRef();
>> 
>> I am not familiar with this leakRef() method and don't see it used very much in the WebKit code, so you'll either have to ask somebody who knows more about this (abarth, for example).
>> Or, change to a pattern like this:
>> 
>> RefPtr<WaveTable> table = WaveTable::createSine(sampleRate);
>> s_waveTableSine = table.release();
> 
> I started this way, but s_waveTableSine is a WaveTable* and table.release() is a PassRefPtr<WaveTable> so you can't do that assignment.
> 
> http://www.webkit.org/coding/RefPtr.html says to convert a RefPtr to a raw pointer, use table.release().leakRef().
> 
> I'll ask abarth.

Yeah, this sort of situation is one of the rare cases where we use leakRef.  You're intentionally leaking the object into a static variable (in the sense that we'll never deallocate the object).

If you only plan to use the static object in a single lexical scope, you can avoid calling leakRef using this pattern:

DEFINE_STATIC_LOCAL(RefPtr<WaveTable>, waveTableSine, (WaveTable::createSine(sampleRate)));

but the approach you've taken here is fine too.

[Chris Rogers]

Comment on attachment 157039 [details]
Patch

Looks good - thanks Ray!

[WebKit Review Bot]

Comment on attachment 157039 [details]
Patch

Rejecting attachment 157039 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
(content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [chromium] Add missing OVERRIDE and virtual annotations in compositor

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13459406

[Adam Barth]

Comment on attachment 157039 [details]
Patch

Sorry, there are a ton of patches landing right now.  Let's try again.

[WebKit Review Bot]

Comment on attachment 157039 [details]
Patch

Clearing flags on attachment: 157039

Committed r125122: <http://trac.webkit.org/changeset/125122>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.