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

Make SampleBuffer adjust its members when resampling. Fixes #5218 for stable-1.2 #5226

Merged
merged 3 commits into from Oct 19, 2019

Conversation

@softrabbit
Copy link
Member

@softrabbit softrabbit commented Oct 6, 2019

My proposition on how to make oversampling work again in the 1.2 line after the changes in #4991. Feel free to chime in if some use case of SampleBuffer remains broken (or gets a fresh bug) after this PR.

@softrabbit softrabbit changed the title Make SampleBuffer adjust its members when resampling. Fixes #5218. Make SampleBuffer adjust its members when resampling. Fixes #5218 for stable-1.2 Oct 6, 2019
@PhysSong PhysSong requested a review from Reflexe Oct 6, 2019
@softrabbit
Copy link
Member Author

@softrabbit softrabbit commented Oct 7, 2019

Oops. This won't work, just realized I must've only tested the latest changes from command line. The loop points will be off when returning to normal sample rate after GUI export. I'll yet be refining this.

@Reflexe
Copy link
Member

@Reflexe Reflexe commented Oct 7, 2019

I have heavily refactored this class in the recording branch. Maybe it would be smarter to use that code instead (Will do that hopefully later this day).

@softrabbit
Copy link
Member Author

@softrabbit softrabbit commented Oct 7, 2019

I have heavily refactored this class in the recording branch. Maybe it would be smarter to use that code instead (Will do that hopefully later this day).

In master, no doubt. It's meant to end up there anyway.

In stable, why not if you already fixed the bug in your branch. If not, I'd be inclined to not backport and just fix. But either way works for me, you are most familiar with your code so you get to decide.

sample rate. Now this should work also after exporting from GUI.
@Reflexe
Copy link
Member

@Reflexe Reflexe commented Oct 9, 2019

Looks good, however, maybe it would be better to just avoid normalization altogether? (let play handle resampling).
That will also make sure we won't have to resample again after we finish exporting (which will may reduce quality)

@softrabbit
Copy link
Member Author

@softrabbit softrabbit commented Oct 9, 2019

Looks good, however, maybe it would be better to just avoid normalization altogether? (let play handle resampling).
That will also make sure we won't have to resample again after we finish exporting (which will may reduce quality)

I believe update() calls normalizeSampleRate() only after reloading the original data from file, so there shouldn't be a successive resampling problem.

@Reflexe
Copy link
Member

@Reflexe Reflexe commented Oct 9, 2019

believe update() calls normalizeSampleRate() only after reloading the original data from file, so there shouldn't be a successive resampling problem.

You're right. I just hate this update function so much ;)

I think it just would be better to extract (float)mixerSampleRate() / old_rate into one variable and we're done.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Oct 15, 2019

Is it okay if I(or someone else) add a commit addressing #5226 (comment) and merge this?

@softrabbit
Copy link
Member Author

@softrabbit softrabbit commented Oct 15, 2019

Is it okay if I(or someone else) add a commit addressing #5226 (comment) and merge this?

Sure. I will probably not be much around my dev box for the next few days, so go ahead.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Oct 15, 2019

Okay, but I'm not sure how to name the variable.

@Reflexe Reflexe requested a review from PhysSong Oct 18, 2019
@Reflexe Reflexe merged commit 4f11cf1 into LMMS:stable-1.2 Oct 19, 2019
2 of 3 checks passed
@tresf tresf mentioned this pull request Oct 29, 2019
27 tasks
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Nov 18, 2019

It looks like this is still an issue on drumsynth (.ds) files.
https://lmms.io/forum/viewtopic.php?f=7&t=31122

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants