We should: - Make the size of the iFFT depend on the sampling rate. For now, we're generating way too much partials, and that's wasteful. - Consider caching the resulting tables. It's a common pattern create a lot of OscillatorNode, and they should ideally share their (internal) PeriodicWaves created for the basic waveforms - Consider using only linear interpolation for the curves wave tables. That should be good enough.

I need to find a good way to manager the lifetime of those, they can be pretty big so there is no way I'm just letting them in a global. I'm thinking maybe I can shove those PeriodicWaves cache on the AudioContext. I also created an new benchmark for this. Without this patch, we are around 50x realtime. With this patch, we are around 130x realtime, so that's a 260% speedup. Chome clocks in at about 150x or so. Note that this patch does not include the optimization where we tailor the size of the FFT to the number of partials we want, depending on the context's AudioContext. We are doing FFTs of size 4096 for now, we could do 2048 without changing the sound (the second half of the partials are above nyquist, but we still do a big FFT). I'm still doing the maths to make sure I'm not doing something crazy, though. Finally, this also does not include the bilinear -> linear change.

This implements the real stuff. The cache is refcounted and on the AudioContext and OscillatorNodeEngines. This is fine because they get created and destroyed on the main thread. The cache gets used on the MSG thread (both populating the cache and using the entries). I'll do the bit about computing only the right partials in another bug. Also, we are doing a linear interpolation, I mis-read what our code was doing when I wrote the other comment.

I also have written a benchmark that will go in our benchmark suite so we don't regress this.

Comment on attachment 8622534 [details] [diff] [review] Cache the basic waveform PeriodicWaves. r= >+BasicWaveFormCache* AudioContext::GetBasicWaveFormCache() Function name at beginning of a new line please. >+BasicWaveFormCache::~BasicWaveFormCache() { >+ MOZ_ASSERT(NS_IsMainThread()); >+} >+class BasicWaveFormCache >+{ >+public: >+ BasicWaveFormCache(uint32_t aSampleRate); >+ NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BasicWaveFormCache) >+ // Cache to avoid recomputing basic waveforms all the time. This is >+ // addrefed/released by the OscillatorNodeEngine on the main thread and then >+ // used from the MSG thread. >+ nsRefPtr<BasicWaveFormCache> mBasicWaveFormCache; This addrefed/release comment is probably better moved to the BasicWaveFormCache class definition as it describes the use of the class in general rather than describing access to this variable. I would look first at the class declaration when looking for such documentation. If only addrefed/released on the main thread, then BasicWaveFormCache() does not need thread-safe ref-counting. However, see next comment. >+ ~OscillatorNodeEngine() { >+ MOZ_ASSERT(NS_IsMainThread()); > } AudioNodeEngines are owned by AudioNodeStreams, the base class of which also has NS_INLINE_DECL_THREADSAFE_REFCOUNTING(). How do you know the last reference will always be released on the main thread and why is this important? This release code expects streams to be destroyed on the graph thread: https://hg.mozilla.org/mozilla-central/annotate/d7c148c84594/dom/media/MediaStreamGraph.cpp#l150 >- case TYPE: >+ case TYPE: { >+ } Unnecessary new brace.

(In reply to Karl Tomlinson (ni?:karlt back June 30) from comment #4) > >+ ~OscillatorNodeEngine() { > >+ MOZ_ASSERT(NS_IsMainThread()); > > } > > AudioNodeEngines are owned by AudioNodeStreams, the base class of > which also has NS_INLINE_DECL_THREADSAFE_REFCOUNTING(). > > How do you know the last reference will always be released on the main thread > and why is this important? It's not very important since we ref-count in a thread-safe way. I think I just got confused by the other patch about destroying the OscillatorNode and AudioBufferSourceNode MediaStreams early.

This should be better.

Comment on attachment 8625166 [details] [diff] [review] Cache the basic waveform PeriodicWaves. r= >+ NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PeriodicWave); >+ > static PeriodicWave* createSine(float sampleRate); > static PeriodicWave* createSquare(float sampleRate); > static PeriodicWave* createSawtooth(float sampleRate); > static PeriodicWave* createTriangle(float sampleRate); > > // Creates an arbitrary periodic wave given the frequency components > // (Fourier coefficients). > static PeriodicWave* create(float sampleRate, Can you make the create methods return already_AddRefed please, to make these leak-safe? That can be a separate patch or follow-up.

