LMMS / LMMS/lmms

Race condition in arpeggio processing

Open
#2,606 13 comments 0 reactions 0 assignees View on GitHub

A pull request for this has already been merged.

  • #7025 by @szeli1 — merged
bug core
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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.