Note length is calculated before tempo automation

[LostRobotMusic]

Please let me know if this is a duplicate.

Steps to reproduce:

1. Place one note at the beginning of measure 2
2. Automate the tempo (no slope) so measure 1 is 100 BPM, and immediately switches to 200 BPM at the beginning of measure 2
3. Play it, and you'll notice that the note lasts twice as long as usual.

This is most likely because the note's length is calculated at 100 BPM, and then the project's tempo switches to 200 BPM. This is frustrating, because whenever you want a sudden tempo change you have to go into 1/192 and change it right before the measure switches (and may also throw things off slightly when somebody wants to remix your song and you send them the project stems).

[DomClark]

Reproduced.

This is most likely because the note's length is calculated at 100 BPM, and then the project's tempo switches to 200 BPM.

Pretty much, but, as always, there's a little more to it than that. LMMS is supposed to change the length of notes to match tempo changes. You'll notice that if you place the note a tick before (or after) the tempo change, it behaves properly. The problem appears to be as follows:

• Song::processNextBuffer processes the automation, then processes the new note.
  

  lmms/src/core/Song.cpp

  Lines 400 to 408 in 032c324

  	processAutomations(trackList, m_playPos[m_playMode], framesToPlay);
  	// loop through all tracks and play them
  	for( int i = 0; i < trackList.size(); ++i )
  	{
  	trackList[i]->play( m_playPos[m_playMode],
  	framesToPlay,
  	framesPlayed, tcoNum );
  	}

• The tempo automation updates the tempo value, and emits Song::m_tempoModel::dataChanged, which is connected to Song::setTempo. This connection is not explicitly direct, and the signal is emitted from the processing thread, so it is queued.

• The NotePlayHandle is created in InstrumentTrack::play, where the length of the note in frames is calculated based on Engine::framesPerTick. This value is still set based on the old tempo, since it is updated from Song::setTempo, which is still queued.
  

  lmms/src/tracks/InstrumentTrack.cpp

  Lines 686 to 689 in 032c324

  	const f_cnt_t note_frames =
  	cur_note->length().frames( frames_per_tick );
  	NotePlayHandle* notePlayHandle = NotePlayHandleManager::acquire( this, _offset, note_frames, *cur_note );

• The NotePlayHandle's initial tempo is set to the new tempo, which is read directly from the tempo model and thus not affected by the queued connection.
  

  lmms/src/core/NotePlayHandle.cpp

  Line 71 in 032c324

  m_origTempo( Engine::getSong()->getTempo() ),

• Later, the queued signal is processed, and Song::setTempo is called on the main thread. No matter when the thread is scheduled, the code here is guaranteed to run after the relevant buffer has been processed since the method begins with Engine::mixer()->requestChangeInModel();. Here the notes are resized to the new tempo, then Engine::updateFramesPerTick is finally called so later notes behave correctly.
  

  lmms/src/core/Song.cpp

  Lines 141 to 157 in 032c324

  	Engine::mixer()->requestChangeInModel();
  	const bpm_t tempo = ( bpm_t ) m_tempoModel.value();
  	PlayHandleList & playHandles = Engine::mixer()->playHandles();
  	for( PlayHandleList::Iterator it = playHandles.begin();
  	it != playHandles.end(); ++it )
  	{
  	NotePlayHandle * nph = dynamic_cast<NotePlayHandle *>( *it );
  	if( nph && !nph->isReleased() )
  	{
  	nph->lock();
  	nph->resize( tempo );
  	nph->unlock();
  	}
  	}
  	Engine::mixer()->doneChangeInModel();
  	Engine::updateFramesPerTick();

• NotePlayHandle::resize resizes the NotePlayHandle based on the ratio of the new and original tempos, but since the original tempo was set to the new tempo, no resizing occurs.
  

  lmms/src/core/NotePlayHandle.cpp

  Lines 531 to 534 in 032c324

  	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 );

TL;DR: The note has the wrong length because neither the original size calculation nor the tempo resizing give the note the correct length. The original size calculation only works for notes after the tempo change, and the tempo resizing only works for notes before it.

How to fix: the connection from Song::m_tempoModel::dataChanged to Song::setTempo should be direct. I didn't make this change in #4692 because I was scared off by the use of Engine::mixer()->requestChangeInModel();, thinking there was something else critical resting on it, but after looking more at the code I think it should be reasonable to change it.

@douglasdgi Can you try making the following change and see if it fixes the bug? In src/core/Song.cpp, change lines 94-97 from

	connect( &m_tempoModel, SIGNAL( dataChanged() ),
						this, SLOT( setTempo() ) );
	connect( &m_tempoModel, SIGNAL( dataUnchanged() ),
						this, SLOT( setTempo() ) );

to

	connect( &m_tempoModel, SIGNAL( dataChanged() ),
						this, SLOT( setTempo() ), Qt::DirectConnection );
	connect( &m_tempoModel, SIGNAL( dataUnchanged() ),
						this, SLOT( setTempo() ), Qt::DirectConnection );

[LostRobotMusic]

@DomClark It looks like it worked perfectly, though I am getting constant complaints in the terminal when running LMMS:
QObject::connect: Cannot queue arguments of type 'bpm_t' (Make sure 'bpm_t' is registered using qRegisterMetaType().)

[LostRobotMusic]

@DomClark Ignore the last post, I replaced the wrong part (because I modified Song.cpp earlier). I'll try it on a fresh copy of LMMS.

[LostRobotMusic]

Nevermind my neverminding of that post (someday I'll grow a brain), I tried it on a fresh copy of LMMS RC8 and the terminal gave me the same complaints. But it does appear to function just fine.

[DomClark]

It looks like it worked perfectly, though I am getting constant complaints in the terminal when running LMMS:
QObject::connect: Cannot queue arguments of type 'bpm_t' (Make sure 'bpm_t' is registered using qRegisterMetaType().)

Glad to hear that it works; I'll make a PR soon. The warning is from further connections downstream which are now queued instead of the initial dataChanged() -> setTempo() connection. I'll address those as well.