Race condition in arpeggio processing

[Fastigium]

For instruments that use both NotePlayHandles and an InstrumentPlayHandle (like LB302 and Sf2 Player), this code in InstrumentPlayHandle.h ensures that all NotePlayHandles are processed before the InstrumentPlayHandle. To do so, it just processes any unfinished NotePlayHandle jobs itself. This often results in NotePlayHandles being processed twice simultanously, wreaking havoc with the internal frame counters. This causes random note skipping in the second part of this test file by zonkmachine, and probably causes other strange behaviors as well. It may, for example, be the cause of #2565.

At any rate, it's something to fix. However, I'd like some opinions on how to fix it best. As far as I know, the mixer can't give any guarantees on order and overlapping when processing PlayHandles. Neither can a PlayHandle delay its execution and allow a different one to be processed instead. Therefore, I see the following solutions to this problem:

1. Implement PlayHandle dependency support in the mixer, allowing the InstrumentPlayHandle to depend on the corresponding NotePlayHandles. This seems like a lot of work and extra code to maintain to fix such a small issue, but it may come in handy for other PlayHandle types that have dependencies. If anyone knows other PlayHandles that could use it, please mention it.
2. Implement a yield mechanism, allowing threads to pause execution and return to the job queue, making room for a different thread. This would allow the InstrumentPlayHandle to yield when one or more of its NotePlayHandles are not finished yet. Probably easier/smaller to implement, if a bit less elegant.
3. Make the InstrumentPlayHandle busy-wait until the NotePlayHandles are ready. This wastes time and CPU cycles, but is ridiculously easy to implement.

Number 3 is a no-go as far as I'm concerned, but the choice between 1 and 2 depends on how many other uses 1 has or may have in the future. @LMMS/developers Feedback welcome! What do you think of the solutions? Do you know alternative solutions?

[Fastigium]

Credits go to @zonkmachine for uncovering this issue and suggesting the link with #2565!

[zonkmachine]

Closed #340 as a duplicate of this one.

[zonkmachine]

3. Make the InstrumentPlayHandle busy-wait until the NotePlayHandles are ready. This wastes time and CPU cycles, but is ridiculously easy to implement.

A simple fix would be good actually. Just for the sake of helping to trouble shoot the arpeggiator, so we can take this one out of the equation. I don't mean merging something that wastes unnecessary cpu cycles.

[PhysSong]

1. Implement PlayHandle dependency support in the mixer, allowing the InstrumentPlayHandle to depend on the corresponding NotePlayHandles. This seems like a lot of work and extra code to maintain to fix such a small issue, but it may come in handy for other PlayHandle types that have dependencies. If anyone knows other PlayHandles that could use it, please mention it.

Good idea. I think we may apply this for whole ThreadablJobs, including FX channels.

3. Make the InstrumentPlayHandle busy-wait until the NotePlayHandles are ready. This wastes time and CPU cycles, but is ridiculously easy to implement.

Looks not so good, but not too bad. Sounds better than double processing at least.

[zonkmachine]

Looks not so good, but not too bad. Sounds better than double processing at least.

I meant this just to help trouble shoot the arpeggiator. For now at least if it's noticeably wasteful with cpu cycles.

[PhysSong]

And I have one more solution:

4. Add something into NotePlayHandle to check whether it is processing(or have processed) or not.

[zonkmachine]

I added this: qDebug() << "cnphv.size(): " << cnphv.size(); after

lmms/src/core/InstrumentFunctions.cpp

Line 355 in 2d583db

ConstNotePlayHandleList cnphv = NotePlayHandle::nphsOfInstrumentTrack( _n->instrumentTrack() );

Playing 3 note chords in a loop I got some instances that got the count wrong. This was pretty much expected. I think the issues we see in the arpeggiator mostly relates to #2606.

cnphv.size():  3 
cnphv.size():  5 
cnphv.size():  3 
cnphv.size():  3 
cnphv.size():  3 
cnphv.size():  4 
cnphv.size():  3 
cnphv.size():  3 

[DomClark]

I did some investigation into the arpeggio skipping and here's what I found:

I don't think any NotePlayHandles are being processed multiple times per mixer round:

• I can reproduce the arpeggio bugs with TripleOscillator, which doesn't use an InstrumentPlayHandle, and with VeSTige, which is MIDI-based and so takes the shortcut out of the top of InstrumentPlayHandle::play.
• If the frame counters were made incorrect, this would mess up the timing of arpeggio notes, but those that aren't skipped play at the correct time. Also, notes would generally end too early.
• Even though process may be called multiple times per mixer round on a NotePlayHandle, ThreadableJob::process atomically checks and updates its state to ensure that the processing is never done more than once:

  lmms/include/ThreadableJob.h

  Lines 70 to 77 in 2070ef2

  	void process()
  	{
  	if( m_state.testAndSetOrdered( Queued, InProgress ) )
  	{
  	doProcessing();
  	m_state = Done;
  	}
  	}

Currently, the code uses a variant of suggestion (3):

Make the InstrumentPlayHandle busy-wait until the NotePlayHandles are ready. This wastes time and CPU cycles, but is ridiculously easy to implement.

