Closed Bug 923106 Opened 6 years ago Closed 6 years ago

Sound is not changed in the Oscillator audio source node

Categories

(Core :: Web Audio, defect)

25 Branch
x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox24 --- unaffected
firefox25 - wontfix
firefox26 - verified
firefox27 --- verified

People

(Reporter: simona.marcu, Assigned: karlt)

References

Details

(Keywords: regression, Whiteboard: [see comments 2 and 4 when verifying])

Attachments

(1 file)

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.
Karl, can you take a look please?  Thanks!
Assignee: nobody → 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.
Status: NEW → ASSIGNED
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.
Attachment #813442 - Flags: review?(paul)
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.
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.
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...
Blocks: webaudio
Attachment #813442 - Flags: review?(paul) → review+
Nominating for tracking given this blocks bug 779297.
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.
Flags: needinfo?(ehsan)
This shouldn't block Web Audio in FF25 but we should get a fix in if we can.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc486a6f645
Flags: in-testsuite?
Whiteboard: [see comments 2 and 4 when verifying]
Flags: in-testsuite? → in-testsuite+
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
Attachment #813442 - Flags: approval-mozilla-beta?
Attachment #813442 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7fc486a6f645
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(In reply to lsblakk@mozilla.com [: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.
Flags: needinfo?(ehsan)
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.
Attachment #813442 - Flags: approval-mozilla-beta?
Attachment #813442 - Flags: approval-mozilla-beta-
Attachment #813442 - Flags: approval-mozilla-aurora?
Attachment #813442 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 929621
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.
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.