Closed Bug 1322883 Opened 3 years ago Closed 3 years ago

AudioNode constructors

Categories

(Core :: Web Audio, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

(Keywords: dev-doc-complete)

Attachments

(18 files, 6 obsolete files)

14.47 KB, patch
padenot
: review+
Details | Diff | Splinter Review
7.56 KB, patch
padenot
: review+
Details | Diff | Splinter Review
5.44 KB, patch
padenot
: review+
Details | Diff | Splinter Review
7.09 KB, patch
padenot
: review+
Details | Diff | Splinter Review
5.81 KB, patch
padenot
: review+
Details | Diff | Splinter Review
8.01 KB, patch
padenot
: review+
Details | Diff | Splinter Review
7.43 KB, patch
padenot
: review+
Details | Diff | Splinter Review
6.41 KB, patch
padenot
: review+
Details | Diff | Splinter Review
6.62 KB, patch
padenot
: review+
Details | Diff | Splinter Review
6.66 KB, patch
padenot
: review+
Details | Diff | Splinter Review
7.08 KB, patch
padenot
: review+
Details | Diff | Splinter Review
7.00 KB, patch
padenot
: review+
Details | Diff | Splinter Review
6.99 KB, patch
padenot
: review+
Details | Diff | Splinter Review
7.32 KB, patch
padenot
: review+
Details | Diff | Splinter Review
6.21 KB, patch
padenot
: review+
Details | Diff | Splinter Review
11.00 KB, patch
padenot
: review+
Details | Diff | Splinter Review
7.16 KB, patch
padenot
: review+
Details | Diff | Splinter Review
4.28 KB, patch
padenot
: review+
Details | Diff | Splinter Review
AudioNodes should have constructors. This bug is about implementing them.
Assignee: nobody → amarchesini
Attachment #8817845 - Flags: review?(padenot)
Attached patch part 1 - AudioBufferSourceNode (obsolete) — Splinter Review
Attachment #8817849 - Flags: review?(padenot)
Attachment #8817849 - Attachment is obsolete: true
Attachment #8817849 - Flags: review?(padenot)
Attachment #8817850 - Flags: review?(padenot)
Attachment #8817851 - Flags: review?(padenot)
Attachment #8817852 - Flags: review?(padenot)
Attached patch part 4 - biquadFilter (obsolete) — Splinter Review
Attachment #8817854 - Flags: review?(padenot)
Attachment #8817854 - Attachment is obsolete: true
Attachment #8817854 - Flags: review?(padenot)
Attachment #8817855 - Flags: review?(padenot)
Attachment #8817856 - Flags: review?(padenot)
Attached patch part 6 - WaveShaperNode (obsolete) — Splinter Review
Attachment #8817887 - Flags: review?(padenot)
Attachment #8817888 - Flags: review?(padenot)
Attachment #8817889 - Flags: review?(padenot)
Attached patch part 9 - ConvolverNode (obsolete) — Splinter Review
Attachment #8817890 - Flags: review?(padenot)
Attachment #8817891 - Flags: review?(padenot)
Attachment #8817892 - Flags: review?(padenot)
Attachment #8817893 - Flags: review?(padenot)
Attached patch part E - PeriodicWave (obsolete) — Splinter Review
Here I do an extra copy and I don't like it too much but I need to know if this can be a real issue. I can have 2 CTORs or find a better solution if you want.
Attachment #8817897 - Flags: review?(padenot)
Attached patch part E - PeriodicWave (obsolete) — Splinter Review
Read the previous comment
Attachment #8817897 - Attachment is obsolete: true
Attachment #8817897 - Flags: review?(padenot)
Attachment #8817899 - Flags: review?(padenot)
Attachment #8817923 - Flags: review?(padenot)
Attachment #8817924 - Flags: review?(padenot)
Attachment #8817925 - Flags: review?(padenot)
Attachment #8817926 - Flags: review?(padenot)
Attachment #8817887 - Attachment is obsolete: true
Attachment #8817887 - Flags: review?(padenot)
Attachment #8818480 - Flags: review?(padenot)
Rank: 15
Priority: -- → P1
Attachment #8817845 - Flags: review?(padenot) → review+
Attachment #8817850 - Flags: review?(padenot) → review+
Comment on attachment 8817851 [details] [diff] [review]
part 2 - GainNode

Review of attachment 8817851 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a simple test that checks that the gain is set by the ctor.

::: dom/media/webaudio/test/test_gainNode.html
@@ +25,1 @@
>  

Passing a gain value in the ctor is not tested here.
Attachment #8817851 - Flags: review?(padenot) → review+
Comment on attachment 8817852 [details] [diff] [review]
part 3 - DelayNode

Review of attachment 8817852 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a couple more tests. We need to check that the two attribute passed are set properly.

::: dom/media/webaudio/test/test_delayNode.html
@@ +60,5 @@
> +      new DelayNode(context, { maxDelayTime: NaN });
> +    }, DOMException.NOT_SUPPORTED_ERR);
> +    expectException(function() {
> +      new DelayNode(context, { maxDelayTime: -1 });
> +    }, DOMException.NOT_SUPPORTED_ERR);