If any notes haven't yet started processing, the wait-loop will process them, but if another thread is already working on them, the call to process returns straight away and the loop becomes a busy-wait.
This may well be worth optimising, but isn't exactly a bug.

My guess for the cause of the arpeggio skipping is this:

• When not in free mode, the current frame of the arpeggio is taken from the first NotePlayHandle belonging to the track:
  

  lmms/src/core/InstrumentFunctions.cpp

  Lines 381 to 383 in 2070ef2

  	int cur_frame = ( ( m_arpModeModel.value() != FreeMode ) ?
  	cnphv.first()->totalFramesPlayed() :
  	_n->totalFramesPlayed() ) + arp_frames - 1;

• If the track has multiple notes playing at once, there is a race condition: the notes are processed on different threads, so it can vary as to whether the first note's frame count has been updated when the arpeggio code runs for the other notes. This can lead to the arpeggiator repeating or skipping a buffer's worth of processing.

I'm not sure how to verify if this is the case or how to fix it, but it matches the symptoms well:

• The arpeggiator sometimes plays notes twice as well as skipping notes (this is easier to see with an external MIDI receiver).
• The arpeggio for the first note is always correct; the skipping only happens for other notes.
• The errors are random and change with each play through.

Other notes:

• I failed to reproduce zonkmachine's findings with the incorrect note count.
• There appear to be some slight mistakes with the frame counting in the arpeggiator (or I'm reading the code wrong), but after fixing them with the following patch the bug persisted. (The observations above were made with the patch applied.)

diff --git a/src/core/InstrumentFunctions.cpp b/src/core/InstrumentFunctions.cpp
index b44dbdc90..5f8489146 100644
--- a/src/core/InstrumentFunctions.cpp
+++ b/src/core/InstrumentFunctions.cpp
@@ -375,27 +375,23 @@ void InstrumentFunctionArpeggio::processNote( NotePlayHandle * _n )
 	const f_cnt_t arp_frames = (f_cnt_t)( m_arpTimeModel.value() / 1000.0f * Engine::mixer()->processingSampleRate() );
 	const f_cnt_t gated_frames = (f_cnt_t)( m_arpGateModel.value() * arp_frames / 100.0f );
 
-	// used for calculating remaining frames for arp-note, we have to add
-	// arp_frames-1, otherwise the first arp-note will not be setup
-	// correctly... -> arp_frames frames silence at the start of every note!
 	int cur_frame = ( ( m_arpModeModel.value() != FreeMode ) ?
 						cnphv.first()->totalFramesPlayed() :
-						_n->totalFramesPlayed() ) + arp_frames - 1;
+						_n->totalFramesPlayed() );
 	// used for loop
 	f_cnt_t frames_processed = ( m_arpModeModel.value() != FreeMode ) ? cnphv.first()->noteOffset() : _n->noteOffset();
 
 	while( frames_processed < Engine::mixer()->framesPerPeriod() )
 	{
-		const f_cnt_t remaining_frames_for_cur_arp = arp_frames - ( cur_frame % arp_frames );
-		// does current arp-note fill whole audio-buffer?
-		if( remaining_frames_for_cur_arp > Engine::mixer()->framesPerPeriod() )
+		const f_cnt_t cur_arp_frame = cur_frame % arp_frames;
+		if( cur_arp_frame > 0 )
 		{
-			// then we don't have to do something!
-			break;
+			// skip to start of next arp note
+			frames_processed += arp_frames - cur_arp_frame;
+			cur_frame += arp_frames - cur_arp_frame;
+			continue;
 		}
 
-		frames_processed += remaining_frames_for_cur_arp;
-
 		// in sorted mode: is it our turn or do we have to be quiet for
 		// now?
 		if( m_arpModeModel.value() == SortMode &&
@@ -497,6 +493,9 @@ void InstrumentFunctionArpeggio::processNote( NotePlayHandle * _n )
 			sub_note_key < 0 ||
 			Engine::mixer()->criticalXRuns() )
 		{
+			// update counters
+			frames_processed += arp_frames;
+			cur_frame += arp_frames;
 			continue;
 		}
 

[PhysSong]

@DomClark Thank you for the investigation! I think the best way to fix this is to process arpeggio before rendering any notes. It will ensure the arpeggiator uses correct frame counts.

[T0NIT0RMX]

@PhysSong Is this somehow related to #4380 ?

[PhysSong]

@T0NIT0RMX I think it's not.

[zonkmachine]

Here is a project that automates between different arpeggiator settings. It has two tracks playing the same notes but one is inverted so you only hear a note if one of them skips over one. It cycles through the various modes and between one and two notes. I only hear notes skipping on bars 21 - 24, two notes and sync mode.

arpnotesilence-01.mmp.zip

Edit: Exporting the project indeed shows occasional glitches on the other settings. When I exported bar 1-20 some 300 times I have so far seen about 3 notes skipped in some 2 hours of wav file. I don't know if there are notes being skipped in both tracks simultaneously. That's a possibility.

[zonkmachine]

This causes random note skipping in the second part of this test file, and probably causes other strange behaviors as well.

The issues in my original arpeggio.mmp test file seem to be fixed in #7025
The issues in my last post above isn't however, and seem to be unrelated.