Fix bug introduced by #5657 #5982
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a bug introduced by LMMS#5657 where reloading a project and playing it could cause a Segmentation Fault crash. After some debugging, @DomClark tracked the issue to be likely a use-after-free being caused by m_oldAutomatedValues not being cleared when the project was loaded again. This commit adds a line to clear the m_oldAutomatedValues map on Song::clearProject(), which is called from Song::loadProject(). Co-authored-by: Dominic Clark <mrdomclark@gmail.com>
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Linux
macOS
Windows
🤖{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://13596-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.104%2Bg487fb3f-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13596?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://13599-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.104%2Bg487fb3f0f-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13599?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://13605-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.104%2Bg487fb3f0f-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13605?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/sjy3grqjmc2g4dkh/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38799424"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/1i2q43vr7uunt7qv/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38799424"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://13603-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.104%2Bg487fb3f0f-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13603?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "04f5c63b1a6a2985b297c703c63ccfb7fcac4a94"} |
Veratil
approved these changes
Apr 13, 2021
|
One liner, fixes what is supposed to fix. Test project is reloaded and played this time without crash. |
Now, instead of using a Signal/Slot connection to move the control of the models back to the controllers, every time the song is processing the automations, the control of the models that were processed in the last cycle are moved back to the controller. The same is done under Song::stop(), so the last cycle models control is moved back to the controller. That removes the need to have a pointer to the controlled model in the controller object.
|
Last commit introduces some changes to the behavior of #5657 that remove the need to hold a pointer to the controlled model on the controller object. |
|
Looks like those changes broke behaviour of 5657. Be more corrent, it doesn't work like previously. User needs to mute automation track to regain control with MIDI controller. |
Can you ellaborate? I tested here with a LFO controller and it behaved as previously (LFO has control when automation is not affecting model, when it's recording and when music is stopped). Could you describe step by step what you did and how it behaved differently? |
|
Looks like it could be a quickest revocation of the sentence ever. I did more testing, and thinking about it. It wasn't broken, and I was just an idiot. |
DomClark
reviewed
Apr 18, 2021
src/core/Song.cpp
Outdated
Comment on lines
408
to
416
| // Checks if an automated model stopped being automated by automation patterns | ||
| // so we can move the control back to any connected controller again | ||
| // Moves the control of the models that were processed earlier to their controllers. | ||
| // If they get processed again, setAutomatedValue() will move the control back | ||
| // to the automation. | ||
| if (!m_oldAutomatedValues.isEmpty()) | ||
| { | ||
| for (auto it = m_oldAutomatedValues.begin(); it != m_oldAutomatedValues.end(); it++) | ||
| { | ||
| AutomatableModel * am = it.key(); | ||
| if (am->controllerConnection() && !values.contains(am)) | ||
| { | ||
| am->setUseControllerValue(true); | ||
| } | ||
| am->setUseControllerValue(true); |
There was a problem hiding this comment.
setUseControllerValue causes the model to emit its dataChanged signal, so this change means that all automated models will emit that signal twice per buffer (plus an additional third time if their value actually changes). I'm not sure what sort of impact this has, but it may be worth looking at.
Adds mixer model change request to avoid race condition. Revert changes to processAutomations.
DomClark
approved these changes
Apr 20, 2021
src/core/Song.cpp
Outdated
Comment on lines
695
to
702
| if (!m_oldAutomatedValues.isEmpty()) | ||
| { | ||
| for (auto it = m_oldAutomatedValues.begin(); it != m_oldAutomatedValues.end(); it++) | ||
| { | ||
| AutomatableModel * am = it.key(); | ||
| am->setUseControllerValue(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
The if statement here isn't necessary.
Since we are iterating from QVector::begin() to QVector::end() there's no need to check if it's empty first.
|
Random pipelines seem to be failing, reran once and now linux.gcc is failing instead of mingw32. Travis failure seems to be the same problem. Assuming there's a hiccup in the system somewhere on github side, so merging since good. |
sdasda7777
pushed a commit
to sdasda7777/lmms
that referenced
this pull request
Jun 28, 2022
* Fix bug introduced by LMMS#5657 There was a bug introduced by LMMS#5657 where reloading a project and playing it could cause a Segmentation Fault crash. After some debugging, @DomClark tracked the issue to be likely a use-after-free being caused by m_oldAutomatedValues not being cleared when the project was loaded again. This commit adds a line to clear the m_oldAutomatedValues map on Song::clearProject(), which is called from Song::loadProject(). Now, instead of using a Signal/Slot connection to move the control of the models back to the controllers, every time the song is processing the automations, the control of the models that were processed in the last cycle are moved back to the controller. The same is done under Song::stop(), so the last cycle models control is moved back to the controller. That removes the need to have a pointer to the controlled model in the controller object. Adds mixer model change request to avoid race condition. Co-authored-by: Dominic Clark <mrdomclark@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a bug introduced by #5657 where reloading a project and playing it could cause a Segmentation Fault crash. After some debugging, @DomClark tracked the issue to be likely a use-after-free being caused by m_oldAutomatedValues not being cleared when the project was loaded again.
This commit adds a line to clear the
m_oldAutomatedValuesmap onSong::clearProject(), which is called fromSong::loadProject().