Lmms: 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]

1. 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.

1. 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:

1. 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
https://github.com/LMMS/lmms/blob/2d583db990385506bff7020f4b4109976af5835f/src/core/InstrumentFunctions.cpp#L355

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 https://github.com/LMMS/lmms/issues/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: https://github.com/LMMS/lmms/blob/2070ef21f575a4fbc80505ccd0280c756f03c5e8/include/ThreadableJob.h#L70-L77

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:
  https://github.com/LMMS/lmms/blob/2070ef21f575a4fbc80505ccd0280c756f03c5e8/src/core/InstrumentFunctions.cpp#L381-L383
• 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.