AudioNode constructors

RESOLVED FIXED in Firefox 53

Status

()

Core
Web Audio
P1
normal
Rank:
15
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

({dev-doc-complete})

unspecified
mozilla53
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(18 attachments, 6 obsolete attachments)

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

Description

a year ago
AudioNodes should have constructors. This bug is about implementing them.
(Assignee)

Updated

a year ago
Assignee: nobody → amarchesini
(Assignee)

Comment 1

a year ago
Created attachment 8817845 [details] [diff] [review]
part 0 - AnalyserNode
Attachment #8817845 - Flags: review?(padenot)
(Assignee)

Comment 2

a year ago
Created attachment 8817849 [details] [diff] [review]
part 1 - AudioBufferSourceNode
Attachment #8817849 - Flags: review?(padenot)
(Assignee)

Comment 3

a year ago
Created attachment 8817850 [details] [diff] [review]
part 1 - AudioBufferSourceNode
Attachment #8817849 - Attachment is obsolete: true
Attachment #8817849 - Flags: review?(padenot)
Attachment #8817850 - Flags: review?(padenot)
(Assignee)

Comment 4

a year ago
Created attachment 8817851 [details] [diff] [review]
part 2 - GainNode
Attachment #8817851 - Flags: review?(padenot)
(Assignee)

Comment 5

a year ago
Created attachment 8817852 [details] [diff] [review]
part 3 - DelayNode
Attachment #8817852 - Flags: review?(padenot)
(Assignee)

Comment 6

a year ago
Created attachment 8817854 [details] [diff] [review]
part 4 - biquadFilter
Attachment #8817854 - Flags: review?(padenot)
(Assignee)

Comment 7

a year ago
Created attachment 8817855 [details] [diff] [review]
part 4 - biquadFilter
Attachment #8817854 - Attachment is obsolete: true
Attachment #8817854 - Flags: review?(padenot)
Attachment #8817855 - Flags: review?(padenot)
(Assignee)

Comment 8

a year ago
Created attachment 8817856 [details] [diff] [review]
part 5 - IIRFilter
Attachment #8817856 - Flags: review?(padenot)
(Assignee)

Comment 9

a year ago
Created attachment 8817887 [details] [diff] [review]
part 6 - WaveShaperNode
Attachment #8817887 - Flags: review?(padenot)
(Assignee)

Comment 10

a year ago
Created attachment 8817888 [details] [diff] [review]
part 7 - PannerNode
Attachment #8817888 - Flags: review?(padenot)
(Assignee)

Comment 11

a year ago
Created attachment 8817889 [details] [diff] [review]
part 8 - StereoPannerNode
Attachment #8817889 - Flags: review?(padenot)
(Assignee)

Comment 12

a year ago
Created attachment 8817890 [details] [diff] [review]
part 9 - ConvolverNode
Attachment #8817890 - Flags: review?(padenot)
(Assignee)

Comment 13

a year ago
Created attachment 8817891 [details] [diff] [review]
part A - ChannelSplitterNode
Attachment #8817891 - Flags: review?(padenot)
(Assignee)

Comment 14

a year ago
Created attachment 8817892 [details] [diff] [review]
part B - ChannelMergerNode
Attachment #8817892 - Flags: review?(padenot)
(Assignee)

Comment 15

a year ago
Created attachment 8817893 [details] [diff] [review]
part D - OscillatorNode
Attachment #8817893 - Flags: review?(padenot)
(Assignee)

Comment 16

a year ago
Created attachment 8817897 [details] [diff] [review]
part E - PeriodicWave

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

Comment 17

a year ago
Created attachment 8817899 [details] [diff] [review]
part E - PeriodicWave

Read the previous comment
Attachment #8817897 - Attachment is obsolete: true
Attachment #8817897 - Flags: review?(padenot)
Attachment #8817899 - Flags: review?(padenot)
(Assignee)

Comment 18

a year ago
Created attachment 8817923 [details] [diff] [review]
part F - AudioBuffer
Attachment #8817923 - Flags: review?(padenot)
(Assignee)

Comment 19

a year ago
Created attachment 8817924 [details] [diff] [review]
part G - MediaElementAudioSource
Attachment #8817924 - Flags: review?(padenot)
(Assignee)

Comment 20

a year ago
Created attachment 8817925 [details] [diff] [review]
part H - MediaStreamAudioSource
Attachment #8817925 - Flags: review?(padenot)
(Assignee)

Comment 21

a year ago
Created attachment 8817926 [details] [diff] [review]
part I - MediaStreamDestination
Attachment #8817926 - Flags: review?(padenot)
(Assignee)

Comment 22

a year ago
Created attachment 8818480 [details] [diff] [review]
part 6 - WaveShaperNode
Attachment #8817887 - Attachment is obsolete: true
Attachment #8817887 - Flags: review?(padenot)
Attachment #8818480 - Flags: review?(padenot)

Updated

a year ago
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+
(Assignee)

Comment 28

a year ago
Created attachment 8818899 [details] [diff] [review]
part 9 - ConvolverNode
Attachment #8817890 - Attachment is obsolete: true
Attachment #8818899 - Flags: review?(padenot)
Attachment #8818480 - Flags: review?(padenot) → review+
Attachment #8818899 - Flags: review?(padenot) → review+
(Assignee)

Comment 29

a year ago
Created attachment 8818920 [details] [diff] [review]
part E - PeriodicWave
Attachment #8817899 - Attachment is obsolete: true
Attachment #8818920 - Flags: review?(padenot)
Attachment #8818920 - Flags: review?(padenot) → review+

Comment 30

a year ago
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

Updated

8 months ago
Duplicate of this bug: 1308443
Keywords: dev-doc-needed
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!
Keywords: dev-doc-needed → dev-doc-complete
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.