Bug 865253 (oscillatornode)

Implement OscillatorNode

RESOLVED FIXED in Firefox 25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: rillian)

Tracking

Trunk
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox25 verified, firefox26 fixed)

Details

(Whiteboard: [blocking-webaudio+])

Attachments

(6 attachments, 24 obsolete attachments)

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
Reporter

Description

6 years ago
No description provided.
Reporter

Updated

6 years ago
Depends on: 865256
Reporter

Comment 1

6 years ago
I'll prepare a patch for the DOM bindings part, ETA some time today.
Assignee: nobody → giles
Reporter

Comment 2

6 years ago
Posted patch Part 1: DOM bindings (obsolete) — Splinter Review
Attachment #754567 - Flags: review?(roc)
Reporter

Comment 3

6 years ago
Posted 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
Reporter

Comment 4

6 years ago
Posted patch Part 1: DOM bindings (obsolete) — Splinter Review
Rebased on top of bug 876024.
Attachment #754757 - Attachment is obsolete: true
Reporter

Comment 5

6 years ago
(This really belongs in bug 879014, but the part 1 patch here has not landed yet.)
Attachment #757670 - Flags: review?(roc)
Reporter

Comment 6

6 years ago
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
Reporter

Comment 8

6 years ago
Posted patch Part 1: DOM bindings (obsolete) — Splinter Review
Rebased on top of inbound.
Attachment #756075 - Attachment is obsolete: true
Reporter

Comment 9

6 years ago
Attachment #757670 - Attachment is obsolete: true
Reporter

Comment 10

6 years ago
Posted patch Part 1: DOM bindings (obsolete) — Splinter Review
Rebased on top of bug 884632 which I just landed on inbound.
Reporter

Comment 11

6 years ago
Posted 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
Posted 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)
Reporter

Comment 14

6 years ago
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+
Posted 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
Reporter

Comment 18

6 years ago
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
Reporter

Comment 20

6 years ago
Including the changes for bug 886165 which affect OscillatorNode.
Attachment #765561 - Attachment is obsolete: true
Reporter

Comment 21

6 years ago
Posted patch Part 1: DOM bindings (obsolete) — Splinter Review
Rebased on top of mozilla-central.
Attachment #765050 - Attachment is obsolete: true
Reporter

Comment 22

6 years ago
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.
Reporter

Comment 25

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

Comment 26

6 years ago
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
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)
Reporter

Comment 30

6 years ago
(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.
Reporter

Comment 31

6 years ago
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)
Reporter

Comment 33

6 years ago
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.
Reporter

Comment 35

6 years ago
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
Posted patch Part 5: Shorten a comment (obsolete) — Splinter Review
Just a cosmetic fix, NPOTB.
Attachment #790448 - Flags: review?(ehsan)
Posted 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.
Reporter

Comment 42

6 years ago
Do you mean something that is not tracked by bug 848954?
Reporter

Comment 43

6 years ago
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+
Reporter

Updated

6 years ago
Attachment #790445 - Flags: review?(ehsan) → review+
Reporter

Updated

6 years ago
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.
Posted 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+
Reporter

Comment 50

6 years ago
(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
Reporter

Updated

6 years ago
Whiteboard: [blocking-webaudio+] → [blocking-webaudio+][checkin-needed-aurora]
Posted 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!
Depends on: 918212
You need to log in before you can comment on or make changes to this bug.