insomnimus / nodi

A rust library for playing and abstraction of MIDI files.
MIT License
20 stars 4 forks source link

Time drift during MIDI playback #1

Open mdwn opened 4 months ago

mdwn commented 4 months ago

For context: I am writing https://github.com/mdwn/mtrack, and I am playing a MIDI track into a MIDI to DMX translator against a click track.

As I was testing playback, I noticed that the lights were slowly getting behind the beat of the click. After doing some digging, I believe the issue lies here: https://github.com/insomnimus/nodi/blob/main/src/player.rs#L45. The sleep is accurate, however, I think the writing of the events out using midir is adding in a slight latency per tick, which cumulatively becomes noticeable.

I was able to fix within my repo with this PR: https://github.com/mdwn/mtrack/pull/32. However, I suspect a better solution could be integrated directly into nodi. I'd be happy to submit a PR, but wanted to check with you first.

mdwn commented 4 months ago

Just to be specific:

/// AccurateTimer is a timer for the nodi player that allows a more accurate clock. It uses the last
/// known instant to properly calculate the next intended sleep duration.
struct AccurateTimer<T: Timer> {
    timer: T,
    last_instant: Option<Instant>,
}

impl<T: Timer> AccurateTimer<T> {
    fn new(timer: T) -> AccurateTimer<T> {
        AccurateTimer {
            timer,
            last_instant: None,
        }
    }
}

impl<T: Timer> Timer for AccurateTimer<T> {
    fn sleep_duration(&mut self, n_ticks: u32) -> std::time::Duration {
        let mut duration = self.timer.sleep_duration(n_ticks);

        // Modify the sleep duration if the last duration is populated, as we
        // know about when the next tick should be.
        match self.last_instant {
            Some(last_instant) => {
                self.last_instant = Some(last_instant.add(duration));

                // Subtract the duration unless it would be an overflow. If so, use the original duration.
                duration = match duration.checked_sub(Instant::now().duration_since(last_instant)) {
                    Some(duration) => duration,
                    None => duration,
                };
            }
            None => self.last_instant = Some(time::Instant::now()),
        };

        duration
    }

    fn change_tempo(&mut self, tempo: u32) {
        self.timer.change_tempo(tempo);
    }
}

Just to clarify, when I'm doing here is calculating what the next tick should "objectively" be and only sleep that amount, even if the tick length is technically longer.

insomnimus commented 1 month ago

Sorry for the large delay. Yes, I'd appreciate a PR!