This does not check that the values passed in the ctor are reflected when the node is created.
Attachment #8817852 - Flags: review?(padenot) → review+
Attachment #8817855 - Flags: review?(padenot) → review+
Attachment #8817856 - Flags: review?(padenot) → review+
Attachment #8817888 - Flags: review?(padenot) → review+
Attachment #8817889 - Flags: review?(padenot) → review+
Comment on attachment 8817890 [details] [diff] [review]
part 9 - ConvolverNode

Review of attachment 8817890 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webaudio/ConvolverNode.cpp
@@ +231,5 @@
> +      return nullptr;
> +    }
> +  }
> +
> +  // FIXME: aOptions.disableNormalization?

Yes, this needs to be done, and needs to be done before setting the buffer, to save some CPU.

Just do `audioNode->setNormalize(!aOptions.disableNormalization)` _before_ the buffer is set (otherwise, per spec, the attribute change should not do anything).

See this paragraph in the spec:

> If the AudioNode being constructed is a ConvolverNode, set its normalize attribute with the inverse of the value of the disableNormalization dictionary member, and then set its the buffer attribute to the value of the buffer dictionary member, in this order, and jump to the last step of this algorithm.
Attachment #8817890 - Flags: review?(padenot)
Attachment #8817891 - Flags: review?(padenot) → review+
Attachment #8817892 - Flags: review?(padenot) → review+
Attachment #8817893 - Flags: review?(padenot) → review+
Comment on attachment 8817899 [details] [diff] [review]
part E - PeriodicWave

Review of attachment 8817899 [details] [diff] [review]:
-----------------------------------------------------------------

This needs a bit more work, but looks good.

::: dom/media/webaudio/AudioContext.cpp
@@ +508,5 @@
> +
> +  for (uint32_t i = 0; i < realLength; ++i) {
> +    options.mReal.Value()[i] = aRealData.Data()[i];
> +    options.mImag.Value()[i] = aImagData.Data()[i];
> +  }

Yeah indeed it's best to only copy once. Maybe it's best to keep the other ctor as well.

::: dom/media/webaudio/PeriodicWave.cpp
@@ +42,5 @@
>    }
> +
> +  for (uint32_t i = 0; i < mLength; ++i) {
> +    buffer[i] = aReal[i];
> +  }

It's better to use `PodCopy` here. It will use `memcpy` under the hood a produce much faster code.
Attachment #8817899 - Flags: review?(padenot)
Comment on attachment 8817923 [details] [diff] [review]
part F - AudioBuffer

Review of attachment 8817923 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webaudio/test/test_stereoPannerNode.html
@@ +71,5 @@
>  // A couple mono and stereo buffers: the StereoPannerNode equation is different
>  // if the input is mono or stereo
> +var stereoBuffer = new AudioBuffer(context, { numberOfChannels: 2,
> +                                              length: BUF_SIZE,
> +                                              sampleRate: context.sampleRate });

