865256 - Implement PeriodicWave

Status: VERIFIED FIXED

(Core :: Web Audio, defect)

Comment 1

[(no longer active)]

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

Comment 2

[(no longer active)]

Attachment: Part 1: DOM bindings

Comment 3

[(no longer active)]

Comment on attachment 754525 [details] [diff] [review]
Part 1: DOM bindings

https://hg.mozilla.org/integration/mozilla-inbound/rev/f95813c97dd6

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f95813c97dd6

Comment 5

[(no longer active)]

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

Comment 6

[(no longer active)]

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

Comment 7

[(no longer active)]

Attachment: Part 2: Rename WaveTable to PeriodicWave

Comment 8

[(no longer active)]

Attachment: Part 2: Rename WaveTable to PeriodicWave

Forgot to add the Bindings.conf entry...

Comment 9

[(no longer active)]

Comment on attachment 764712 [details] [diff] [review]
Part 2: Rename WaveTable to PeriodicWave

https://hg.mozilla.org/integration/mozilla-inbound/rev/af744b5304d8

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/af744b5304d8

Comment 11

[Ralph Giles (:rillian)]

Attachment: 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?

Comment 12

[Ralph Giles (:rillian)]

Attachment: Part 4: Implement PeriodicWave

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

Comment 13

[(no longer active)]

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.

Comment 14

[(no longer active)]

