I'll prepare a patch for the DOM bindings part, ETA some time today.

Rebased. I won't land this for now as this patch on its own will add a node which doesn't do anything...

Rebased on top of bug 876024.

(This really belongs in bug 879014, but the part 1 patch here has not landed yet.)

Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.

I wrote a simplest-possible test page. http://people.mozilla.org/~rgiles/2013/osc01.html

Rebased on top of inbound.

Rebased on top of bug 884632 which I just landed on inbound.

Rebased on top of https://hg.mozilla.org/integration/mozilla-inbound/rev/af744b5304d8.

Rebase part two on m-c + part one. The current patch didn't 'git am' cleanly for me.

In the spirit of something-is-better-than-nothing, here's the a cleaned up version of the 'beep' patch I did a week or two ago. This generates a sine wave directly. We probably want to use the PeriodicWave stuff to generate the fixed waveforms instead, but this is quick. This patch tries to implement dynamic frequency and detune parameter changes, but doesn't correctly reset the phase on 'start'. Ehsan, on today's m-c the osc engine doesn't seem to see the 'stop' event from my test page at http://people.mozilla.org/~rgiles/2013/osc01.html and keeps making noise forever. I'm pretty sure that worked properly two weeks ago when I first tried this.

Comment on attachment 765656 [details] [diff] [review] compute a sine directly Review of attachment 765656 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Ralph Giles (:rillian) from comment #13) > Ehsan, on today's m-c the osc engine doesn't seem to see the 'stop' event > from my test page at http://people.mozilla.org/~rgiles/2013/osc01.html and > keeps making noise forever. I'm pretty sure that worked properly two weeks > ago when I first tried this. See my comment below about handling mStop. I'm really surprised if the audio playback would ever stop without you explicitly doing that... ::: content/media/webaudio/OscillatorNode.cpp @@ +116,5 @@ > + AllocateAudioBlock(1, aOutput); > + float* output = static_cast<float*>(const_cast<void*>(aOutput->mChannelData[0])); > + double rate = 2.*M_PI / aStream->SampleRate(); > + double phase = mPhase; > + TrackTicks ticks = aStream->GetCurrentPosition(); So, you need to take mStart/mStop into account. mStart tells you when you should start producing the waveform. 0 means immediately, but if mStart>0, you should only start producing the waveform once GetCurrentPosition()+i==mStart. mStop, if equal to TRACK_TICKS_MAX, means keep generating the waveform. 0 means stop immediately. Otherwise, you should stop generating the waveform once GetCurrentPosition()+i==mStop. Also, when you get to mStop, you should set *aFinished to true. AudioBufferSourceNodeEngine does all of these things, so you can check it out for ideas. Note that before and after the start/stop position you should produce silence. If the entire WEBAUDIO_BLOCK_SIZE will be silent (for example, before we get to mStart) you can do aOutput->SetNull(WEBAUDIO_BLOCK_SIZE) which avoids allocating a buffer. Otherwise you should just fill in 0's.

Thanks for the feedback! I guess I was imagining this working earlier.

Patch from Bug 885583. Carrying over r=ehsan from there.

Updated patch in response to feedback. - Move Sine generation to a local function - by-block mStop handling. Still need by-sample and mStart handling.

Comment on attachment 766172 [details] [diff] [review] Part 4: Generated sine Review of attachment 766172 [details] [diff] [review]: ----------------------------------------------------------------- Great, this is moving in the right direction!

Great. At JSConf.br, Ricardo Tomasi (@ricardobeat) showed nice examples of oscillators on Chrome, and Mozilla fans asked about this bug. Just giving encouragement -- let me know if I can help anything. /be

Including the changes for bug 886165 which affect OscillatorNode.

Rebased on top of mozilla-central.

Comment on attachment 766170 [details] [diff] [review] Part 3: use PushPrefEnv in the mochitest This patch was obsoleted by the rebase after bug 885583 landed.

