Locked Note with VeSTige While Changing Tempo

[BiRD4]

When I play a note while changing the tempo of a song, the note I played doesn't end until I have played it again (or stopped the song if it's in the song editor).

Using Windows 8.1 64-bit and LMMS version 1.2.0-rc2. I used Helm (64-bit) in VeSTige.

[musikBear]

Specific for one VST, or is this also the case with those that are tested?

[BiRD4]

The only VST that I currently have is Helm, so someone else would have to test to be sure, but it appears to be an LMMS issue. (mtytel/helm#138)

[DomClark]

It appears that the issue lies in NotePlayHandle::resize.

lmms/src/core/NotePlayHandle.cpp

Lines 529 to 540 in f79c292

	void NotePlayHandle::resize( const bpm_t _new_tempo )
	{
	double completed = m_totalFramesPlayed / (double) m_frames;
	double new_frames = m_origFrames * m_origTempo / (double) _new_tempo;
	m_frames = (f_cnt_t)new_frames;
	m_totalFramesPlayed = (f_cnt_t)( completed * new_frames );
	for( NotePlayHandleList::Iterator it = m_subNotes.begin(); it != m_subNotes.end(); ++it )
	{
	( *it )->resize( _new_tempo );
	}
	}

The product m_origFrames * m_origTempo is a signed 32-bit integer multiplication, which is liable to overflow. This always occurs when playing with a MIDI controller or the instrument window keyboard, since m_origFrames is set to typeInfo<f_cnt_t>::max() / 2 (2^30-1) and m_origTempo has a minimum value of 10. While it's unlikely that this would also occur during normal song playback, it's far from impossible: at the maximum tempo of 999bpm, and a sample rate of 44100Hz, a 100-second note will overflow here.

Using doubles for the multiplication will fix the latter case, but not the former; the conversion back to f_cnt_t can overflow for sufficiently large tempo changes, which leads to undefined behaviour. (In practise, GCC seems to use the cvttsd2si instruction to convert back, which is documented to produce the value 0x80000000=-2^31 in case of overflow.) To fully address this, I would suggest we also either bound the resulting frame count, or have a special case to do nothing if m_origin == OriginMidiInput.

The observed issue is caused by the resulting incorrect values of m_totalFramesPlayed and m_frames, which cause the following condition in NotePlayHandle::play to be triggered erroneously:

lmms/src/core/NotePlayHandle.cpp

Lines 222 to 229 in f79c292

	if( m_released == false &&
	instrumentTrack()->isSustainPedalPressed() == false &&
	m_totalFramesPlayed + framesThisPeriod > m_frames )
	{
	noteOff( m_totalFramesPlayed == 0
	? ( m_frames + offset() ) // if we have noteon and noteoff during the same period, take offset in account for release frame
	: ( m_frames - m_totalFramesPlayed ) ); // otherwise, the offset is already negated and can be ignored
	}

The result is a note-off message with a wildly incorrect offset. In the case that the offset is negative, Helm doesn't turn off the note, but no later note-off message will be sent so note gets stuck. Other VSTi plugins and other built-in MIDI-driven instruments apply the note-off immediately, which simply results in a staccato note rather than anything stuck.

[DomClark]

Fixed in #5365.