865253 - (oscillatornode) Implement OscillatorNode

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Comment 1

[(no longer active)]

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

Comment 2

[(no longer active)]

Attachment: Part 1: DOM bindings

Comment 3

[(no longer active)]

Attachment: Part 1: DOM bindings

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

Comment 4

[(no longer active)]

Attachment: Part 1: DOM bindings

Rebased on top of bug 876024.

Comment 5

[(no longer active)]

Attachment: Part 2: Implement the alternate enum values for OscillatorNode

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

Comment 6

[(no longer active)]

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

Comment 7

[Ralph Giles (:rillian)]

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

Comment 8

[(no longer active)]

Attachment: Part 1: DOM bindings

Rebased on top of inbound.

Comment 9

[(no longer active)]

Attachment: Part 2: Implement the alternate enum values for OscillatorNode

Comment 10

[(no longer active)]

Attachment: Part 1: DOM bindings

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

Comment 11

[(no longer active)]

Attachment: Part 1: DOM bindings

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

Comment 12

[Ralph Giles (:rillian)]

Attachment: Part 2: Implement the alternate enum values for OscillatorNode

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

Comment 13

[Ralph Giles (:rillian)]

Attachment: compute a sine directly

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 14

[(no longer active)]

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.

Comment 15

[Ralph Giles (:rillian)]

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

Comment 16

[Ralph Giles (:rillian)]

Attachment: Part 3: use PushPrefEnv in the mochitest

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

Comment 17

[Ralph Giles (:rillian)]

Attachment: Part 4: Generated sine

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 18

[(no longer active)]

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!

Comment 19

[Brendan Eich [:brendan]]

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

Comment 20

[(no longer active)]

Attachment: Part 2: Implement the alternate enum values for OscillatorNode

Including the changes for bug 886165 which affect OscillatorNode.

Comment 21

[(no longer active)]

Attachment: Part 1: DOM bindings

Rebased on top of mozilla-central.

Comment 22

[(no longer active)]

Attachment: Part 2: Implement the alternate enum values for OscillatorNode

Comment 23

[Ralph Giles (:rillian)]

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.

Comment 24

[Ralph Giles (:rillian)]

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.

Comment 25

[(no longer active)]

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

Comment 26

[(no longer active)]

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.

Comment 27

[Ralph Giles (:rillian)]

Attachment: Part 3: Direct generation of fixed types

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

Comment 28

[Ralph Giles (:rillian)]

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

Comment 29

[Ralph Giles (:rillian)]

Attachment: Part 4: Stop OscillatorNodes on context shutdown

Ehsan, is this what you meant?

Do we need to be setting mStartCalled back to false at the end of ::Stop()?

Comment 30

[(no longer active)]

(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 31

[(no longer active)]

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.

Comment 32

[Ralph Giles (:rillian)]

Attachment: Part 4: Stop OscillatorNodes on context shutdown

Good point. This better?

Comment 33

[(no longer active)]

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.

Comment 34

[Ralph Giles (:rillian)]

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.

Comment 35

[(no longer active)]

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

Comment 36

[Ralph Giles (:rillian)]

Attachment: Part 3: direct generation of fixed types

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.

Comment 37

[Ralph Giles (:rillian)]

Attachment: Part 4: Stop OscillatorNodes on context shutdown

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.

Comment 38

[Ralph Giles (:rillian)]

Attachment: Part 5: Shorten a comment

Just a cosmetic fix, NPOTB.

Comment 39

[Ralph Giles (:rillian)]

Attachment: Part 1: DOM bindings

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

Comment 40

[Ralph Giles (:rillian)]

Attachment: Part 2: Implement the alternate enum values for OscillatorNode

Corresponding rebase of ehsan's patch for Part 2.

Comment 41

[Ralph Giles (:rillian)]

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.

Comment 42

[(no longer active)]

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

Comment 43

[(no longer active)]

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.

Comment 44

[Ralph Giles (:rillian)]

(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.

Comment 45

[Ralph Giles (:rillian)]

Attachment: Part 1: DOM bindings

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

Comment 46

[Ralph Giles (:rillian)]

Attachment: Part 2: Implement the alternate enum values

rebase, probably unnecessary. Carrying forward r=roc.

Comment 47

[Ralph Giles (:rillian)]

Attachment: Part 3: Direct generation of fixed types

Updated to address review comments:

- use uint32_t consistently
- remove else after return

This fixes the sign-compare warnings. Thanks!

Carrying forward r=ehsan

Comment 48

[Ralph Giles (:rillian)]

Attachment: Part 4: Stop OscillatorNodes on context shutdown

Rebase; carrying forward r=ehsan.

Comment 49

[Ralph Giles (:rillian)]

Attachment: Part 5: Shorten a comment

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

Comment 50

[(no longer active)]

(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.

Comment 51

[Ralph Giles (:rillian)]

Attachment: Part 1: DOM bindings

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.

Comment 52

[Ralph Giles (:rillian)]

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

Comment 53

[Ed Morley [:emorley]]

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

Comment 54

[Ralph Giles (:rillian)]

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

Comment 55

[Manuela Muntean [Away]]

Attachment: ScreenShot.png

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!

Comment 56

[u279076]

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.

Comment 57

[Ralph Giles (:rillian)]

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.

Comment 58

[Manuela Muntean [Away]]

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