Tonejs / Tone.js

A Web Audio framework for making interactive music in the browser.
https://tonejs.github.io
MIT License
13.52k stars 983 forks source link

fixes for Context options handling & micro timing bugs #987

Closed marcelblum closed 2 years ago

marcelblum commented 2 years ago

This is mostly to fix the Context constructor's mishandling of updateInterval but also along the way fixes some other minor Context and Ticker constructor bugs, removes some unreachable/overridden/legacy code in _setLatencyHint(), and ensures that updateInterval is never unintentionally set higher than lookAhead which can lead to subtle timing issues. Also added a check for the expected updateInterval value in the Context constructor tests.

At first glance this probably looks like a lot of major internal plumbing refactors but it's not really! πŸ˜‰ There are no breaking changes, all defaults remain as before; this is really just to address some bugs with edge cases when the user wants to roll their own Context using non-default values.

List of issues this addresses:

1) bugfix: updateInterval option in Context constructor is ignored:

console.log(new Tone.Context({updateInterval: .02}).updateInterval);
// 0.05

2) bugfix: updateInterval gets set to an unintended value in certain cases where non-default latencyHint or lookAhead are used, often greater than lookAhead, leading to potential subtle timing issues:

console.log(new Tone.Context({lookAhead: .1, latencyHint:"playback"}).updateInterval);
// 0.25

3) Changing lookAhead after context creation can lead to similar subtle timing issues because updateInterval doesn't automatically update along with it. Fixed by creating a lookAhead setter which sets the updateInterval to ( lookAhead / 2 ) in the spirit of the legacy _setLatencyHint(). When lookAhead is 0, updateInterval defaults to .01 which, admittedly, is just a value that I arrived at for best x-browser operation after some subjective testing on a few different devices, and can still be forced to a different value when explicitly set. See https://codepen.io/keymapper/pen/KKXKvPV in Chrome for a quick demo. It uses a short loud bass drum which results in an audible pop if the sound is clipped by the smallest amount, and uses setInterval rather than Tone scheduling in order to simulate a realtime source play trigger. When lookAhead = 0 and updateInterval is left at the default .05, you'll hear a pop every 5-10 hits from the stop time getting miscalculated by a microsecond. When updateInterval is adjusted to .01 the miscalculation seems mostly gone. (Note: due to timing/latency quirks this doesn't fix the micro-timing issue in all cases on all browsers though! I will open a separate issue for further discussion on this.).

4) bugfix: Minimum allowable updateInterval is miscalculated in certain cases because it is based on an assumed fixed 44100 sampleRate. Most non-Apple platforms seem to default to 48000, and the user can roll their own context at a wide range different rates depending on browser. Fixed by passing the context's sampleRate in with the Ticker constructor and calculating a lowest practical _minimumUpdateInterval. Also added the ability to pass an updateInterval of 0 to make it automatically set to the _minimumUpdateInterval.

5) bugfix: Context.latencyHint returns incorrect value in some cases when using a native AudioContext. This is mostly just an informational error, but this incorrect value was then being used by _setLatencyHint() to derive updateInterval. Since it's impossible for Tone to infer the latencyHint from a user-instantiated native context, I've changed Context.latencyHint to return an empty string when an own-rolled native context is passed in, and also set updateInterval to be calculated from lookAhead by default.

console.log(new Tone.Context({context: new AudioContext({latencyHint : "balanced"})}).latencyHint);
// "interactive"

6) Remove vestigial legacy handling of latencyHint in _setLatencyHint() much of which was unreachable code or getting overridden depending on constructor options. I think _setLatencyHint() was a holdover from older Tone versions where latencyHint in Tone had a different handling than the browser native AudioContext.latencyHint, perhaps from back when latencyHint was ignored by some browsers.

tambien commented 2 years ago

Thanks for these updates!

tambien commented 2 years ago

Thanks for this! πŸ™πŸ» Sorry took me a little while to pull it in.