Closed Bug 865253 (oscillatornode) Opened 7 years ago Closed 7 years ago

Implement OscillatorNode

Categories

(Core :: Web Audio, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox25 --- verified
firefox26 --- fixed

People

(Reporter: ehsan, Assigned: rillian)

References

Details

(Whiteboard: [blocking-webaudio+])

Attachments

(6 files, 24 obsolete files)

4.55 KB, patch
rillian
: review+
Details | Diff | Splinter Review
6.04 KB, patch
rillian
: review+
Details | Diff | Splinter Review
4.87 KB, patch
rillian
: review+
Details | Diff | Splinter Review
869 bytes, patch
rillian
: review+
Details | Diff | Splinter Review
20.06 KB, patch
rillian
: review+
Details | Diff | Splinter Review
1.85 MB, image/png
Details
No description provided.
Depends on: 865256
I'll prepare a patch for the DOM bindings part, ETA some time today.
Assignee: nobody → giles
Attached patch Part 1: DOM bindings (obsolete) — Splinter Review
Attachment #754567 - Flags: review?(roc)
Attached patch Part 1: DOM bindings (obsolete) — Splinter Review
Rebased.  I won't land this for now as this patch on its own will add a node which doesn't do anything...
Attachment #754567 - Attachment is obsolete: true
Attached patch Part 1: DOM bindings (obsolete) — Splinter Review
Rebased on top of bug 876024.
Attachment #754757 - Attachment is obsolete: true
(This really belongs in bug 879014, but the part 1 patch here has not landed yet.)
Attachment #757670 - Flags: review?(roc)
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
I wrote a simplest-possible test page. http://people.mozilla.org/~rgiles/2013/osc01.html
Attached patch Part 1: DOM bindings (obsolete) — Splinter Review
Rebased on top of inbound.
Attachment #756075 - Attachment is obsolete: true
Attachment #757670 - Attachment is obsolete: true
Attached patch Part 1: DOM bindings (obsolete) — Splinter Review
Rebased on top of bug 884632 which I just landed on inbound.
Attached patch Part 1: DOM bindings (obsolete) — Splinter Review
Rebased on top of https://hg.mozilla.org/integration/mozilla-inbound/rev/af744b5304d8.
Attachment #760701 - Attachment is obsolete: true
Attachment #764705 - Attachment is obsolete: true
Whiteboard: [blocking-webaudio+]
Rebase part two on m-c + part one. The current patch didn't 'git am' cleanly for me.
Attachment #760704 - Attachment is obsolete: true
Attached patch compute a sine directly (obsolete) — Splinter Review
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.
Attachment #765656 - Flags: feedback?(ehsan)
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.
Attachment #765656 - Flags: feedback?(ehsan) → feedback-
Thanks for the feedback! I guess I was imagining this working earlier.
Patch from Bug 885583. Carrying over r=ehsan from there.
Attachment #766170 - Flags: review+
Attached patch Part 4: Generated sine (obsolete) — Splinter Review
Updated patch in response to feedback.

- Move Sine generation to a local function
- by-block mStop handling. Still need by-sample and mStart handling.
Attachment #765656 - Attachment is obsolete: true
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
Alias: oscillatornode
Including the changes for bug 886165 which affect OscillatorNode.
Attachment #765561 - Attachment is obsolete: true
Attached patch Part 1: DOM bindings (obsolete) — Splinter Review
Rebased on top of mozilla-central.
Attachment #765050 - Attachment is obsolete: true
Attachment #766491 - Attachment is obsolete: true
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.
Attachment #766170 - Attachment is obsolete: true
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
Attachment #766172 - Attachment is obsolete: true
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()?
Attachment #771514 - Flags: review?(ehsan)
(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.
Attachment #771514 - Flags: review?(ehsan) → review-
Good point. This better?
Attachment #771514 - Attachment is obsolete: true
Attachment #773708 - Flags: review?(ehsan)
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.
Attachment #773708 - Flags: review?(ehsan) → review+
OS: Mac OS X → All
Hardware: x86 → All
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.
Assignee: giles → paul
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.
Attachment #771473 - Attachment is obsolete: true
Attachment #790441 - Flags: review?(ehsan)
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.
Attachment #773708 - Attachment is obsolete: true
Attachment #790445 - Flags: review?(ehsan)
Attachment #790445 - Attachment description: 0004-Bug-865253-Part-4-Stop-OscillatorNodes-on-context-sh.patch → Part 4: Stop OscillatorNodes on context shutdown
Attached patch Part 5: Shorten a comment (obsolete) — Splinter Review
Just a cosmetic fix, NPOTB.
Attachment #790448 - Flags: review?(ehsan)
Attached patch Part 1: DOM bindings (obsolete) — Splinter Review
Oops, previous version has bitrotted. Here's a rebase.
Attachment #766497 - Attachment is obsolete: true
Corresponding rebase of ehsan's patch for Part 2.
Attachment #766498 - Attachment is obsolete: true
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.
Attachment #790441 - Flags: review?(ehsan) → review+
Attachment #790445 - Flags: review?(ehsan) → review+
Attachment #790448 - Flags: review?(ehsan) → review+
(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.
Attached patch Part 1: DOM bindings (obsolete) — Splinter Review
Rebased on top of Bug 905409. Carrying forward r=roc.
Attachment #790451 - Attachment is obsolete: true
Attachment #792324 - Flags: review+
rebase, probably unnecessary. Carrying forward r=roc.
Attachment #790454 - Attachment is obsolete: true
Attachment #792327 - Flags: review+
Updated to address review comments:

- use uint32_t consistently
- remove else after return

This fixes the sign-compare warnings. Thanks!

Carrying forward r=ehsan
Attachment #790441 - Attachment is obsolete: true
Attachment #792331 - Flags: review+
Rebase; carrying forward r=ehsan.
Attachment #790445 - Attachment is obsolete: true
Attachment #792333 - Flags: review+
Rebase; fix review attribution in commit message. carrying forward r=ehsan.
Assignee: paul → giles
Attachment #790448 - Attachment is obsolete: true
Attachment #792334 - Flags: review+
(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.
Attachment #792324 - Attachment is obsolete: true
Attachment #792398 - Flags: review+
Depends on: 907318
Whiteboard: [blocking-webaudio+] → [blocking-webaudio+][checkin-needed-aurora]
Attached image 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!
Flags: needinfo?
QA Contact: manuela.muntean
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.
Flags: needinfo?
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!
You need to log in before you can comment on or make changes to this bug.