Investigate rewriting AudioEventTimeline for speed

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: padenot, Assigned: amarchesini)

Tracking

(Depends on: 1 bug, {perf})

32 Branch
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
AudioEventTimeline is the implementation of the AudioParam automation and such.

The current implementation could be improved:
- We should be computing 128 values at a time (one block), for a-rate params
- There is no pruning of past events, so we keep running the algorithm to find the right event for every tick, from the beginning. Ideally, we should clean up the array when returning from computing a block
- We should determine, in the 128 ticks block, the boundaries for which to compute which event, and then compute the actual values in one pass after
- Then if really needed, we can speed it up with SIMD, since it's all linearly independent functions

Bonus points if we can make sure this code is independent enough of Gecko so that we can test it easily (as it is now), and nicely emscriptenable so that web app can do content-side (as opposed to audio thread-side) computations, as they often want to do: https://github.com/jsantell/web-audio-automation-timeline
Assignee: nobody → amarchesini
Created attachment 8593475 [details] [diff] [review]
patch

This patch introduces:

AudioEventTimeline::GetValuesAtTime(TimeType aTime, float* aBuffer, const size_t aSize)

and it uses everywhere we do a for loop to get the values.
And it passes the tests.
Attachment #8593475 - Flags: review?(padenot)
(Reporter)

Comment 2

3 years ago
Comment on attachment 8593475 [details] [diff] [review]
patch

Review of attachment 8593475 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

I think it's possible to use this for the OscillatorNode, but we can do this in a followup. It's more critical for the GainNode anyways, because it's used for all envelopes.

In any case, I'd like this to implement the event pruning optimization as well, as it's the main issue of this bug.

Also, if possible, I'd be happy to know how much of a win this is, for example by writing a performance test, so we're sure we don't regress.

::: dom/media/webaudio/AudioEventTimeline.h
@@ +279,4 @@
>    {
> +    MOZ_ASSERT(aBuffer);
> +
> +    size_t lastEventId = 0;

We can use this to "prune" the list of events to remove past events so it does not grow and we don't spend a lot of time looking for the right event. Consider the following program:

> window.onload = function() {
>   var ac = new AudioContext();
>   var gain = ac.createGain();
>   gain.gain.setValueAtTime(0, ac.currentTime);
>   gain.connect(ac.destination);
>   function trigger() {
>     var o = ac.createOscillator();
>     o.connect(gain);
>     gain.gain.setTargetAtTime(1.0, ac.currentTime, 0.01);
>     gain.gain.setTargetAtTime(0.0, ac.currentTime, 0.03);
>     o.frequency.value = 220;
>     o.start();
>     o.stop(ac.currentTime + 1.);
>   }
>   setInterval(trigger, 70);
>

You can see the CPU time of the "threaded-ml" (on linux, threads have other names on other platforms) growing (in top/htop for example), and then after some time, it stops playing, because all the time of the audio callback is spent finding the right event instead of actually output sound.

@@ +286,2 @@
>  
> +    for (size_t bufferId = 0; bufferId < aSize; ++bufferId, ++aTime) {

s/bufferId/bufferIndex/g, I first thought it was an identifier of some sort and got confused.
Attachment #8593475 - Flags: review?(padenot)
Created attachment 8594873 [details] [diff] [review]
a.patch

Here a new version: it removes all events when needed.
About the OscillatorNode, any particular reason you want to split it in a follow-up?

I'm going to test the performance improvement tomorrow or tonight.
Attachment #8593475 - Attachment is obsolete: true
Attachment #8594873 - Flags: review?(padenot)
(Reporter)

Comment 4

3 years ago
(In reply to Andrea Marchesini (:baku) from comment #3)
> Created attachment 8594873 [details] [diff] [review]
> a.patch
> 
> Here a new version: it removes all events when needed.
> About the OscillatorNode, any particular reason you want to split it in a
> follow-up?

It's just less trivial, we can do it here as well.
I tested your script from comment 2 and, yes, the removing of events works perfectly. We always go back to 1 event (I added the count of mEvents to stdout).
(Reporter)

Comment 6

3 years ago
Yeah I did the same here it works nicely, there is way less CPU buildup. Next up, we need to find why there is still some CPU buildups.
(Reporter)

Comment 7

3 years ago
Comment on attachment 8594873 [details] [diff] [review]
a.patch

Review of attachment 8594873 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webaudio/AudioEventTimeline.h
@@ +287,2 @@
>  
> +    // Let's remove old events except the last one: we need it to calculate some curve.

s/curve/curves/
Attachment #8594873 - Flags: review?(padenot) → review+
https://hg.mozilla.org/mozilla-central/rev/65050168e0b7
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment hidden (typo)
Comment hidden (typo)
Comment hidden (typo)
Comment 11 was intended for Bug 1140048.
(Reporter)

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (obsolete)
Comment hidden (typo)
Comment hidden (obsolete)
Comment hidden (obsolete)
Depends on: 1213313
Depends on: 1257718
(Reporter)

Updated

2 years ago
Blocks: 1265435
No longer blocks: 1265435
Depends on: 1265435
Depends on: 1184057
You need to log in before you can comment on or make changes to this bug.