Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix notes getting stuck under high CPU conditions #4908

Merged
merged 2 commits into from Apr 24, 2019

Conversation

Projects
None yet
5 participants
@DomClark
Copy link
Member

commented Mar 22, 2019

Fixes #4826. Also happens to fix a bug where notes get stuck for MIDI-based instruments if the arpeggiator is turned on while the note is playing.

This PR changes NotePlayHandles so they send a note-on message when they are first played, rather than when they are constructed. This means that NotePlayHandles that are immediately discarded due to high CPU usage no longer send note-on messages to their plugins.

@DomClark

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

CI failure appears to be unrelated to these changes.

@karmux

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Fantastic! I was suffering from notes getting stuck a lot and with these code changes they don't anymore.

@DomClark

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2019

Don't merge this yet; it seems like it might have some issues with tempo automation.

@qnebra

This comment has been minimized.

Copy link

commented Mar 23, 2019

If this was what I thinking then this was really amazing fix for me. How many notes are lose due to high CPU usage.

@DomClark

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

Tempo automation with this pull request can sometimes create stuck notes; this is due to m_totalFramesPlayed being zero for two consecutive buffers, resulting in an extra note-on message. This happens often when playing using the keyboard or a MIDI controller, due to the overflow issues mentioned in #3640 (comment), and can also occur occasionally during normal playback when m_totalFramesPlayed is changed as the NotePlayHandle is resized. @PhysSong suggests in #3775 (comment) that m_totalFramesPlayed should be left alone during NotePlayHandle resizing; until that is implemented, I'm just going to add an additional check that the note-on message has not yet been sent.

@DomClark

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

This should work properly now; it can be reviewed and then hopefully merged.

@PhysSong

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

This means that NotePlayHandles that are immediately discarded due to high CPU usage no longer send note-on messages to their plugins.

Does this PR affect MIDI output ports connected to the instrument? Isn't InstrumentTrack::processOutEvent supposed to handle MIDI output by calling m_midiPort.processOutEvent()?

@DomClark

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

Does this PR affect MIDI output ports connected to the instrument?

Yes, it does. However, prior to this change, MIDI output ports would be given a note-on message followed immediately by a note-off message in the same MIDI tick, resulting in either a very short note or no note at all. After this change, they will simply not receive any message, so I don't think this is a serious regression.

The issue here is that MIDI output ports and MIDI-based instruments both receive the same MIDI events, so it's not possible to have the MIDI output port receive a note but the instrument not. Also, MIDI notes are driven by play handles, so these would need to be kept around rather than be discarded in high-CPU conditions. Perhaps in the future we should separate external MIDI output from internal MIDI control and CPU-saving note skipping, but I feel this is out of scope here.

@PhysSong

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Then I'd say let's merge this. I can't see any other issues with these changes. 🚀

@DomClark DomClark merged commit 461facc into LMMS:stable-1.2 Apr 24, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.