If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement PeriodicWave

VERIFIED FIXED in Firefox 25

Status

()

Core
Web Audio
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: rillian)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla27
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25+ verified, firefox26+ verified, firefox27+ verified)

Details

(Whiteboard: [blocking-webaudio+])

Attachments

(8 attachments, 21 obsolete attachments)

12.89 KB, patch
roc
: review+
Ehsan
: checkin+
Details | Diff | Splinter Review
16.16 KB, patch
roc
: review+
Ehsan
: checkin+
Details | Diff | Splinter Review
1.65 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
2.21 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
16.22 KB, patch
rillian
: review+
Details | Diff | Splinter Review
20.70 KB, patch
rillian
: review+
Details | Diff | Splinter Review
17.80 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
158.96 KB, image/png
Details
Comment hidden (empty)
I'll prepare a patch for the DOM bindings part, ETA some time today.
Assignee: nobody → giles
Created attachment 754525 [details] [diff] [review]
Part 1: DOM bindings
Attachment #754525 - Flags: review?(roc)
Attachment #754525 - Flags: review?(roc) → review+
Whiteboard: [leave open]
Comment on attachment 754525 [details] [diff] [review]
Part 1: DOM bindings

https://hg.mozilla.org/integration/mozilla-inbound/rev/f95813c97dd6
Attachment #754525 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/f95813c97dd6
Flags: in-testsuite+
Whiteboard: [leave open]
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
See <https://dvcs.w3.org/hg/audio/rev/7c4a40a9bb57>.  We need to change our implementation according to that.
Created attachment 764707 [details] [diff] [review]
Part 2: Rename WaveTable to PeriodicWave
Attachment #764707 - Flags: review?(roc)
Created attachment 764712 [details] [diff] [review]
Part 2: Rename WaveTable to PeriodicWave

Forgot to add the Bindings.conf entry...
Attachment #764707 - Attachment is obsolete: true
Attachment #764707 - Flags: review?(roc)
Attachment #764712 - Flags: review?(roc)
Attachment #764712 - Flags: review?(roc) → review+
Comment on attachment 764712 [details] [diff] [review]
Part 2: Rename WaveTable to PeriodicWave

https://hg.mozilla.org/integration/mozilla-inbound/rev/af744b5304d8
Attachment #764712 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/af744b5304d8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

4 years ago
Whiteboard: [blocking-webaudio+]
(Assignee)

Updated