My plan now is to implement the fixed waveform types directly as in Part 3 above, and land that. Doing so will enable testing outside of the setWaveTable method. Support for that method can land separately on bug 865256. Then we'll probably want to re-implement the fixed types in terms of the PeriodWave code in a third bug. I get some crackling sounds with my demo page, especially when starting a new node. Running with NSPR_LOG_MODULES=AudioStream:5 shows the noise is correlated with underruns: -1475025152[7f8e58009f20]: AudioStream 7f8e54004ad0 lost 3137 frames -1475025152[7f8e58009f20]: AudioStream 7f8e54004ad0 lost 420 frames -1475025152[7f8e58009f20]: AudioStream 7f8e54004ad0 lost 29 frames -1475025152[7f8e58009f20]: AudioStream 7f8e54004ad0 lost 72 frames However, I see similar underrun reports when the AudioContext is first initialized, and every few seconds a small (5-50 frames) underrun is reported even when no audio is playing. This could be some combination of clock drift from the partial MSG implementation and a problem with graph change scheduling. Roc suggested using OfflineAudioContext to verify the OscillatorNode was generating clean data in absence of real-time constraints, and this seems to be the case. The plot at http://people.mozilla.org/~rgiles/2013/osc02.html looks clean.

Great! FWIW bug 848953 covers fixing the underrun issues, so don't let that hold you back!

Note that we probably need to stop the oscillator node from AudioContext::Shutdown() similar to AudioBufferSourceNode and ScriptProcessorNode in order to drop the playing ref.

Updated patch implementing the other fixed oscillator types. Still needs to handle start/stop inside a block, resetting phase on start, and comment #26. Tested with http://people.mozilla.org/~rgiles/2013/osc01.html

Sorry, I meant testing with http://people.mozilla.org/~rgiles/2013/osc02.html

Ehsan, is this what you meant? Do we need to be setting mStartCalled back to false at the end of ::Stop()?

