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.
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.
Created attachment 156986 [details] Patch
(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 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.
Created attachment 157039 [details] Patch
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 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 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 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 on attachment 157039 [details] Patch Looks good - thanks Ray!
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 on attachment 157039 [details] Patch Sorry, there are a ton of patches landing right now. Let's try again.
Comment on attachment 157039 [details] Patch Clearing flags on attachment: 157039 Committed r125122: <http://trac.webkit.org/changeset/125122>
All reviewed patches have been landed. Closing bug.