Can you drop the `context.sampleRate` bit here and below, so that this test also tests that if no rate is passed, it default to the context's sampleRate ?
Attachment #8817923 - Flags: review?(padenot) → review+
Attachment #8817924 - Flags: review?(padenot) → review+
Attachment #8817925 - Flags: review?(padenot) → review+
Attachment #8817926 - Flags: review?(padenot) → review+
Attachment #8817890 - Attachment is obsolete: true
Attachment #8818899 - Flags: review?(padenot)
Attachment #8818480 - Flags: review?(padenot) → review+
Attachment #8818899 - Flags: review?(padenot) → review+
Attachment #8817899 - Attachment is obsolete: true
Attachment #8818920 - Flags: review?(padenot)
Attachment #8818920 - Flags: review?(padenot) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/339a11acf171
AudioNode constructors - part 0 - AnalyserNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/64e8b5cc289f
AudioNode constructors - part 1 - AudioBufferSourceNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/912981c8482f
AudioNode constructors - part 2 - GainNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/b646b6a9bab6
AudioNode constructors - part 3 - DelayNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd9c173ccdaa
AudioNode constructors - part 4 - BiquadFilterNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa5304b535f8
AudioNode constructors - part 5 - IIRFilterNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/8895dafe20fe
AudioNode constructors - part 6 - WaveShaperNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/473f4d9e733e
AudioNode constructors - part 7 - PannerNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf7cf225e9f
AudioNode constructors - part 8 - StereoPannerNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/7599aaf0e1dc
AudioNode constructors - part 9 - ConvolverNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/269333231452
AudioNode constructors - part 10 - ChannelSplitterNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/eea2997c1f91
AudioNode constructors - part 11 - ChannelMergerNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa5c90d8192
AudioNode constructors - part 12 - DynamicsCompressorNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee819615a35a
AudioNode constructors - part 13 - OscillatorNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/32d6100a73ad
AudioNode constructors - part 14 - PeriodicWave, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/6242dead061e
AudioNode constructors - part 15 - AudioBuffer, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1c90ad617b
AudioNode constructors - part 16 - MediaElementAudioSourceNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/79147ddefd2e
AudioNode constructors - part 17 - MediaStreamAudioSourceNode, r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/9df23f010cf9
AudioNode constructors - part 18 - MediaStreamAudioDestinationNode, r=padenot
Duplicate of this bug: 1308443
I have made sure all of these are documented, and the browser support info is up-to-date:

https://developer.mozilla.org/en-US/docs/Web/API/PeriodicWave/PeriodicWave
https://developer.mozilla.org/en-US/docs/Web/API/ConvolverNode/ConvolverNode
https://developer.mozilla.org/en-US/docs/Web/API/WaveShaperNode/WaveShaperNode
https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamAudioDestinationNode/MediaStreamAudioDestinationNode
https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamAudioSourceNode/MediaStreamAudioSourceNode
https://developer.mozilla.org/en-US/docs/Web/API/MediaElementAudioSourceNode/MediaElementAudioSourceNode
https://developer.mozilla.org/en-US/docs/Web/API/AudioBuffer/AudioBuffer
https://developer.mozilla.org/en-US/docs/Web/API/OscillatorNode/OscillatorNode
https://developer.mozilla.org/en-US/docs/Web/API/ChannelMergerNode/ChannelMergerNode
https://developer.mozilla.org/en-US/docs/Web/API/ChannelSplitterNode/ChannelSplitterNode
https://developer.mozilla.org/en-US/docs/Web/API/StereoPannerNode/StereoPannerNode
https://developer.mozilla.org/en-US/docs/Web/API/PannerNode/PannerNode
https://developer.mozilla.org/en-US/docs/Web/API/IIRFilterNode/IIRFilterNode
https://developer.mozilla.org/en-US/docs/Web/API/BiquadFilterNode/BiquadFilterNode
https://developer.mozilla.org/en-US/docs/Web/API/DelayNode/DelayNode
https://developer.mozilla.org/en-US/docs/Web/API/GainNode/GainNode
https://developer.mozilla.org/en-US/docs/Web/API/AudioBufferSourceNode/AudioBufferSourceNode
https://developer.mozilla.org/en-US/docs/Web/API/AnalyserNode/AnalyserNode

I have added a page covering the AudioNodeOptions dictionary that all the constructor option objects inherit from:

https://developer.mozilla.org/en-US/docs/Web/API/AudioNodeOptions

I have also added a note to the Fx53 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53#Web_Audio_API

Has anything been missed? Thanks!
That's amazing Chris, thanks! I had a look, I think you covered everything!
You need to log in before you can comment on or make changes to this bug.