Closed Bug 1140448 Opened 6 years ago Closed 5 years ago

Investigate rewriting AudioEventTimeline for speed


(Core :: Web Audio, defect)

32 Branch
Not set



Tracking Status
firefox40 --- fixed


(Reporter: padenot, Assigned: baku)


(Depends on 1 open bug)


(Keywords: perf)


(1 file, 1 obsolete file)

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:
Assignee: nobody → amarchesini
Attached patch patch (obsolete) — Splinter Review
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)
Comment on attachment 8593475 [details] [diff] [review]

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)
Attached patch a.patchSplinter Review
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)
(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).
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.
Comment on attachment 8594873 [details] [diff] [review]

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.

Attachment #8594873 - Flags: review?(padenot) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 11 was intended for Bug 1140048.
Resolution: FIXED → ---
Depends on: 1213313
Depends on: 1257718
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.