923106 - Sound is not changed in the Oscillator audio source node

Status: VERIFIED FIXED

(Core :: Web Audio, defect)

Description

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.0; rv:25.0) Gecko/20100101 Firefox/25.0
Build ID: 20131001024718

1. Navigate to http://webaudioplayground.appspot.com/ 
2. For the Audio Source select OSCILLATOR
3. Select any module - for e.g: Biquad Filter
4. Connect the nodes
5. Press the Play button on the OSCILLATOR
6. While the sound is playing change them from the Oscillator drop-down.

Expected results:
The sound is changed every time a new one is selected (at least on Chrome 28.0.1500.95 m)

Actual results:
The sound remains the same.

Notes:
Reproducible on Windows, Ubuntu and Mac OS X.
Reproducible on the latest Nightly 27.0a1 and on the latest Aurora 26.0a2.

Comment 1

[(no longer active)]

Karl, can you take a look please?  Thanks!

Comment 2

[Karl Tomlinson (:karlt)]

The playground is using legacy numeric OscillatorTypes, so a new boolean pref named "media.webaudio.legacy.OscillatorNode" needs to created with value "true" to get the expected results.

Even then, the sounds do not change correctly.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: recompute frequency dependent parameters when OscillatorType changes

mSquare, mTriangle, and mSaw are not initialized in the OscillatorNodeEngine
constructor, just like they are not initialized when switching to
OscillatorType::Sine.  These parameters are initialized when switching to the
OscillatorTypes that use them.

mFinalFrequency, mNumberOfHarmonics, mSignalPeriod, mAmplitudeAtZero,
mPhaseIncrement, mPhaseWrap are not initialized in the OscillatorNodeEngine
constructor, just like they are not initialized immediately on switching
OscillatorType.  These parameters are initialized in
UpdateParametersIfNeeded(), conditional on mRecomputeParameters.

It is better to leave initialization to the appropriate functions, rather than initialize to arbitrary values, so that we can more easily detect when they are not initialized when they should be.

Comment 4

[Karl Tomlinson (:karlt)]

The "wavetable" option in the webaudioplayground menu does not work and is not meant to work.  The exception thrown is not expected by the web page, and leads to corrupting the state of the oscillator.  The web page also has a bug when trying to
change the type while the Oscillator is not playing, so that has no effect.

Comment 5

[Karl Tomlinson (:karlt)]

Given, the single start / single stop nature of OscillatorNode, I wouldn't expect changing the type mid stream to be a high-demand feature, and so we don't necessarily need to rush this fix into beta.

Comment 6

[(no longer active)]

Can you please add a comment to the ctor saying why some of the fields are not initialized there?  Otherwise somebody my feel helpful in the future to add those initializations again...

Comment 7

[u279076]

Nominating for tracking given this blocks bug 779297.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Ehsan - does this truly block the ability to ship Web Audio in FF25? Seems like this is a smaller feature that could be a follow up fix in 26 or later if need be.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This shouldn't block Web Audio in FF25 but we should get a fix in if we can.

Comment 10

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc486a6f645

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 813442 [details] [diff] [review]
recompute frequency dependent parameters when OscillatorType changes

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Regression from bug 908669 in a recent, web audio feature.

User impact if declined:

Attempts to change the waveform type of an oscillator will produce incorrect waveforms, often not audible at all, but possibly extra loud, depending on timing.

It was only recently semi-agreed that changing the type of an oscillator would change the waveform [1].  This patch means that we at least get the right kind of waveform if someone tries this.  The phase of the new waveform still differs between Gecko and Chrome - there hasn't been agreement there.

I would not expect changing the type of waveform midstream to be a widely used feature (comment 5), because usually OscillatorNodes are single use.  They cannot be restarted once stopped.  However, there may be some demos, like the one in comment 0, wanting to show the breadth of the web audio API.

I'm only requesting approval for beta because roc asked me to.
 
Testing completed (on m-c, etc.): in-testsuite

Risk to taking this patch (and alternatives if risky): 

Risk is confined to a single web audio feature, which is easy to test.
The patch changes only floating point operations and so there should be no security risk.

Backing out changes in bug 908669 would not make sense, because that bug is going to be more widely noticeable.

String or IDL/UUID changes made by this patch: none

[1] https://github.com/WebAudio/web-audio-api/issues/136

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fc486a6f645

Comment 13

[(no longer active)]

(In reply to [e-mail] [:lsblakk] from comment #8)
> Ehsan - does this truly block the ability to ship Web Audio in FF25? Seems
> like this is a smaller feature that could be a follow up fix in 26 or later
> if need be.

See comment 9 and comment 11.  This is nice to have for 25, but not a blocker.

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 813442 [details] [diff] [review]
recompute frequency dependent parameters when OscillatorType changes

Since this is a nice-to-have, we won't track. That being said, it's also a low risk fix for what could be an annoying experience in this new feature, so we'll take the uplift to Aurora 26.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/45a6b62a0c56

Comment 17

[Bogdan Maris, Desktop QA]

Tested on latest Aurora after setting the pref "media.webaudio.legacy.OscillatorNode" to true I can hear all sounds except "wavetable" which based on comment 4 it should not work so this is verified fixed on 26.

Comment 18

[Ada [:adalucinet]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0

Verified as fixed on latest Aurora (Build ID: 20131104004000) with STRs from comment 0 after setting the pref "media.webaudio.legacy.OscillatorNode" to true: I can hear all sounds except for "wavetable" which does not work and is not meant to work as per comment 4.