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

Don't reload sample from disk when reversing #5701

Merged
merged 2 commits into from
Oct 7, 2020

Conversation

DomClark
Copy link
Member

@DomClark DomClark commented Oct 5, 2020

Currently, when reversing a sample, SampleBuffer reloads it from disk. This is unnecessary and makes it impossible to do so in realtime code. This change still isn't realtime safe, since it locks a mutex, but a proper fix requires much wider changes, and it doesn't read from the disk any more at least.

Blocks #5695.

@LmmsBot
Copy link

LmmsBot commented Oct 5, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

Linux

macOS

🤖
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://9390-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-729%2Bgdde5576be-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9390?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://9393-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-729%2Bgdde5576be-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9393?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://9391-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-729%2Bgdde5576-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9391?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://9392-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-729%2Bgdde5576be-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9392?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "eb6b8b3753cd936213ada57cfcb50aee6d7a2f6f"}

@Spekular
Copy link
Member

Spekular commented Oct 6, 2020

Code looks reasonable to me! Do you want any testing on this? Anything specific?

@Spekular
Copy link
Member

Spekular commented Oct 6, 2020

Wait, actually, doesn't this assume that every call to setReversed toggles the state? It appears to me that _on is meant to define if the sample should be reversed or not, so repeated calls to setReversed with the same argument should do nothing, right? Whereas here the sample would be reversed each time.

@DomClark
Copy link
Member Author

DomClark commented Oct 6, 2020

Good catch! I was looking at similar code by Reflexe, but didn't quite take in that he changed the function from setReversed(bool) to reverse(). Should be fixed now. (Also, m_reversed is now set while the lock is held, which should be safer.)

In terms of testing, I don't think an awful lot is required. Flip some samples back and forth in AFP, make sure they save and load the same way round.

Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Still not RT safe, but a great performance increase.

@Spekular
Copy link
Member

Spekular commented Oct 6, 2020

Did some very quick testing, everything worked as it should. I would probably have wrapped the whole function inside if (m_reversed != on) { ... } to avoid taking the locks if they're not needed, but in practice the if should always be true so it won't make any difference.

@DomClark
Copy link
Member Author

DomClark commented Oct 6, 2020

I would probably have wrapped the whole function inside if (m_reversed != on) { ... } to avoid taking the locks if they're not needed

If this function is going to be called from multiple threads, the variable should be read and written under the same lock. Suppose two threads tried to reverse the sample simultaneously - both see that it's not reversed, so both reverse it and it ends up unreversed, even though m_reversed is now true.

Anyway, this has two approvals now, so I will merge it tomorrow if there are no objections.

@Spekular
Copy link
Member

Spekular commented Oct 6, 2020

Ah, great point.

@DomClark DomClark merged commit 8c63582 into LMMS:master Oct 7, 2020
@DomClark DomClark deleted the samplebuffer-reverse branch October 7, 2020 12:38
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
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.

4 participants