(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 15

[(no longer active)]

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.

Comment 16

[Ralph Giles (:rillian)]

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?

Comment 17

[Ralph Giles (:rillian)]

Attachment: Part 3a: AudioBufferPeakValue utility

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

Comment 18

[Ralph Giles (:rillian)]

Attachment: Part 3b: New ifft method on FFTBlock

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

Comment 19

[Ralph Giles (:rillian)]

Attachment: Part 3cd: import and port blink's PeriodicWave implementation

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?

Comment 20

[Ralph Giles (:rillian)]

Attachment: Part 4: Implement PeriodicWave

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

Comment 21

[Ralph Giles (:rillian)]

(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[]"

Comment 22

[Karl Tomlinson (:karlt)]

(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 23

[(no longer active)]

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.

Comment 24

[Ralph Giles (:rillian)]

(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.

Comment 25

[Ralph Giles (:rillian)]

Attachment: Part 3c: Import blink's PeriodicWave

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

Comment 26

[Ralph Giles (:rillian)]

Attachment: Part 3d: Port blink's PeriodicWave to gecko

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

Comment 27

[Ralph Giles (:rillian)]

Attachment: Part 4: Implement PeriodicWave

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'.

Comment 28

[Ralph Giles (:rillian)]

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

Comment 29

[Ralph Giles (:rillian)]

Attachment: Part 3d: Port blink's PeriodicWave to gecko

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.

Comment 30

[Ralph Giles (:rillian)]

Attachment: Part 4: Implement PeriodicWave

WIP. Call createBandLimitedTables, fix associated bugs.

Comment 31

[Ralph Giles (:rillian)]

Attachment: Part 3d: Port blink's PeriodicWave to gecko. v8

Comment 32

[Ralph Giles (:rillian)]

Attachment: Part 4: Implement PeriodicWave v8

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 33

[(no longer active)]

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 34

[(no longer active)]

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 35

[(no longer active)]

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.

Comment 36

[Ralph Giles (:rillian)]

(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.

Comment 37

[Ralph Giles (:rillian)]

I meant nsRefPtr, not nsAutoPtr.

Comment 38

[Ralph Giles (:rillian)]

Attachment: Part 3c: Import blink's PeriodicWave v9

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

Comment 39

[Ralph Giles (:rillian)]

Attachment: Part 3d: Port blink's PeriodicWave to gecko. v9

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

Comment 40

[Ralph Giles (:rillian)]

Attachment: Part 4: Implement PeriodicWave v9

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.

Comment 41

[Ralph Giles (:rillian)]

Attachment: Part 3c: Import blink's PeriodicWave v9

*sigh*. This is the actual updated patch. Previous ones were from a stale export.

Carrying forward r=ehsan

Comment 42

[Ralph Giles (:rillian)]

Attachment: Part 3d: Port blink's PeriodicWave to gecko. v9

Correct updated patch file. Carrying forward r=ehsan.

Comment 43

[Ralph Giles (:rillian)]

Attachment: bug865256-4-v9.patch

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.

Comment 44

[Ralph Giles (:rillian)]

https://tbpl.mozilla.org/?tree=Try&rev=a1fcd6c774a9

Comment 45

[Ralph Giles (:rillian)]

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 46

[Paul Adenot (:padenot)]

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.

Comment 47

[Ralph Giles (:rillian)]

Attachment: Part 3d: Port blink's PeriodicWave to gecko v10

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

Comment 48

[Ralph Giles (:rillian)]

Attachment: Part 4: Implement PeriodicWave v=10

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 49

[Ralph Giles (:rillian)]

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 50

[Ralph Giles (:rillian)]

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 51

[Ralph Giles (:rillian)]

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 52

[Ralph Giles (:rillian)]

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 53

[Ralph Giles (:rillian)]

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

Comment 54

[Ralph Giles (:rillian)]

Attachment: Part 4: Implement PeriodicWave v11

Rebase patch, fixing merge conflicts with bug 908669.

Paul, see comment 48 for other changes since last review.

Comment 55

[Ralph Giles (:rillian)]

https://tbpl.mozilla.org/?tree=Try&rev=a6e45a46e327

Comment 56

[Ralph Giles (:rillian)]

Attachment: Part 4: Implement PeriodicWave v12

Updated with build fixes from try. Rebased on top of the rollup from bug 908669.

https://tbpl.mozilla.org/?tree=Try&rev=5736e32bf082

Comment 57

[Ralph Giles (:rillian)]

Attachment: Part 4: Implement PeriodicWave v13

Remove duplicate case error from conflict resolution. Local build actually beat try this time!

https://tbpl.mozilla.org/?tree=Try&rev=d849073e3641

Comment 58

[(no longer active)]

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.

Comment 59

[Ralph Giles (:rillian)]

Attachment: Part 4: Implement PeriodicWave v14

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.

Comment 60

[Ralph Giles (:rillian)]

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 61

[Ralph Giles (:rillian)]

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 62

[Ralph Giles (:rillian)]

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 63

[Ralph Giles (:rillian)]

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 64

[Ralph Giles (:rillian)]

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 65

[Ralph Giles (:rillian)]

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 66

[Ralph Giles (:rillian)]

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

Comment 67

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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 68

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 69

[Ralph Giles (:rillian)]

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 70

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 71

[Ryan VanderMeulen [:RyanVM]]

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

Comment 72

[Ralph Giles (:rillian)]

Thanks Ryan, Lukas.

Comment 73

[u279076]

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

Comment 74

[Ralph Giles (:rillian)]

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

Comment 75

[Manuela Muntean [Away]]

Attachment: Screenshot.png

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.

Comment 76

[Ralph Giles (:rillian)]

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!

Comment 77

[Manuela Muntean [Away]]

(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 :)

Comment 78

[Tracy Walker [:tracy]]

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?

Comment 79

[(no longer active)]

(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?

Comment 80

[Ralph Giles (:rillian)]

(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.

Comment 81

[Tracy Walker [:tracy]]

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.

Comment 82

[Ralph Giles (:rillian)]

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

Comment 83

[(no longer active)]

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

Comment 84

[Tracy Walker [:tracy]]

Yes, the test in bug 930764 fails for me, as well. 

marking this bug as dependent on 930764 for final verification sake.  

Thanks guys!

Comment 85

[Bogdan Maris, Desktop QA]

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.

Comment 86

[Jean-Yves Perrier [:teoli]]

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