notator / WebMIDISynthHost

Hosts software Web MIDI Synths that implement a superset of the Web MIDI API Output Device interface.
Other
18 stars 3 forks source link

Audible artifacts (clicks) on noteOff #34

Open timjrd opened 7 years ago

timjrd commented 7 years ago

Hi :)

I'm experimenting with your ResidentSf2Synth and I was hearing annoying clicks each noteOff event (it's mostly outstanding when you send quickly quite a lot of noteOn/noteOff).

I managed to almost get rid of these clicks by randomly hacking your code, but I didn't send a pull request because I am not sure if it's a good solution, so I'm posting the patched function here :

By the way thank you for this very cool software :) .

My changes are tagged with timjrd

// file residentSf2Synth/soundFontSynthNote.js line 159

    SoundFontSynthNote.prototype.noteOff = function()
        {
                var volRelease = Math.max(0.05, this.instrument.volRelease); // timjrd

        var
        instrument = this.instrument,
        bufferSource = this.bufferSource,
        output = this.gainOutput,
        //now = this.ctx.currentTime,
        now = this.ctx.currentTime + 0.05, // timjrd
        // begin gree
        //volEndTime = now + instrument.volRelease,
                // ===============
                // IMPORTANT NOTE:
                // With the Titanic soundfont that I use there is no need to reduce
                // instrument.volRelease contrary to Arachno, but this is unrelated
                // to the artifacts.
                // ===============  
        modEndTime = now + instrument.modRelease;
        // end gree
        // begin ji
        // instrument.volRelease is 3.08 in preset 0 (grand piano) in the Arachno font.   
        // It cannot be the case that a piano note only stops 3.08 seconds after a
        // noteOff arrives. Multiplying these parameters by 0.1 sounds more realistic...
        //   volEndTime = now + (instrument.volRelease * 0.1),
        //   modEndTime = now + (instrument.modRelease * 0.1);
            // end ji

                var volEndTime = now + volRelease; // timjrd

        if(!this.audioBuffer)
        {
            return;
        }

        //---------------------------------------------------------------------------
        // Release
        //---------------------------------------------------------------------------
        // begin original gree
        //output.gain.cancelScheduledValues(0);
        //output.gain.linearRampToValueAtTime(0, volEndTime);
        //bufferSource.playbackRate.cancelScheduledValues(0);
        //bufferSource.playbackRate.linearRampToValueAtTime(this.computedPlaybackRate, modEndTime);
        // end original gree

        // begin ji
            //output.gain.cancelScheduledValues(volEndTime);
            //output.gain.linearRampToValueAtTime(0, volEndTime);
            //bufferSource.playbackRate.cancelScheduledValues(modEndTime);
        //bufferSource.playbackRate.linearRampToValueAtTime(this.computedPlaybackRate, modEndTime);
                // end ji

                // begin timjrd
            output.gain.cancelScheduledValues(volEndTime);
            output.gain.setTargetAtTime(0, now, volRelease);
            bufferSource.playbackRate.cancelScheduledValues(modEndTime);
        bufferSource.playbackRate.linearRampToValueAtTime(this.computedPlaybackRate, modEndTime);            
                // end timjrd

        bufferSource.loop = false;
        //bufferSource.stop(volEndTime);
        bufferSource.stop(volEndTime + 0.1); // timjrd

        // disconnect
        setTimeout(
          (function(note)
            {
                return function()
                {
                    note.bufferSource.disconnect(0);
                    note.panner.disconnect(0);
                    note.gainOutput.disconnect(0);
                };
            }(this)),
            //instrument.volRelease * 1000
            (volRelease + 1) * 1000 // timjrd
        );
    };
notator commented 7 years ago

Hi Timothée,

As you suggested, the current version now

  1. limits both volEndTime and modEndTime to maximum values of 0.05sec.
  2. uses setTargetAtTime(...) instead of linearRampToValueAt(...) to close the gain during the release phase.

I've also changed the order in which cancelScheduledValues(...) is called. Its called as the last function in the closing sequence in all the examples I've seen on the web, so that might make a difference when it comes to avoiding click artifacts. Could you test the current version to see if the clicks have gone away?

You might try your other suggestions again if the clicks are still there: now = this.ctx.currentTime + 0.05 volEndTime + 0.1 and volRelease + 1

However, introducing arbitrary constants into the code probably hides underlying problems, so its something that should be avoided if possible. I'm currently digging deep into the parser to see if I can find any relevant parameter(s) that the synth is ignoring. The gree code was a very good start, but there are comments saying that it is incomplete, and its not necessarily bug-free. I'll report back here if/when I find anything pertinent.

notator commented 7 years ago

I've just completed a detailed review of the code while looking as closely as possible at the SoundFont 2.04 specification. The terminology is a nightmare, but I understand a lot more now, and am confident that the parsing is working correctly. (At least of the variables that are currently being used.) Made a lot of changes to the code, and have given up trying to document the changes I made to the original gree version. The most important/drastic correction was to the way envelope decay times are calculated. The above noteOff function has now become much simpler (!). I've tested this version on the Assistant Performer (using just the Arachno Grand Piano), and on the WebMIDISynthHost (using all the available presets there). It has to be said that the Grand Piano does not yet sound as good as it does on the VirtualMIDISynth, so there's still some work to do. (I think that work would be best done by people who are experts with the Web Audio API.) I've left extensive comments in the code, and think that the soundFont parsing is now complete and substantially bug-free. The tweeking will have to be done in only a few functions: createKeyInfo() in soundFont.js, and the functions in soundFontSynthNote.js (All the necessary variables have been set. It will usually just be a case of deciding what to do with them, and fixing decimal point positions...). Note: There were some artefacts at loop endings when I first tested the French Horn in the WebMIDISynthHost. Second time round, everything was very clean. The code is uncompressed and currently very verbose. Maybe some optimization will have to be done, or compromises made, before this code works perfecly.