We need to rename the PeriodicWave object imported from Blink, othewise our leak detection system thinks the Gecko PeriodicWave object and Blink's PeriodicWave object are the same, and is confused about having a different size. This is because the macros to implement AddRef and Release only exist in an "inline" version, and only takes the name of the class, without caring about the namespace. I don't like PeriodicWaveInternal too much, though. Considering that we might end up rewriting the files we borrowed from Blink at some point, to really have a separate implementation, it might not be important. This adds already_AddRefed, and MOZ_COUNT_{C,D}TOR.

Comment on attachment 8629341 [details] [diff] [review] Properly refcount the inner PeriodicWave object. r= >We need to rename the PeriodicWave object imported from Blink, othewise our leak >detection system thinks the Gecko PeriodicWave object and Blink's PeriodicWave >object are the same, and is confused about having a different size. > >This is because the macros to implement AddRef and Release only exist in an >"inline" version, and only takes the name of the class, without caring about the >namespace. Have you tried adding the namespace explicitly: NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WebCore::PeriodicWave)? That's probably preferable to the renames if it works. >-PeriodicWave::PeriodicWave(float sampleRate) >- : m_sampleRate(sampleRate) >- , m_periodicWaveSize(PeriodicWaveSize) >- , m_numberOfRanges(NumberOfRanges) >- , m_centsPerRange(CentsPerRange) >+PeriodicWaveInternal::PeriodicWaveInternal(float sampleRate) >+ : m_sampleRate(sampleRate) >+ , m_periodicWaveSize(PeriodicWaveInternalSize) >+ , m_numberOfRanges(NumberOfRanges) >+ , m_centsPerRange(CentsPerRange) This file has 4 space indentation, so the previous form was consistent. >This adds already_AddRefed, and MOZ_COUNT_{C,D}TOR. >+ MOZ_COUNT_CTOR(PeriodicWaveInternal); MOZ_COUNT_CTOR is not usually used in ref-counted classes because it and NS_INLINE_DECL_THREADSAFE_REFCOUNTING will each count so bloat counts and any leak counts will be double. Not having MOZ_COUNT_CTOR does risk an undetected leak if the object is never AddRef()ed, but the private constructor and already_AddRefed factory methods now protect against that. I think it's probably better to stick with convention and drop MOZ_COUNT_CTOR. See comment at https://hg.mozilla.org/mozilla-central/annotate/136c41fca853/xpcom/glue/nsISupportsImpl.h#l167

(In reply to Karl Tomlinson (ni?:karlt) from comment #9) > Comment on attachment 8629341 [details] [diff] [review] > Properly refcount the inner PeriodicWave object. r= > > >We need to rename the PeriodicWave object imported from Blink, othewise our leak > >detection system thinks the Gecko PeriodicWave object and Blink's PeriodicWave > >object are the same, and is confused about having a different size. > > > >This is because the macros to implement AddRef and Release only exist in an > >"inline" version, and only takes the name of the class, without caring about the > >namespace. > > Have you tried adding the namespace explicitly: > NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WebCore::PeriodicWave)? > That's probably preferable to the renames if it works. I'm not sure how I misread the macro in the first place, but just adding the namespace works.

https://hg.mozilla.org/integration/mozilla-inbound/rev/146b55675c93 https://hg.mozilla.org/integration/mozilla-inbound/rev/911c598a9ef2

https://hg.mozilla.org/mozilla-central/rev/146b55675c93 https://hg.mozilla.org/mozilla-central/rev/911c598a9ef2 https://hg.mozilla.org/mozilla-central/rev/01d7e27c5833