943138 - ADSR envelopes are bad/ineffective

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect, P1)

Description

[Karl Tomlinson (:karlt)]

Attachment 8335570 [details] demonstrates a change in dialer behavior.
Bug 937505 and bug 919215 cover the delay, but this bug is about the change in
ADSR envelopes.

I'll attach plots of the tones before and after the change.

The changes are believed to be due to https://github.com/mozilla-b2g/gaia/commit/add12c96c3e9281baa7f483f7ba751ce63df5749

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: tone for 2 - before change

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: tone for 2 - after change

This sounds like it starts and ends suddenly.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: tone for 5 - before change

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: tone for 5 - after change

Comment 5

[Karl Tomlinson (:karlt)]

I expect the lack of an attack curve in the web audio gain node implementation is due to the issues described at http://lists.w3.org/Archives/Public/public-audio/2013OctDec/0286.html

In short, the Web Audio API is not designed to be used in this way.
Unfortunately it doesn't provide an elegant way to do this, and envelopes need to be applied to each sample in JS.

One option might be to use AudioBufferSourceNodes scheduled to play end on end.
These will only merge seemlessly when the buffer sampleRate is the same as that of the AudioContext.

The MediaSource interface looks like it is better designed for the purposes here.

Comment 6

[Karl Tomlinson (:karlt)]

There are some unexplained observations here:


Before add12c96 the code specified

kKeyToneFrames = 1200
_sampleRate = 8000

implying a duration of 150 ms (consistent with comment), but the recorded
tones are 80 - 100 ms

(With the web audio implementation the short tones of 60 to 85 ms instead of
250 could be explained by missing the start of the tone, but the observations
before change show gradual attack and release portions, so apparently the ends
of the tone are not truncated.)


The web audio implementation shortened the release phase from 50 to 25 ms, but
in the recordings this is only 10 ms, and there is a sudden drop in amplitude at the beginning of this phase.

Comment 7

[Karl Tomlinson (:karlt)]

The Web Audio API does provide what we want here if we create a new
AudioContext for each tone.  If the context is created and the oscillators are
started before returning to the event loop then they can start at time zero,
and so phases can be scheduled accurately.  However, in Gecko (and Chromium
too, I think), each (online) AudioContext has its own system audio stream and
these may take some time to create, which was what introduced the latency in
bug 919215.

(In reply to Karl Tomlinson (:karlt) from comment #5)
> The MediaSource interface looks like it is better designed for the purposes
> here.

I spoke with Matthew and he pointed out that MediaSource is intended for use
with encoded media and so does not seem right here.

> One option might be to use AudioBufferSourceNodes scheduled to play end on
> end.
> These will only merge seemlessly when the buffer sampleRate is the same as
> that of the AudioContext.

And this is only possible if the buffers are scheduled far enough in
the future that they will not fall off schedule.  It doesn't work for a
do-this-as-soon-as-possible approach.

Short presses could be produced by a single AudioBuffer containing ADSR-shaped
samples at a sample rate that can represent the frequencies.  These could be
used with AudioBufferSourceNode on which start(0) is called.  The audio will
start at the beginning of the next processing block, which is as soon as
possible, and will have the appropriate intervals between effects.
OfflineAudioContext provides a callback API to render this buffer, but it may
be better to generate it in JS.

However long presses couldn't be handled this way because we don't know how
long the note will be - it stops on finger release.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: asr-2.html

The best option I think is to use a BufferSource with a ramp for the transient
effects on the gain AudioParam.

This leaks due to bug 897796, but that can be worked around.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8338351 [details]
asr-2.html

>    // To ramp to full volume (1), set the destination volume, then subtract a
>    // ramp down to zero.
>    gain.gain.value = 1;
>    rampSource = context.createBufferSource();
>    rampSource.buffer = rampUp;
>    rampSource.start();
>    rampSource.connect(gain.gain);

Actually, this is not quite right.

The spec says "The implementation must make gain changes to the audio stream smoothly, without introducing noticeable clicks or glitches", which in practice means that changes from setting AudioParam.value have a transition.  If the change to gain.value does not cause a step function change,
then the initial summed value of the AudioParam will not be zero.

I think setValueAtTime() can be used with currentTime or any value in the past (including 0) to ensure the change is a step function, but this should be tested.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: unagi-web-audio-3.ogg

This is recorded on a Unagi, tapping 1 2 3 4 5 6 7 8 9 in order.  The taps can be heard before the tones.  The tones start and stop abruptly.

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: unagi-adsr-3.ogg

This is with the changes at https://github.com/karlt/gaia/tree/943138, which include the fix for bug 942751.
I'll submit a pull request after some tidying up.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: pull request

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> There are some unexplained observations here:

> The web audio implementation shortened the release phase from 50 to 25 ms,
> but
> in the recordings this is only 10 ms, and there is a sudden drop in
> amplitude at the beginning of this phase.

This is probably due to stopping the tone on touchEnd, addressed in bug 942751 comment 1.

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8345567 [details] [review]
pull request

Thanks for the clear comments it helped a lot, and it's working well!

r=me

(but it probably needs rebasing)

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to Etienne Segonzac (:etienne) from comment #14)

Thanks.

> (but it probably needs rebasing)

These changes were on top of 6725558, which was merged in https://github.com/mozilla-b2g/gaia/commit/577f46a9a8f3f16540c007fa420bd5630c5f2ae3
so I assume this can be merged without conflicts.

I pulled from mozilla-b2g and "git merge 943138" was successful:
Merge made by the 'recursive' strategy.
 apps/communications/dialer/js/tone_player.js | 146 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 104 insertions(+), 42 deletions(-)

There are some seemingly unrelated test failures at
https://travis-ci.org/mozilla-b2g/gaia/builds/15256047

I'm learning my way around here, so please let me know if there is something more I need to do.

Comment 16

[Joe Cheng [:jcheng] (please needinfo)]

triage: blocking a 1.3+ blocker. 1.3+

Comment 17

[Etienne Segonzac (:etienne)]

(In reply to Karl Tomlinson (:karlt) from comment #15)
> I'm learning my way around here, so please let me know if there is something
> more I need to do.

No problem.
We usually do 1 commit by bug, can you rebase and squash those into one.
(this will also kick off a new travis build)

Comment 18

[Maire Reavy [:mreavy]]

P1 since this is 1.3+  (needed to improve the Dialer).

Comment 19

[Karl Tomlinson (:karlt)]

Rebased and squashed, and Travis threw the winning numbers.

Comment 20

[Etienne Segonzac (:etienne)]

Perfect, thanks!

https://github.com/mozilla-b2g/gaia/commit/3330e45ecd79a12241663db8bd1799cdfccd1451

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 3330e45ecd79a12241663db8bd1799cdfccd1451 to:
v1.3: 16e9309a10388efe7c78fa8bd2c9144b089f3740