(In reply to Ralph Giles (:rillian) from comment #29) > Created attachment 771514 [details] [diff] [review] > Part 4: Stop OscillatorNodes on context shutdown > > Ehsan, is this what you meant? Basically yes. > Do we need to be setting mStartCalled back to false at the end of ::Stop()? No, since the spec only allows each note to be started and stopped once.

Comment on attachment 771514 [details] [diff] [review] Part 4: Stop OscillatorNodes on context shutdown Review of attachment 771514 [details] [diff] [review]: ----------------------------------------------------------------- You also need something like AudioContext::UnregisterAudioBufferSourceNode, otherwise you'll be accessing a pointer to OscillatorNodes which have been destroyed.

Good point. This better?

Comment on attachment 773708 [details] [diff] [review] Part 4: Stop OscillatorNodes on context shutdown Review of attachment 773708 [details] [diff] [review]: ----------------------------------------------------------------- Yes, that's what I meant! Thanks.

I'm on vacation next week. I'll look at it again when I get back August 5, but if someone else wants to pick it up in the meantime, feel free. The next step is just to start/stop the waveforms at the request sample, instead of at the nearest block boundary. I also still get crackling when playback starts.

I asked Paul to take a look at this while Ralph is away.

Updated patch to start/stop with sample accuracy, instead of at the next block boundary as in the previous patch. I think this is complete enough to land the feature.

I got a missing symbol error in the cycle collector boilerplate after rebasing and moving the MacOS while travelling. Added NS_IMPL_CYCLE_COLLECTION_CLASS(OscillatorNode) at the start of OscillatorNode.cpp, otherwise the patch is the same.

Just a cosmetic fix, NPOTB.

Oops, previous version has bitrotted. Here's a rebase.

Corresponding rebase of ehsan's patch for Part 2.

Test build: https://tbpl.mozilla.org/?tree=Try&rev=f1168284534f With this set of patches I still get some glitching in the audio output with http://people.mozilla.org/~rgiles/2013/osc01.html. However, the waveform data looks clean if I render to an OfflineAudioContext as in http://people.mozilla.org/~rgiles/2013/osc02.html I propose we land this as-in and file follow-up bugs for the glitching.

Do you mean something that is not tracked by bug 848954?

Comment on attachment 790441 [details] [diff] [review] Part 3: direct generation of fixed types Review of attachment 790441 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/OscillatorNode.cpp @@ +113,5 @@ > + "WEBAUDIO_BLOCK_SIZE overflows interator bounds."); > + start = 0; > + if (ticks < mStart) { > + start = mStart - ticks; > + for (uint i = 0; i < start; ++i) { Nit: uint32_t @@ +120,5 @@ > + } > + end = WEBAUDIO_BLOCK_SIZE; > + if (ticks + end > mStop) { > + end = mStop - ticks; > + for (uint i = end; i < WEBAUDIO_BLOCK_SIZE; ++i) { Ditto. @@ +137,5 @@ > + FillBounds(output, ticks, start, end); > + > + double rate = 2.*M_PI / mSource->SampleRate(); > + double phase = mPhase; > + for (int i = start; i < end; ++i) { This should warn because of the signed/unsigned compare, please use uint32_t here and elsewhere. @@ +234,5 @@ > + if (ticks + WEBAUDIO_BLOCK_SIZE < mStart) { > + // We're not playing yet. > + ComputeSilence(aOutput); > + return; > + } else if (ticks >= mStop) { Nit: no else after return.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #42) > Do you mean something that is not tracked by bug 848954? If that's the likely cause, great. I remembered an attempted fix had landed, and wanted to be clear I still get glitches.

Rebased on top of Bug 905409. Carrying forward r=roc.

rebase, probably unnecessary. Carrying forward r=roc.

Updated to address review comments: - use uint32_t consistently - remove else after return This fixes the sign-compare warnings. Thanks! Carrying forward r=ehsan

Rebase; carrying forward r=ehsan.

Rebase; fix review attribution in commit message. carrying forward r=ehsan.

(In reply to Ralph Giles (:rillian) from comment #44) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #42) > > Do you mean something that is not tracked by bug 848954? > > If that's the likely cause, great. I remembered an attempted fix had landed, > and wanted to be clear I still get glitches. That bug causes audible glitches for pure sine waveforms, so I bet that is what's happening here, although you can verify that by adding debugging code as suggested in that bug. I don't think that we have so far landed any experimental fixes for that bug, but I could be wrong.

Update Part 1 for bug 859022. Carrying forward r=roc. Fixes "'this': used in base member initializer list" warning on MSVC. Thanks to padenot for the fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a7b48cfa4f2 https://hg.mozilla.org/integration/mozilla-inbound/rev/09c06b817d43 https://hg.mozilla.org/integration/mozilla-inbound/rev/8978b161d870 https://hg.mozilla.org/integration/mozilla-inbound/rev/f9bad5e3e7ba https://hg.mozilla.org/integration/mozilla-inbound/rev/94a23a49ca26

https://hg.mozilla.org/mozilla-central/rev/8a7b48cfa4f2 https://hg.mozilla.org/mozilla-central/rev/09c06b817d43 https://hg.mozilla.org/mozilla-central/rev/8978b161d870 https://hg.mozilla.org/mozilla-central/rev/f9bad5e3e7ba https://hg.mozilla.org/mozilla-central/rev/94a23a49ca26

https://hg.mozilla.org/releases/mozilla-aurora/rev/19845caac775 https://hg.mozilla.org/releases/mozilla-aurora/rev/5c95ce1874b3 https://hg.mozilla.org/releases/mozilla-aurora/rev/cfeac11731ae https://hg.mozilla.org/releases/mozilla-aurora/rev/78a602878032 https://hg.mozilla.org/releases/mozilla-aurora/rev/a152af6e080e

While testing for the pre-beta sign-off of this feature, I tried to verify this bug, so here are my questions and results: 1) the link from comment 7 sounds the same, both on Chrome and latest Aurora (build ID: 20130902004002), on Ubuntu 12.10 32bit and Mac OSX 10.9 in 32bit mode 2) the link from comment 28 doesn't look quite the same with Aurora and Chrome (please see the attached screenshot: above is Chrome, and below is Aurora) Does anyone have any thoughts/suggestions? Thanks!

Setting need-info to Ralph. Manuela, please make sure you set a requestee for all need-info requests in the future otherwise they will go unanswered.

Manuela, the difference in screenshots is tracked as bug 908669, so that shouldn't concern us here. Verifying you get output as you did is sufficient for this bug.

Marking this as verified based on comment 55 and comment 57. Thank you!