I'll prepare a patch for the DOM bindings part, ETA some time today.

Comment on attachment 754525 [details] [diff] [review] Part 1: DOM bindings https://hg.mozilla.org/integration/mozilla-inbound/rev/f95813c97dd6

Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.

See <https://dvcs.w3.org/hg/audio/rev/7c4a40a9bb57>. We need to change our implementation according to that.

Forgot to add the Bindings.conf entry...

Comment on attachment 764712 [details] [diff] [review] Part 2: Rename WaveTable to PeriodicWave https://hg.mozilla.org/integration/mozilla-inbound/rev/af744b5304d8

Work in progress, doesn't build. I gave up on doing this from scratch and am borrowing blink's implementation. Still needs PeriodicWave::createBandLimitedTables rewritten to use our FFTBlock or kiss_fft directly. How much do we want to use FFTBlock?

WIP; copy the coefficient data into the OscillatorNode and send it to the Engine.

Comment on attachment 801040 [details] [diff] [review] Part 3: Import blink's PeriodicWave implementation Review of attachment 801040 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/blink/PeriodicWave.cpp @@ +152,5 @@ > + > + // Generate complex conjugate because of the way the > + // inverse FFT is defined. > + float minusOne = -1; > + vsmul(imagP, 1, &minusOne, imagP, 1, halfSize); You should probably use AudioBufferInPlaceScale here. @@ +166,5 @@ > + imagP[i] = 0; > + } > + // Clear packed-nyquist if necessary. > + if (numberOfPartials < halfSize) > + imagP[0] = 0; Note that our FFTBlock implementation does not use packed nyquist. @@ +183,5 @@ > + // For the first range (which has the highest power), calculate > + // its peak value then compute normalization scale. > + if (!rangeIndex) { > + float maxValue; > + vmaxmgv(data, 1, &maxValue, m_periodicWaveSize); We don't yet have an implementation of this function, you can add your own to AudioNodeEngine.h. Should be fairly easy to implement. @@ +190,5 @@ > + normalizationScale = 1.0f / maxValue; > + } > + > + // Apply normalization scale. > + vsmul(data, 1, &normalizationScale, data, 1, m_periodicWaveSize); AudioBufferInPlaceScale here as well. ::: content/media/webaudio/blink/PeriodicWave.idl @@ +24,5 @@ > + > +// PeriodicWave represents a periodic audio waveform given by its Fourier coefficients. > +[ > + Conditional=WEB_AUDIO > +] interface PeriodicWave { You shouldn't need to add this file.

