Race condition in arpeggio processing
A pull request for this has already been merged.
- #7025 by @szeli1 — merged
- Dominant language
- C++
- Stars
- 10.4k
- Forks
- 1.3k
- Avg merge
- 2d 13h
- Merged PRs (30d)
- 7
Description
For instruments that use both NotePlayHandles and an InstrumentPlayHandle (like LB302 and Sf2 Player), [this code in InstrumentPlayHandle.h](https://github.com/LMMS/lmms/blob/b55b1ab059b8db6aac688c85ce4a2fef831d31ea/include/InstrumentPlayHandle.h#L52-L69) 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](https://github.com/LMMS/lmms/files/142977/arpeggio.mmp.zip), 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?
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with include/InstrumentPlayHandle.h, especially the linked processing logic, and inspect the arpeggio.mmp test file attached in the issue. Trace how the mixer processes PlayHandles and how NotePlayHandles interact with the InstrumentPlayHandle. Done means the race is eliminated and the test no longer shows random note skipping.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- audio-video-rtc
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100