midilib

midilib copied to clipboard

MIDIs (even in format 1) can have multiple tempos.

method beats_per_minute in sequence.rb assumes that there is one single tempo on the first track for the entire song. Tempo events can occur on any track, but more importantly, multiple tempo events often occur throughout a song to account for accelerandos, etc.

# Returns the song tempo in beats per minute.
def beats_per_minute
  return DEFAULT_TEMPO if @tracks.nil? || @tracks.empty?
  event = @tracks.first.events.detect { |e| e.kind_of?(MIDI::Tempo) }
  return event ? (Tempo.mpq_to_bpm(event.tempo)) : DEFAULT_TEMPO
end

As a consequence, pulses per second is also incorrect. You can't make the assumption that there is a single, static tempo. A correct implementation of this requires summation across multiple tempos.

def pulses_to_seconds(pulses)
  (pulses.to_f / @ppqn.to_f / beats_per_minute()) * 60.0
end

Would it make more sense to have these two methods take an argument that says at what offset in the song the BPM is being requested? By default, if no argument was given it would behave as it currently does by using the first tempo of the first track.