4 years ago
Summary: Implement WaveTable → Implement PeriodicWave
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?
Attachment #801040 - Flags: feedback?(ehsan)
Created attachment 801043 [details] [diff] [review]
Part 4: Implement PeriodicWave

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.
Attachment #801040 - Flags: feedback?(ehsan) → feedback+
(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?
Created attachment 802497 [details] [diff] [review]
Part 3a: AudioBufferPeakValue utility

Add an equivalent to blink's VectorMath::vmaxmgv.
Attachment #802497 - Flags: review?(ehsan)
Created attachment 802500 [details] [diff] [review]
Part 3b: New ifft method on FFTBlock

Add and external to external ifft method it FFTFrame so we can convert the coefficient data.
Attachment #802500 - Flags: review?(ehsan)
Created attachment 802503 [details] [diff] [review]
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?
Attachment #801040 - Attachment is obsolete: true
Attachment #802503 - Flags: feedback?(karlt)
Created attachment 802504 [details] [diff] [review]
Part 4: Implement PeriodicWave

Updated in response to to ehsan's feedback. Doesn't call the new blink code yet.
Attachment #801043 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #802503 - Attachment description: Part 3cd: import and port blinnk's PeriodicWave implementation → Part 3cd: import and port blink's PeriodicWave implementation
(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.
Attachment #802497 - Flags: review?(ehsan) → review+
Attachment #802500 - Flags: review?(ehsan) → review+
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.
Attachment #802503 - Flags: review-
(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.
Created attachment 803417 [details] [diff] [review]
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.
Created attachment 803418 [details] [diff] [review]
Part 3d: Port blink's PeriodicWave to gecko

Updated patch in response to feedback from karl and ehsan. Thanks guys!
Attachment #802503 - Attachment is obsolete: true
Attachment #802503 - Flags: feedback?(karlt)
Created attachment 803422 [details] [diff] [review]
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'.
Attachment #802504 - Attachment is obsolete: true
This version of the patches is also available as https://github.com/rillian/firefox/commits/periodic6-rollup if that's an easier reference.
Created attachment 804108 [details] [diff] [review]
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.
Attachment #803418 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #804108 - Attachment description: 0004-Bug-865256-Part-3d-Port-blink-s-PeriodicWave-to-geck.patch → Part 3d: Port blink's PeriodicWave to gecko
Created attachment 804111 [details] [diff] [review]
Part 4: Implement PeriodicWave

WIP. Call createBandLimitedTables, fix associated bugs.
Attachment #803422 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #803417 - Flags: review?(ehsan)
Created attachment 805038 [details] [diff] [review]
Part 3d: Port blink's PeriodicWave to gecko. v8
Attachment #804108 - Attachment is obsolete: true
Attachment #805038 - Flags: review?(ehsan)
Created attachment 805039 [details] [diff] [review]
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.
Attachment #803417 - Attachment is obsolete: true
Attachment #804111 - Attachment is obsolete: true
Attachment #803417 - Flags: review?(ehsan)
Attachment #805039 - Flags: review?(ehsan)
(Assignee)

Updated

4 years ago
Attachment #803417 - Attachment is obsolete: false
Attachment #803417 - Flags: review?(ehsan)
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!
Attachment #803417 - Flags: review?(ehsan) → review+
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.
Attachment #805038 - Flags: review?(ehsan) → review+
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.
Attachment #805039 - Flags: review?(paul)
Attachment #805039 - Flags: review?(ehsan)
Attachment #805039 - Flags: review+
(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.
Created attachment 805080 [details] [diff] [review]
Part 3c: Import blink's PeriodicWave v9

Update commit message with blink revision. Carrying forward r=ehsan
Attachment #803417 - Attachment is obsolete: true
Attachment #805080 - Flags: review+
Created attachment 805083 [details] [diff] [review]
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.
Attachment #805038 - Attachment is obsolete: true
Attachment #805083 - Flags: review+
Created attachment 805084 [details] [diff] [review]
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.
Attachment #805039 - Attachment is obsolete: true
Attachment #805039 - Flags: review?(paul)
Attachment #805084 - Flags: review?(paul)
Created attachment 805100 [details] [diff] [review]
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
Attachment #805080 - Attachment is obsolete: true
Attachment #805100 - Flags: review+
Created attachment 805102 [details] [diff] [review]
Part 3d: Port blink's PeriodicWave to gecko. v9

Correct updated patch file. Carrying forward r=ehsan.
Attachment #805083 - Attachment is obsolete: true
Attachment #805102 - Flags: review+
Created attachment 805104 [details] [diff] [review]
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.
Attachment #805084 - Attachment is obsolete: true
Attachment #805084 - Flags: review?(paul)
Attachment #805104 - Flags: review?(paul)
https://tbpl.mozilla.org/?tree=Try&rev=a1fcd6c774a9
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.
Attachment #805104 - Flags: review?(paul) → review+
Created attachment 805423 [details] [diff] [review]
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.
Attachment #805102 - Attachment is obsolete: true
Attachment #805423 - Flags: review+
(Assignee)

Updated

4 years ago
Blocks: 916897
Created attachment 805558 [details] [diff] [review]
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.
Attachment #805104 - Attachment is obsolete: true
Attachment #805558 - Flags: review+
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
Attachment #802497 - Flags: approval-mozilla-beta?
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
Attachment #802500 - Flags: approval-mozilla-beta?
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
Attachment #805100 - Flags: approval-mozilla-beta?
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
Attachment #805423 - Flags: approval-mozilla-beta?
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
Attachment #805558 - Flags: approval-mozilla-beta?
Created attachment 806051 [details] [diff] [review]
Part 4: Implement PeriodicWave v11

Rebase patch, fixing merge conflicts with bug 908669.

Paul, see comment 48 for other changes since last review.
Attachment #805558 - Attachment is obsolete: true
Attachment #805558 - Flags: approval-mozilla-beta?
Attachment #806051 - Flags: review?(paul)
https://tbpl.mozilla.org/?tree=Try&rev=a6e45a46e327
Depends on: 908669
Created attachment 806111 [details] [diff] [review]
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
Attachment #806051 - Attachment is obsolete: true
Attachment #806051 - Flags: review?(paul)
Attachment #806111 - Flags: review?(paul)
Created attachment 806132 [details] [diff] [review]
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
Attachment #806111 - Attachment is obsolete: true
Attachment #806111 - Flags: review?(paul)
Attachment #806132 - Flags: review?(ehsan)
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.
Attachment #806132 - Flags: review?(ehsan) → review+
Created attachment 806331 [details] [diff] [review]
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.
Attachment #806132 - Attachment is obsolete: true
Attachment #806331 - Flags: review?(ehsan)
Attachment #806331 - Flags: review?(ehsan) → review+
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
Attachment #806331 - Flags: approval-mozilla-beta?
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
Attachment #802497 - Flags: approval-mozilla-aurora?
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
Attachment #802500 - Flags: approval-mozilla-aurora?
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
Attachment #805100 - Flags: approval-mozilla-aurora?
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
Attachment #805423 - Flags: approval-mozilla-aurora?
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
Attachment #806331 - Flags: approval-mozilla-aurora?
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
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla24 → mozilla27
Keywords: dev-doc-needed

Updated

4 years ago
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox27: --- → fixed
tracking-firefox25: --- → +
tracking-firefox26: --- → +
tracking-firefox27: --- → +
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.
Attachment #802497 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #802500 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #805100 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #805423 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #806331 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
status-firefox26: affected → fixed
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.
Attachment #802497 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #802500 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #805100 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #805423 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #806331 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
status-firefox25: affected → fixed
Thanks Ryan, Lukas.
Ralph, does this need some extra QA given uplift to Beta? If so, please advise.
Flags: needinfo?(giles)
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
Flags: needinfo?(giles)
Keywords: verifyme
Created attachment 813101 [details]
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.

Updated

4 years ago
status-firefox25: fixed → verified
status-firefox26: fixed → verified
QA Contact: manuela.muntean
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!
Depends on: 930764
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.
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
Keywords: verifyme
Depends on: 981931
Documentation is up-to-date:
https://developer.mozilla.org/en-US/docs/Web/API/PeriodicWave
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1074765
Depends on: 1012609
You need to log in before you can comment on or make changes to this bug.