Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve WebAudio messaging model to match Firefox #21659

Open
Manishearth opened this issue Sep 10, 2018 · 3 comments
Open

Improve WebAudio messaging model to match Firefox #21659

Manishearth opened this issue Sep 10, 2018 · 3 comments

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Sep 10, 2018

The way it's currently set up, WebAudio JS API calls may end up occurring on different render quanta even if they occur in the same task. Plus, currentTime is a cross-thread sync call which is no good.

I discussed this with @padenot and the Firefox model seems like a good way to go forward here.

What Firefox does is:

  • Batches up all messages to the render thread till steady state
  • currentTime is kept up to date on the main/DOM thread via an event sent to the main thread every render quantum
  • Node creation messages are not batched, and instead are executed immediately (waiting for the NodeId to be returned).
  • To be able to throw errors we must maintain AudioParam timelines on both sides. Oh well. We don't need to do this for the entire graph, fortunately.

Worth noting: Chrome handles currentTime with an atomic (so it may update underneath you), but we can't do that anyway because of a possible process boundary.

cc @ferjm

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Sep 19, 2018

I think for the reverse -- the render thread messaging the DOM -- we should also use a single pipeline. This goes for:

  • currentTime updates
  • offlineaudiocontext completion events
  • AnalyserNode update events

This doesn't affect synchronous DOM calls that need to wait on WebAudio.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Sep 19, 2018

The AudioParam fixes will also be needed to implement param-dependent things like BiquadFilterNode::getFrequencyResponse().

Note that currently AudioParams don't like being only occasionally updated -- if they end up skipping events entirely the start values can be wrong. This will also need to be fixed.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Sep 20, 2018

There also may be timing issues when you start audiocontexts.

bors-servo added a commit that referenced this issue Sep 20, 2018
Implement BiquadFilterNode

A bunch of tests still fail but some of it may be a timing issue, looking at it the tests are *at least* affected by #21659 (changing how they work to avoid problems from that does not make them pass but does change the exact value of the error), so I feel like I should fix that first before investigating these.

r? @ferjm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21750)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.