Bug 93194

Summary: Creating "basic waveform" Oscillator nodes is not efficient
Product: WebKit Reporter: Srikumar K. Subramanian <srikumarks>
Component: Web AudioAssignee: Raymond Toy <rtoy>
Status: RESOLVED FIXED    
Severity: Minor CC: abarth, crogers, eric.carlson, feature-media-reviews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 93490    
Attachments:
Description Flags
Patch
none
Patch none

Description Srikumar K. Subramanian 2012-08-04 20:52:27 PDT
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.
Comment 1 Chris Rogers 2012-08-04 22:00:16 PDT
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.
Comment 2 Raymond Toy 2012-08-07 13:14:08 PDT
Created attachment 156986 [details]
Patch
Comment 3 Raymond Toy 2012-08-07 13:15:27 PDT
(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.
Comment 4 Chris Rogers 2012-08-07 13:29:49 PDT
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.
Comment 5 Raymond Toy 2012-08-07 16:34:13 PDT
Created attachment 157039 [details]
Patch
Comment 6 Raymond Toy 2012-08-07 16:34:57 PDT
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.
Comment 7 Chris Rogers 2012-08-08 10:31:27 PDT
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();
Comment 8 Raymond Toy 2012-08-08 10:51:29 PDT
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.
Comment 9 Adam Barth 2012-08-08 11:00:02 PDT
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.
Comment 10 Chris Rogers 2012-08-08 16:47:04 PDT
Comment on attachment 157039 [details]
Patch

Looks good - thanks Ray!
Comment 11 WebKit Review Bot 2012-08-08 17:02:19 PDT
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
Comment 12 Adam Barth 2012-08-08 17:04:38 PDT
Comment on attachment 157039 [details]
Patch

Sorry, there are a ton of patches landing right now.  Let's try again.
Comment 13 WebKit Review Bot 2012-08-08 17:18:43 PDT
Comment on attachment 157039 [details]
Patch

Clearing flags on attachment: 157039

Committed r125122: <http://trac.webkit.org/changeset/125122>
Comment 14 WebKit Review Bot 2012-08-08 17:18:47 PDT
All reviewed patches have been landed.  Closing bug.