1322883 - AudioNode constructors

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Andrea Marchesini [:baku]]

AudioNodes should have constructors. This bug is about implementing them.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 0 - AnalyserNode

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - AudioBufferSourceNode

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - AudioBufferSourceNode

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - GainNode

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3 - DelayNode

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 4 - biquadFilter

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 4 - biquadFilter

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 5 - IIRFilter

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 6 - WaveShaperNode

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 7 - PannerNode

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 8 - StereoPannerNode

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 9 - ConvolverNode

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part A - ChannelSplitterNode

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part B - ChannelMergerNode

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part D - OscillatorNode

Comment 16

[Andrea Marchesini [:baku]]

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

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part E - PeriodicWave

Read the previous comment

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part F - AudioBuffer

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part G - MediaElementAudioSource

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part H - MediaStreamAudioSource

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part I - MediaStreamDestination

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 6 - WaveShaperNode

Comment 23

[Paul Adenot (:padenot)]

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.

Comment 24

[Paul Adenot (:padenot)]

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.

Comment 25

[Paul Adenot (:padenot)]

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.

Comment 26

[Paul Adenot (:padenot)]

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.

Comment 27

[Paul Adenot (: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 ?

Comment 28

[Andrea Marchesini [:baku]]

Attachment: part 9 - ConvolverNode

Comment 29

[Andrea Marchesini [:baku]]

Attachment: part E - PeriodicWave

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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

Comment 31

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/339a11acf171
https://hg.mozilla.org/mozilla-central/rev/64e8b5cc289f
https://hg.mozilla.org/mozilla-central/rev/912981c8482f
https://hg.mozilla.org/mozilla-central/rev/b646b6a9bab6
https://hg.mozilla.org/mozilla-central/rev/fd9c173ccdaa
https://hg.mozilla.org/mozilla-central/rev/aa5304b535f8
https://hg.mozilla.org/mozilla-central/rev/8895dafe20fe
https://hg.mozilla.org/mozilla-central/rev/473f4d9e733e
https://hg.mozilla.org/mozilla-central/rev/cdf7cf225e9f
https://hg.mozilla.org/mozilla-central/rev/7599aaf0e1dc
https://hg.mozilla.org/mozilla-central/rev/269333231452
https://hg.mozilla.org/mozilla-central/rev/eea2997c1f91
https://hg.mozilla.org/mozilla-central/rev/8aa5c90d8192
https://hg.mozilla.org/mozilla-central/rev/ee819615a35a
https://hg.mozilla.org/mozilla-central/rev/32d6100a73ad
https://hg.mozilla.org/mozilla-central/rev/6242dead061e
https://hg.mozilla.org/mozilla-central/rev/9c1c90ad617b
https://hg.mozilla.org/mozilla-central/rev/79147ddefd2e
https://hg.mozilla.org/mozilla-central/rev/9df23f010cf9

Comment 33

[Chris Mills [:cmills]]

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!

Comment 34

[Paul Adenot (:padenot)]

That's amazing Chris, thanks! I had a look, I think you covered everything!