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 semi-infinite loop in PlaybackModel (and an assertion) when opening a certain MusicXML score #23339

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

cbjeukendrup
Copy link
Contributor

Resolves: #23276

  • Check whether string is empty before calling .back()
    This fixes an assertion failure.
    Actually it should be !m_wordsText.empty() && !rawWordsText.empty(), but !rawWordsText.empty() implies !m_wordsText.empty(), so checking only !rawWordsText.empty() suffices.

  • Only check for measure repeats once per staff
    The score from Memory leak upon opening MusicXML file exported from Sibelius #23276 contains a weird situation: notes in voice 2, while there is a measure repeat sign in voice 1. And that repeated for the entire score.

    The playback model would render a measure repeat in two situations: 1) when finding one 2) when a segment's measure turns out to be a measure repeat measure. "Rendering" means here that it looks back at the previous measure and renders that to the location of the measure repeat.

    So, for measure 2 of that score from that issue, it would first encounter the measure repeat element and render playback for it, and thus for the voice 2 notes, it would notice that their measure is a measure repeat measure and thus render the measure repeat again. So, we re-look at measure 1 twice.

    For measure 2, that was just a bit wasteful, but on the other hand not catastrophic. Where it gets exciting is beyond measure 2. Measure 3 contains the same situation as measure 2. So, for measure 3, we look back at measure 2 twice. But for each time that we look at measure 2, we look twice at measure 1. So, now we look 4 times at measure 1! And it only gets worse from here; exponentially worse, to be precise.

    We solve this by only checking whether the current measure is a measure repeat once per staff, i.e. at the first voice. This way, we're back in the linear world. (Well, rendering the n-th measure now takes linear time, so rendering n measures takes quadratic time.)

Actually it should be `!m_wordsText.empty() && !rawWordsText.empty()`, but `!rawWordsText.empty()` implies `!m_wordsText.empty()`, so checking only `!rawWordsText.empty()` suffices.
The score from musescore#23276 contains a weird situation: notes in voice 2, while there is a measure repeat sign in voice 1. And that repeated for the entire score.

The playback model would render a measure repeat in two situations: 1) when finding one 2) when a segment's measure turns out to be a measure repeat measure. "Rendering" means here that it looks back at the previous measure and renders that to the location of the measure repeat.

So, for measure 2 of that score from that issue, it would first encounter the measure repeat element and render playback for it, and thus for the voice 2 notes, it would notice that their measure is a measure repeat measure and thus render the measure repeat _again_. So, we re-look at measure 1 twice.

For measure 2, that was just a bit wasteful, but on the other hand not catastrophic. Where it gets exciting is _beyond_ measure 2. Measure 3 contains the same situation as measure 2. So, for measure 3, we look back at measure 2 twice. But for each time that we look at measure 2, we look twice at measure 1. So, now we look 4 times at measure 1! And it only gets worse from here; _exponentially_ worse, to be precise.

We solve this by only checking whether the current measure is a measure repeat once per staff, i.e. at the first voice. This way, we're back in the linear world. (Well, rendering the n-th measure now takes linear time, so rendering n measures takes quadratic time.)
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jun 24, 2024
Actually it should be `!m_wordsText.empty() && !rawWordsText.empty()`, but `!rawWordsText.empty()` implies `!m_wordsText.empty()`, so checking only `!rawWordsText.empty()` suffices.

Bacport of musescore#23339, commit 1
@zacjansheski
Copy link
Contributor

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved
#23276 FIXED

@cbjeukendrup cbjeukendrup merged commit d71bf34 into musescore:master Jul 3, 2024
11 checks passed
@cbjeukendrup cbjeukendrup deleted the musicxml_crash branch July 3, 2024 14:45
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 9, 2024
Actually it should be `!m_wordsText.empty() && !rawWordsText.empty()`, but `!rawWordsText.empty()` implies `!m_wordsText.empty()`, so checking only `!rawWordsText.empty()` suffices.

Bacport of musescore#23339, commit 1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak upon opening MusicXML file exported from Sibelius
3 participants