(In reply to Ralph Giles (:rillian) from comment #11) > Created attachment 801040 [details] [diff] [review] > Part 3: Import blink's PeriodicWave implementation > > Work in progress, doesn't build. I gave up on doing this from scratch and am > borrowing blink's implementation. > > Still needs PeriodicWave::createBandLimitedTables rewritten to use our > FFTBlock or kiss_fft directly. How much do we want to use FFTBlock? What's the build issue here? I'd really like us to use FFTBlock here if possible, in case later on we start to optimize it for some platforms, that would help all callers pick up the optimizations. Do my comments above help?

Comment on attachment 801043 [details] [diff] [review] Part 4: Implement PeriodicWave Review of attachment 801043 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/OscillatorNode.cpp @@ +259,5 @@ > + for (uint32_t i = start; i < end; ++i) { > + output[i] = out[i].r; > + } > + delete out; > + free(cfg); Please use FFTBlock here if possible. ::: content/media/webaudio/PeriodicWave.cpp @@ +33,5 @@ > + /* Copy frequency-domain data. */ > + mRealData = new float[aLength]; > + memcpy(mRealData, aRealData, aLength*sizeof(float)); > + mImagData = new float[aLength]; > + memcpy(mImagData, aImagData, aLength*sizeof(float)); Nit: Please use PodCopy. @@ +48,5 @@ > + memcpy(real, mRealData, mLength*sizeof(float)); > + data->SetData(0, real, real); > + float *imag = new float[mLength]; > + memcpy(imag, mImagData, mLength*sizeof(float)); > + data->SetData(1, imag, imag); Here, you should malloc (and not new[]) a buffer big enough for both of these arrays, and call SetData for each channel, passing in the correct offset into the allocated buffer. For the first buffer, pass the buffer address as the "data to free" argument. For the rest, pass nullptr. See here as an example of code which does this: <http://dxr.mozilla.org/mozilla-central/source/content/media/webaudio/ConvolverNode.cpp#l221> Also, to avoid double copying here, I suggest that you make this class hold a ThreadSharedFloatArrayBufferList member, and create the buffer list in the constructor, and change GetThreadSharedBuffer() to just return a pointer to that member. The idea of ThreadSharedFloatArrayBufferList is that the buffer is immutable and shareable, so you basically refcount the ThreadSharedFloatArrayBufferList objects, and can read the buffer pointers from inside it freely from any thread, since you know that if you have a reference to the ThreadSharedFloatArrayBufferList object, the underlying data is live and ready to be used. ::: content/media/webaudio/PeriodicWave.h @@ +7,4 @@ > #ifndef PeriodicWave_h_ > #define PeriodicWave_h_ > > +#include "AudioContext.h" You should not need this in the header. Please move it to the cpp file? @@ +52,5 @@ > private: > nsRefPtr<AudioContext> mContext; > + > + nsAutoPtr<float> mRealData; > + nsAutoPtr<float> mImagData; You want nsAutoArrayPtr. It's pretty terrible that we can't turn this into a build failure. :( @@ +53,5 @@ > nsRefPtr<AudioContext> mContext; > + > + nsAutoPtr<float> mRealData; > + nsAutoPtr<float> mImagData; > + int32_t mLength; Nit: uint32_t.

Thanks for the feedback. Very helpful! (In reply to :Ehsan Akhgari (needinfo? me!) from comment #15) > Comment on attachment 801043 [details] [diff] [review] > Part 4: Implement PeriodicWave > > ::: content/media/webaudio/OscillatorNode.cpp > @@ +259,5 @@ > [...] > Please use FFTBlock here if possible. We need to call blink's OscillatorNode::process here, or more likely reproduce the linear interpolation between the already-transformed waveforms provided by their PeriodicWave. I'll try to make that use FFTBlock though. I've removed this code now. > @@ +48,5 @@ > > + memcpy(real, mRealData, mLength*sizeof(float)); > > + data->SetData(0, real, real); > > + float *imag = new float[mLength]; > > + memcpy(imag, mImagData, mLength*sizeof(float)); > > + data->SetData(1, imag, imag); > > Here, you should malloc (and not new[]) a buffer big enough for both of > these arrays, and call SetData for each channel, passing in the correct > offset into the allocated buffer. For the first buffer, pass the buffer > address as the "data to free" argument. For the rest, pass nullptr. See > here as an example of code which does this: > <http://dxr.mozilla.org/mozilla-central/source/content/media/webaudio/ > ConvolverNode.cpp#l221> > > Also, to avoid double copying here, I suggest that you make this class hold > a ThreadSharedFloatArrayBufferList member, and create the buffer list in the > constructor, and change GetThreadSharedBuffer() to just return a pointer to > that member. The idea of ThreadSharedFloatArrayBufferList is that the > buffer is immutable and shareable, so you basically refcount the > ThreadSharedFloatArrayBufferList objects, and can read the buffer pointers > from inside it freely from any thread, since you know that if you have a > reference to the ThreadSharedFloatArrayBufferList object, the underlying > data is live and ready to be used. So the buffer is immutable, and the refcount update is threadsafe? ok, that's a better idea. > ::: content/media/webaudio/PeriodicWave.h > > > +#include "AudioContext.h" > > You should not need this in the header. Please move it to the cpp file? nsRefPtr wants it for mContext. See also AudioBuffer.h. > @@ +52,5 @@ > > private: > > nsRefPtr<AudioContext> mContext; > > + > > + nsAutoPtr<float> mRealData; > > + nsAutoPtr<float> mImagData; > > You want nsAutoArrayPtr. It's pretty terrible that we can't turn this into > a build failure. :( Is the difference just semantic here? I thought the difference was that nsAutoArrayPtr would 'delete []' the storage, calling per-element destructors which is a no-op for float. > @@ +53,5 @@ > > nsRefPtr<AudioContext> mContext; > > + > > + nsAutoPtr<float> mRealData; > > + nsAutoPtr<float> mImagData; > > + int32_t mLength; > > Nit: uint32_t. I used int32 so I could use SendInt32ParameterToStream. The valid range is 1..4096 (enforced with an assert) casting is safe. I can implement SendUint32ParamterToStream and use that instead if you'd rather?

Add an equivalent to blink's VectorMath::vmaxmgv.

Add and external to external ifft method it FFTFrame so we can convert the coefficient data.

Updated copy of the blink code. Implemented ehsan's feedback and uses gecko routines. This one compiles. Karl, I had trouble plumbing through the smart pointer stuff for WebCore::PeriodicNode and WebCore::AudioFloatBuffer. For now I just switched to raw pointers, which leaks without a destructor (or worse). It looks like you got something to work with the HRTF code. Would you mind taking a look? Specifically what should WebCore::PeriodicWave::create() return, and what type should WebCore::PeriodicWave::m_bandLimitedTables be?

Updated in response to to ehsan's feedback. Doesn't call the new blink code yet.

(In reply to Ralph Giles (:rillian) from comment #16) > > You want nsAutoArrayPtr. It's pretty terrible that we can't turn this into > > a build failure. :( > > Is the difference just semantic here? I thought the difference was that > nsAutoArrayPtr would 'delete []' the storage, calling per-element > destructors which is a no-op for float. Karl explained on irc, "they might behave the same with many implentations of delete[], but i assume it is undefined behavior to delete something from new[]"

(In reply to Ralph Giles (:rillian) from comment #19) > Karl, I had trouble plumbing through the smart pointer stuff for > WebCore::PeriodicNode and WebCore::AudioFloatBuffer. For now I just switched > to raw pointers, which leaks without a destructor (or worse). It looks like > you got something to work with the HRTF code. Would you mind taking a look? > Specifically what should WebCore::PeriodicWave::create() return, and what > type should WebCore::PeriodicWave::m_bandLimitedTables be? Although blink's WebCore::PeriodicWave is ref-counted, I assume you don't have more than one owner and so you won't need reference counting. nsAutoPtr<> doesn't have an equivalent of PassOwnPtr<>. already_AddRefed<> pretends to be, but doesn't guarantee that ownership transfers. nsReturnRef<> is similar to PassOwnPtr, as used in HRTFDatabase.h, but requires more boiler plate. Returning a raw pointer from WebCore::PeriodicWave::create(), like you have, is probably simplest here, and assign it to an nsAutoPtr<WebCore::PeriodicWave> at the call site. The closest equivalent of Vector<OwnPtr<AudioFloatArray> > would be nsTArray<nsAutoPtr<AudioFloatArray> > m_bandLimitedTables.

Comment on attachment 802503 [details] [diff] [review] Part 3cd: import and port blink's PeriodicWave implementation Review of attachment 802503 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/blink/PeriodicWave.cpp @@ +168,5 @@ > + imagP[i] = 0; > + } > + // Clear packed-nyquist if necessary. > + if (numberOfPartials < halfSize) > + imagP[0] = 0; We don't use packed nyquists. I think this is incorrect. @@ +209,5 @@ > + float* imagP = imag.Elements(); > + > + // Clear DC and Nyquist. > + realP[0] = 0; > + imagP[0] = 0; Ditto. @@ +247,5 @@ > + a = (4 - 4 * cos(0.5 * omega)) / (n * n * piFloat * piFloat); > + b = 0; > + break; > + default: > + NS_ASSERTION(false, "Shouldn't reach this point."); You probably want NS_NOTREACHED here. ::: content/media/webaudio/blink/PeriodicWave.h @@ +89,5 @@ > + unsigned numberOfPartialsForRange(unsigned rangeIndex) const; > + > + // Creates tables based on numberOfComponents Fourier coefficients. > + void createBandLimitedTables(const float* real, const float* imag, unsigned numberOfComponents); > + nsTArray<AudioFloatArray*> m_bandLimitedTables; You want nsTArray<nsAutoPtr<AudioFloatArray>> here.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #23) > Comment on attachment 802503 [details] [diff] [review] > Part 3cd: import and port blink's PeriodicWave implementation > > > + // Clear packed-nyquist if necessary. > > + if (numberOfPartials < halfSize) > > + imagP[0] = 0; > > We don't use packed nyquists. I think this is incorrect. Is packed nyquist the thing where the f/2 real component is packed into the imaginary component of the DC coefficient? That should explain why halfsize is size/2 instead of size/2 + 1. But that just means we should clear it unconditionally. The DC coefficient of a real signal must be pure real (and likewise the nyquist frequency, which must be phase-aligned with the samples). kiss_fft seems to ignore these values if they happen to be set, but it's correct to zero them. > > + NS_ASSERTION(false, "Shouldn't reach this point."); > > You probably want NS_NOTREACHED here. nifty. > ::: content/media/webaudio/blink/PeriodicWave.h > > > + nsTArray<AudioFloatArray*> m_bandLimitedTables; > > You want nsTArray<nsAutoPtr<AudioFloatArray>> here. Done, thanks.

Include the original blink code in a separate commit to make it easier to see the changes we made.

Updated patch in response to feedback from karl and ehsan. Thanks guys!

Rebased changes to our Osciallator node code. Still not hookup up to the blink implementation, but fixes several issues passing the custom data and applies feedback suggestions. Changes our implementation to throw INVALID_STATE_ERROR on osc.type = 'custom'.

This version of the patches is also available as https://github.com/rillian/firefox/commits/periodic6-rollup if that's an easier reference.

Updated patch. Make WebCore::PeriodicNode::create() take raw float arrays instead of trying to unpack the shared buffer list. Also fix a number of bugs with array access.

WIP. Call createBandLimitedTables, fix associated bugs.

According to my test pages at https://people.mozilla.org/~rgiles/2013/osc06.html and https://people.mozilla.org/~rgiles/2013/osc07.html there are some noise issues in the higher frequencies, but it otherwise looks and sounds like chrome's implementation.

Comment on attachment 803417 [details] [diff] [review] Part 3c: Import blink's PeriodicWave Review of attachment 803417 [details] [diff] [review]: ----------------------------------------------------------------- Please include the Blink revision from which you took these files in the commit message when landing. Thanks!

Comment on attachment 805038 [details] [diff] [review] Part 3d: Port blink's PeriodicWave to gecko. v8 Review of attachment 805038 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below. Please fix the commit message as well. ::: content/media/webaudio/blink/PeriodicWave.h @@ +95,4 @@ > > // Creates tables based on numberOfComponents Fourier coefficients. > void createBandLimitedTables(const float* real, const float* imag, unsigned numberOfComponents); > + nsTArray<AudioFloatArray*> m_bandLimitedTables; Hmm, you should use nsAutoPtr here like I suggested earlier.

Comment on attachment 805039 [details] [diff] [review] Part 4: Implement PeriodicWave v8 Review of attachment 805039 [details] [diff] [review]: ----------------------------------------------------------------- Over to Paul to check ComputeCustom... ::: content/media/webaudio/PeriodicWave.h @@ +12,4 @@ > #include "mozilla/Attributes.h" > #include "EnableWebAudioCheck.h" > #include "AudioContext.h" > +#include "AudioNodeEngine.h" This shouldn't be needed... ::: content/media/webaudio/blink/PeriodicWave.h @@ +95,4 @@ > > // Creates tables based on numberOfComponents Fourier coefficients. > void createBandLimitedTables(const float* real, const float* imag, unsigned numberOfComponents); > + nsTArray<nsAutoPtr<AudioFloatArray> > m_bandLimitedTables; This belongs to the previous patch.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #35) > ::: content/media/webaudio/PeriodicWave.h > @@ +12,4 @@ > > #include "mozilla/Attributes.h" > > #include "EnableWebAudioCheck.h" > > #include "AudioContext.h" > > +#include "AudioNodeEngine.h" > > This shouldn't be needed... AudioNodeEngine.h in needed for ThreadSharedFloatArrayBuffer. nsAutoPtr seems to want the complete class so it can call Release() from the destructor, so I can't just declare an opaque class name. > ::: content/media/webaudio/blink/PeriodicWave.h > @@ +95,4 @@ > > > + nsTArray<nsAutoPtr<AudioFloatArray> > m_bandLimitedTables; > > This belongs to the previous patch. Oops. Yes. > Please include the Blink revision from which you took these files in the commit message when landing. Thanks! The 'chromium svn r156949' is probably the blink revision, I can't check until I get back to my work machine. I've updated it to 'blink svn trunk r157670' which is the last changed rev on my current checkout and identical to the files in the patch.

I meant nsRefPtr, not nsAutoPtr.

Update commit message with blink revision. Carrying forward r=ehsan

Update in response to review comments. Moved nsAutoPtr from the Part 4 patch, fixed commit message. Carrying forward r=ehsan.

Updated patch to move nsAutoPtr fix to blink code to Part 3d and improved commit message. Carrying forward r=ehsan and r? request to padenot for ComputeCustom. Paul, note this patch will have merge conflicts with yours from bug 908669.

*sigh*. This is the actual updated patch. Previous ones were from a stale export. Carrying forward r=ehsan

Correct updated patch file. Carrying forward r=ehsan.

Actual updated patch file. Sorry of the confusion. Carrying forward r=ehsan and r? for padenot to review ComputeCustom. Paul, note there will be merge conflicts with your BLIT patch.

Android doesn't have log2f. This should be equivalent, in blink/PeriodicWave.cpp: - float centsAboveLowestFrequency = log2f(ratio) * 1200; + float centsAboveLowestFrequency = logf(ratio)/logf(2f) * 1200; https://tbpl.mozilla.org/?tree=Try&rev=0173711290ac

Comment on attachment 805104 [details] [diff] [review] bug865256-4-v9.patch Review of attachment 805104 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/OscillatorNode.cpp @@ +266,5 @@ > + FillBounds(output, ticks, start, end); > + > + uint32_t periodicWaveSize = mPeriodicWave->periodicWaveSize(); > + float *higherWaveData = nullptr; > + float *lowerWaveData = nullptr; nit: * next to the type. ::: content/media/webaudio/OscillatorNode.h @@ +80,3 @@ > if (aType == OscillatorType::Custom) { > + // ::Custom can only be set by setPeriodicWave(). > + // https://www.w3.org/Bugs/Public/show_bug.cgi?id=17368 for exception. This bugtracker is kind of obsolete, now. Better to use [1]. [1]: https://github.com/WebAudio/web-audio-api/issues/105 ::: content/media/webaudio/PeriodicWave.cpp @@ +32,5 @@ > + mLength = static_cast<int32_t>(aLength); > + > + // Copy coefficient data. The two arrays share an allocation. > + mCoefficients = new ThreadSharedFloatArrayBufferList(2); > + float *buffer = static_cast<float*>(malloc(aLength*sizeof(float)*2)); nit: * next to the type. @@ +33,5 @@ > + > + // Copy coefficient data. The two arrays share an allocation. > + mCoefficients = new ThreadSharedFloatArrayBufferList(2); > + float *buffer = static_cast<float*>(malloc(aLength*sizeof(float)*2)); > + MOZ_ASSERT(buffer, "allocation failure"); Aren't we using infallible allocator? ::: content/media/webaudio/PeriodicWave.h @@ +40,5 @@ > JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; > > + ThreadSharedFloatArrayBufferList* GetThreadSharedBuffer(); > + > + int32_t DataLength() const This is not used, right? @@ +49,4 @@ > private: > nsRefPtr<AudioContext> mContext; > + nsRefPtr<ThreadSharedFloatArrayBufferList> mCoefficients; > + int32_t mLength; Unsigned type for lengths. ::: content/media/webaudio/test/test_oscillatorNode.html @@ +46,5 @@ > + // Verify setPeriodicWave() > + var real = new Float32Array([1.0, 0.5, 0.25, 0.125]); > + var imag = new Float32Array([1.0, 0.7, -1.0, 0.5]); > + osc.setPeriodicWave(context.createPeriodicWave(real, imag)); > + is(osc.type, "custom", "Failed to set custom waveform"); Maybe we could have some more involved test, when we have time? Or even porting some tests from Blink/Webkit.

Updated patch to work around Android/MSVC not having log2f() per comment 44. Carrying forward r=ehsan.

Thanks for the review! Carrying forward r=ehsan,padenot. https://tbpl.mozilla.org/?tree=Try&rev=ed601767bf5d Updated patch to address review comments: - Move pointer next to type name in declarations. - Use uint32_t for mLength. >> + // Copy coefficient data. The two arrays share an allocation. >> + mCoefficients = new ThreadSharedFloatArrayBufferList(2); >> + float *buffer = static_cast<float*>(malloc(aLength*sizeof(float)*2)); >> + MOZ_ASSERT(buffer, "allocation failure"); > > Aren't we using infallible allocator? ThreadSharedFloatArrayList calls free(), so I believe I need malloc() here. new and moz_xmalloc() are infallible but MDN says malloc is still fallible.[1] [1] https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation >> + ThreadSharedFloatArrayBufferList* GetThreadSharedBuffer(); >> + >> + int32_t DataLength() const > > This is not used, right? It's called by OscillatorNode::SendPeriodicWaveToStream() so it can pass the array length to the engine. I've moved the implementation of GetThreadSharedBuffer() to the header since it's similarly trivial. > Maybe we could have some more involved test, when we have time? Or even > porting some tests from Blink/Webkit. Yes. I've opened bug 916897 for this.

Comment on attachment 802497 [details] [diff] [review] Part 3a: AudioBufferPeakValue utility [Approval Request Comment] Bug caused by (feature/regressing bug #): 865256 User impact if declined: This and subsequent patches implement the the PeriodicWave object for creating custom oscillators in the Web Audio api. Web Audio is a high-demand feature for game developers. This feature is high priority for Firefox 25 and this is a blocking bug for doing so. Testing completed (on m-c, etc.): I would like to land on nightly, aurora, and beta in quick succession, ASAP. So far tested locally and on try; landing on nightly and aurora currently blocked by the closed tree. Risk to taking this patch (and alternatives if risky): Changes are limited to the Web Audio API feature, not previously enabled in beta, so I don't expect it to regress currently working content. The alternative is to pref off this feature for ff 25, or release with a partial implementation. String or IDL/UUID changes made by this patch: None

Comment on attachment 802500 [details] [diff] [review] Part 3b: New ifft method on FFTBlock [Approval Request Comment] Same as Part 3a above. Bug caused by (feature/regressing bug #): 865256 User impact if declined: This and subsequent patches implement the the PeriodicWave object for creating custom oscillators in the Web Audio API. Web Audio is a high-demand feature for game developers. This feature is high priority for Firefox 25 and this is a blocking bug for doing so. Testing completed (on m-c, etc.): I would like to land on nightly, aurora, and beta in quick succession, ASAP. So far tested locally and on try; landing on nightly and aurora currently blocked by the closed tree. Risk to taking this patch (and alternatives if risky): Changes are limited to the Web Audio API feature, not previously enabled in beta, so I don't expect it to regress currently working content. The alternative is to pref off this feature for ff 25, or release with a partial implementation. String or IDL/UUID changes made by this patch: None

Comment on attachment 805100 [details] [diff] [review] Part 3c: Import blink's PeriodicWave v9 [Approval Request Comment] Same as Part 3a and 3b above. Bug caused by (feature/regressing bug #): 865256 User impact if declined: This and subsequent patches implement the the PeriodicWave object for creating custom oscillators in the Web Audio API. Web Audio is a high-demand feature for game developers. This feature is high priority for Firefox 25 and this is a blocking bug for doing so. Testing completed (on m-c, etc.): I would like to land on nightly, aurora, and beta in quick succession, ASAP. So far tested locally and on try; landing on nightly and aurora currently blocked by the closed tree. Risk to taking this patch (and alternatives if risky): Changes are limited to the Web Audio API feature, not previously enabled in beta, so I don't expect it to regress currently working content. The alternative is to pref off this feature for ff 25, or release with a partial implementation. String or IDL/UUID changes made by this patch: None

Comment on attachment 805423 [details] [diff] [review] Part 3d: Port blink's PeriodicWave to gecko v10 [Approval Request Comment] Same as Parts 3a, 3b, 3d, and 4. Bug caused by (feature/regressing bug #): 865256 User impact if declined: This and subsequent patches implement the the PeriodicWave object for creating custom oscillators in the Web Audio API. Web Audio is a high-demand feature for game developers. This feature is high priority for Firefox 25 and this is a blocking bug for doing so. Testing completed (on m-c, etc.): I would like to land on nightly, aurora, and beta in quick succession, ASAP. So far tested locally and on try; landing on nightly and aurora currently blocked by the closed tree. Risk to taking this patch (and alternatives if risky): Changes are limited to the Web Audio API feature, not previously enabled in beta, so I don't expect it to regress currently working content. The alternative is to pref off this feature for ff 25, or release with a partial implementation. String or IDL/UUID changes made by this patch: None

Comment on attachment 805558 [details] [diff] [review] Part 4: Implement PeriodicWave v=10 [Approval Request Comment] Same as parts 3a, 3b, 3c, 3d above. Note this part is the one which flips the switch, exposing the new code from the previous commits. Bug caused by (feature/regressing bug #): 865256 User impact if declined: This and the previous patches implement the the PeriodicWave object for creating custom oscillators in the Web Audio API. Web Audio is a high-demand feature for game developers. This feature is high priority for Firefox 25 and this is a blocking bug for doing so. Testing completed (on m-c, etc.): I would like to land on nightly, aurora, and beta in quick succession, ASAP. So far tested locally and on try; landing on nightly and aurora currently blocked by the closed tree. Risk to taking this patch (and alternatives if risky): Changes are limited to the Web Audio API feature, not previously enabled in beta, so I don't expect it to regress currently working content. The alternative is to pref off this feature for ff 25, or release with a partial implementation. String or IDL/UUID changes made by this patch: None

Rebase patch, fixing merge conflicts with bug 908669. Paul, see comment 48 for other changes since last review.

Updated with build fixes from try. Rebased on top of the rollup from bug 908669. https://tbpl.mozilla.org/?tree=Try&rev=5736e32bf082

Remove duplicate case error from conflict resolution. Local build actually beat try this time! https://tbpl.mozilla.org/?tree=Try&rev=d849073e3641

Comment on attachment 806132 [details] [diff] [review] Part 4: Implement PeriodicWave v13 Review of attachment 806132 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the OOM handling. ::: content/media/webaudio/PeriodicWave.cpp @@ +5,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "PeriodicWave.h" > #include "AudioContext.h" > +#include "AudioNodeEngine.h" This is #included from the header, no need to #include it again here. @@ +33,5 @@ > + > + // Copy coefficient data. The two arrays share an allocation. > + mCoefficients = new ThreadSharedFloatArrayBufferList(2); > + float* buffer = static_cast<float*>(malloc(aLength*sizeof(float)*2)); > + MOZ_ASSERT(buffer, "allocation failure"); Sorry I did not catch this earlier, you can't just call malloc and assume that it succeeds. :-) You need to actually handle OOMs here.

Updated patch. Is this what you had in mind? - Make PeriodicWave creator fallible and throw an OOM ErrorResult on malloc failure. - Remove redundant include. - Restore OscillatorNodeEngine::SetBuffer, omitted in the previous rebase.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bab56f77e839 https://hg.mozilla.org/integration/mozilla-inbound/rev/5923e96dcd67 https://hg.mozilla.org/integration/mozilla-inbound/rev/85db8f09f7c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/5377bce3b478 https://hg.mozilla.org/integration/mozilla-inbound/rev/532300ec4fb6

Comment on attachment 806331 [details] [diff] [review] Part 4: Implement PeriodicWave v14 [Approval Request Comment] Same as parts 3a, 3b, 3c, 3d above. Note this part is the one which flips the switch, exposing the new code from the previous commits. Bug caused by (feature/regressing bug #): 865256 User impact if declined: This and the previous patches implement the PeriodicWave object for creating custom oscillators in the Web Audio API. Web Audio is a high-demand feature for game developers and high priority for Firefox 25. Testing completed (on m-c, etc.): Pushed to inbound this morning. Would like to land on aurora and beta in quick succession, as soon as inbound merges to m-c. Also tested locally and on try. Risk to taking this patch (and alternatives if risky): Changes are limited to the Web Audio API, not previously enabled in beta, so I don't expect it to regress currently working content. The alternative is to pref off this feature, or release with a partial implementation. String or IDL/UUID changes made by this patch: None

Comment on attachment 802497 [details] [diff] [review] Part 3a: AudioBufferPeakValue utility [Approval Request Comment] Bug caused by (feature/regressing bug #): 802497 User impact if declined: Web Audio API will not be available, or will not offer custom oscillator types. This is a high-priority feature, mostly to enable game development. Testing completed (on m-c, etc.): Landed on inbound. Tested locally and on try. Risk to taking this patch (and alternatives if risky): This patch in the series adds a utility function used only by later patches. Risk is very low. String or IDL/UUID changes made by this patch: None

Comment on attachment 802500 [details] [diff] [review] Part 3b: New ifft method on FFTBlock [Approval Request Comment] Bug caused by (feature/regressing bug #): 802497 User impact if declined: Web Audio API will not be available, or will not offer custom oscillator types. This is a high-priority feature, mostly to enable game development. Testing completed (on m-c, etc.): Landed on inbound. Tested locally and on try. Risk to taking this patch (and alternatives if risky): This patch in the series adds a web audio object method used only by later patches. Risk is very low. String or IDL/UUID changes made by this patch: None

Comment on attachment 805100 [details] [diff] [review] Part 3c: Import blink's PeriodicWave v9 [Approval Request Comment] Bug caused by (feature/regressing bug #): 802497 User impact if declined: Web Audio API will not be available, or will not offer custom oscillator types. This is a high-priority feature, mostly to enable game development. Testing completed (on m-c, etc.): Landed on inbound. Tested locally and on try. Risk to taking this patch (and alternatives if risky): This patch in the series imports code for use by later patches. NPOTB. Risk is very low. String or IDL/UUID changes made by this patch: None

Comment on attachment 805423 [details] [diff] [review] Part 3d: Port blink's PeriodicWave to gecko v10 [Approval Request Comment] Bug caused by (feature/regressing bug #): 802497 User impact if declined: Web Audio API will not be available, or will not offer custom oscillator types. This is a high-priority feature, mostly to enable game development. Testing completed (on m-c, etc.): Landed on inbound. Tested locally and on try. Risk to taking this patch (and alternatives if risky): This patch in the series computes the custom oscillator data within the web audio API implementation. Risk is low. String or IDL/UUID changes made by this patch: None

Comment on attachment 806331 [details] [diff] [review] Part 4: Implement PeriodicWave v14 [Approval Request Comment] Bug caused by (feature/regressing bug #): 802497 User impact if declined: Web Audio API will not be available, or will not offer custom oscillator types. This is a high-priority feature, mostly to enable game development. Testing completed (on m-c, etc.): Landed on inbound. Tested locally and on try. Risk to taking this patch (and alternatives if risky): This final patch in the series implements the content-facing portion of this feature. Changes are limited to the new Web Audio API code. Risk is low. String or IDL/UUID changes made by this patch: None

https://hg.mozilla.org/mozilla-central/rev/bab56f77e839 https://hg.mozilla.org/mozilla-central/rev/5923e96dcd67 https://hg.mozilla.org/mozilla-central/rev/85db8f09f7c3 https://hg.mozilla.org/mozilla-central/rev/5377bce3b478 https://hg.mozilla.org/mozilla-central/rev/532300ec4fb6

Comment on attachment 802497 [details] [diff] [review] Part 3a: AudioBufferPeakValue utility It's been on central for 5 days so we can certainly uplift to Aurora, but will let Alex make the call on when this can get to Beta.

Thanks, Lucas. https://hg.mozilla.org/releases/mozilla-aurora/rev/e543dc7ed4a8 https://hg.mozilla.org/releases/mozilla-aurora/rev/54540fd082e3 https://hg.mozilla.org/releases/mozilla-aurora/rev/856ffe673669 https://hg.mozilla.org/releases/mozilla-aurora/rev/da050f22d76a https://hg.mozilla.org/releases/mozilla-aurora/rev/4b833aca87d4

Comment on attachment 802497 [details] [diff] [review] Part 3a: AudioBufferPeakValue utility Coming back through triage again - I think we should get this landed today so it's in an early Beta and there's more time (and population) for evaluation of this change.

https://hg.mozilla.org/releases/mozilla-beta/rev/d3f322df1668 https://hg.mozilla.org/releases/mozilla-beta/rev/aac5bf5f1d6c https://hg.mozilla.org/releases/mozilla-beta/rev/155090b1191d https://hg.mozilla.org/releases/mozilla-beta/rev/eafe4630e68f https://hg.mozilla.org/releases/mozilla-beta/rev/d72751c410c4

Thanks Ryan, Lukas.

Ralph, does this need some extra QA given uplift to Beta? If so, please advise.

There are mochitests, so I'm not worried outside of b2g, which I think is skipping the 25 release. If someone could verify sound comes out of the speaker/headphones on all the platforms (and the tone matches chrome's output) that would be helpful. https://people.mozilla.org/~rgiles/2013/osc07.html

Verified as fixed with latest beta (25 beta 4, build ID: 20131001024718) and latest Aurora, on: Win 7 64 bit, Ubuntu 12.10 32-bit and Mac OS X 10.7.5, in both 32 and 64-bit mode. I can hear the sound from https://people.mozilla.org/~rgiles/2013/osc07.html. Note: this URL only functions with Chrome on Mac. On Linux and Win I can see this message on the page: " TypeError: Object #<AudioContext> has no method 'createPeriodicWave' ", as it shows in the attached screenshot.

How strange. What version of Chromium is that? I've only tested against Chrome (usually canary) on Mac. In any case, thanks for verifying on all our desktop platforms!

(In reply to Ralph Giles (:rillian) from comment #76) > How strange. What version of Chromium is that? I've only tested against > Chrome (usually canary) on Mac. > How strange. What version of Chromium is that? I've only tested against > Chrome (usually canary) on Mac. On Ubuntu: - Chrome version: 28.0.1500.95 - Chromium version: 25.0.1364.160 Ubuntu 12.10 (25.0.1364.160-0ubuntu0.12.10.1) On Win: - Chrome version tested: 29.0.1547.76 m - today, Chrome updated to 30.0.1599.66 m, and now the URL from comment 75 works > In any case, thanks for verifying on all our desktop platforms! Sure, no problem :)

This doesn't work for me on Mac 10.6.8 with Nightly build 20131024030204 The same build does work on Win 8. Confirmed on same Mac machine that latest Aurora and 25b11 work. Do you want a new bug for Mac Nightly failure?

(In reply to comment #78) > This doesn't work for me on Mac 10.6.8 with Nightly build 20131024030204 > > The same build does work on Win 8. > > Confirmed on same Mac machine that latest Aurora and 25b11 work. > > Do you want a new bug for Mac Nightly failure? What's "this"? :-) Do you have a test case that doesn't work?

(In reply to [:tracy] Tracy Walker - QA Mentor from comment #78) > Do you want a new bug for Mac Nightly failure? New bug please. This one is already very long.

The test from comment #74: https://people.mozilla.org/~rgiles/2013/osc07.html doesn't work on latest Mac Nightly. I get wave form, but no audio tone.

Confirmed with 27.0a1 (2013-10-18). My previous version worked. Again, please open a new bug.

You're probably hitting bug 930764 if that affects all Web Audio content.

Yes, the test in bug 930764 fails for me, as well. marking this bug as dependent on 930764 for final verification sake. Thanks guys!

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 Verified as fixed in latest Aurora 27.0a2 (buildID: 20131105004004) using the TC from comment 81 and from bug 930189.

Documentation is up-to-date: https://developer.mozilla.org/en-US/docs/Web/API/PeriodicWave