update tone.js dependency

[mattetti]

A few issues are related to the fact that this project relies on a old version of tone.js which was since improved and bugs/dependencies fixed. https://github.com/Tonejs/Tone.js/blob/c023181579d55bac110b9d369f79aa5acd275499/CHANGELOG.md#1349

#56 #294 should all be fixed if we can update tone.js @notwaldorf mentioned issues trying to update so it might be trickier than it sounds.

[notwaldorf]

Ok, so I tried this again, and wanted to leave some more details, for posterity. I tried updating to 13.4.9. If you build the demos and go to player.html, this is what I see (only for the default mm.Player. SoundFontPlayers are ok):

• trying to play a note sequence starts somewhere off (like, at the 3rd note, not the first one). Stopping the player at this point doesn't always completely stop it, and sometimes leaves the last note that was playing held forever (and sounds gnarly)
• in the case where that didn't happen and the player stopped correctly, starting the note sequence again will pay it correctly (from the first note, etc).

It almost looks like we're trying to play it before the Tone transport is ready, but I'm not really sure how to debug this (and fewer ideas about how to fix this).

cc @tambien

[tambien]

i think my suggestion would be to try to decouple the players from the Transport. it should be possible to use Tone.Clock in a similar way as you're currently adding things to the Transport, but it would require a bit of reworking. Also for shorter segments, you could just schedule all of the notes upfront instead of playing them through the Tone.Part callback (for sections that are a few minutes long, this sometimes gets too congested to play easily). Then to stop it, you can just disconnect and dispose the player object which would throw away all future events.

i can take a look at this when i have a moment, if no one else gets to it before me.

[notwaldorf]

Also, on that demo page I still see 260+ AudioContext warnings in Chrome, that come from inside of Tone.js (it looks like Tone is very aggressively trying to start the AudioContext without any user interaction, which doesn't fly. It's not a problem, it's just a very spammy console warning, i guess

[notwaldorf]

@tambien I'd loooove if you could take a look instead of me because I'm 100% going to break this.

[mattetti]

I took a quick uneducated look and while I don't see the AudioContext errors going to http://localhost:8080/player.html using the latest tone.js, I do noticed that the base player sequence starts off and there are a lot of clicks. The sequence once played out is fine again but the clicks are still there. I decided to download the MIDI and surely enough the sequence conversion is off now:

Besides the note order being wrong, you can see that the notes are doubled with a very quick note at the end of most note. That's probably what causes the click.

I then run the test suite: $ yarn test
But they all passed.

I guess the next step would be to reproduce the error in the test suite (midi_io_test.ts ?) and then fix the bug. The problem sounds like a lot of fun but unfortunately I can't be late for my 🍣 dinner!

I'll check in tomorrow to see if someone took a look or if I should keep on digging :)

[adarob]

Thanks so much for helping with this Matt! *From: *Matt Aimonetti <[email protected]> *Date: *Tue, May 7, 2019 at 6:28 PM *To: *tensorflow/magenta-js *Cc: *Subscribed I took a quick uneducated look and while I don't see the AudioContext

…

errors going to http://localhost:8080/player.html using the latest tone.js, I do noticed that the base player sequence starts off and there are a lot of clicks. The sequence once played out is fine again but the clicks are still there. I decided to download the MIDI and surely enough the sequence conversion is off now: [image: image] <https://user-images.githubusercontent.com/113/57342766-57921280-70f5-11e9-93c7-8cbfc9ebeb60.png> Besides the note order being wrong, you can see that the notes are doubled with a very quick note at the end of most note. That's probably what causes the click. I then run the test suite: $ yarn test But they all passed. I guess the next step would be to reproduce the error in the test suite ( midi_io_test.ts ?) and then fix the bug. The problem sounds like a lot of fun but unfortunately I can't be late for my 🍣 dinner! I'll check in tomorrow to see if someone took a look or if I should keep on digging :) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#309 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIJV2HQ4DS6ZQDQTMUFKZTPUIUFPANCNFSM4HLCHZFQ> .

[mattetti]

Apologies for the delay, I caught a cold and spent my free time in bed trying to recover 🤧 I still see this task as a key element to unlock of lot of things beyond the bug fixes. I'm not giving up 💃

[mattetti]

I did some digging and while I haven't found a fix yet, here is some interesting information.

tl;dr: the problem seems to be related to the way the player sets up/retrieves or uses Tone's polyphonic synth. Something in Tone changed in the way our use cases breaks the current behavior. The good news is that the problem is quite isolated, the bad news is that we can't write unit tests for it and it will require more manual untangling 🍝

1. I failed to write unit tests for the player since it relies on tone.js which doesn't run outside of the browser (its context isn't set which makes it bug out). Not a blocker but good to know/remember.
2. I isolated the problem to the start(seq: INoteSequence, qpm?: number): Promise<void> function in core/player.ts. More specifically in the callback/anonymous function inside the Tone Part creation: https://github.com/tensorflow/magenta-js/blob/master/music/src/core/player.ts#L166
3. replacing this.playNote(t, n) by a basic inline implementation fixes the timing problem. Here is an example:

if (this.playClick ||
          (n.pitch !== constants.LO_CLICK_PITCH &&
           n.pitch !== constants.HI_CLICK_PITCH)) {
        const freq = new Tone.Frequency(n.pitch, 'midi');
        const dur = n.endTime - n.startTime;
        synth.triggerAttackRelease(freq, dur, t);

Note that I create a new synth every time we press play to help us debug:

    const synth = new Tone.Synth().toMaster();

Looking into the playNote implementation, we see that the big difference between by hacked version that works and the code in place is that we are calling a helper method called getSynth:

this.getSynth(note.instrument, note.program)
          .triggerAttackRelease(freq, dur, time, velocity);

And looking into getSynth we can quickly see the big difference between the 2 implementations:

private getSynth(instrument: number, program?: number) {
    if (this.synths.has(instrument)) {
      return this.synths.get(instrument);
    } 
  // [... non relevant code]
    } else {
      this.synths.set(instrument, new Tone.PolySynth(10).toMaster());
    }
    return this.synths.get(instrument);
  }

The implementation returns a PolySynth instead of a Synth, switching the type of synth or limiting it to 1 voice fixes the bug for this specific issue but we are also changing the expected behavior. So there seems to be something related to using multiple voices that messes the playback, at least the first time around (re playing the part seems to fix the issue, so the problem might be related to the memoization in the class). Stopping during playback also seems to leave a voice on which can't be stopped unless the page is reloaded.

UPDATE: creating the polysynth in the Player class at instantiation fixes the bug, that seems to indicate a race condition in tone when the synth is created on the fly, stored in a map and notes are triggered right away. @tambien does that ring a bell or make any sense?

[mattetti]

@tambien any chance you have an idea what might be going on?