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

Revert clamping frames_needed to the allowed mix/max range in the mixer #3217

Merged
merged 1 commit into from Dec 16, 2023

Conversation

johnnovak
Copy link
Member

@johnnovak johnnovak commented Dec 16, 2023

Description

This fixes a regression by reverting commit a25af54. The clamping causes the audio to become out of sync when using fast-forward mode in release mode (debug mode is not fast enough, but you can still get the audio out-of-sync by fast forwarding, but to a much lesser extent). Tested with OPL and MT-32 output, but probably all sound devices are affected.

The lack of clamping doesn't cause issues in release builds; it just helped not to trip an assert in debug builds when putting your laptop to sleep while running Staging, then waking the machine up a few seconds later. But this was never a proper fix anyway, just a workaround, so it's no great loss.

The problem should be properly addressed when the whole mixer gets refactored. I really don't have the time for that, and we definitely don't want to break the sound sync when fast-forwarding.

Related issues

Reverts #2843

Manual testing

Before fix

  1. Start Space Quest 3 with OPL or MT-32 sound (but any game will do).
  2. Press fast-forward for a few seconds.
  3. When you release the fast-forward hotkey, the audio will "rush" to catch up with the "current time". This sounds quite comical and quite wrong 😄

After fix

Repeat the same steps—the audio always stays in perfect sync, no matter for how long you've been fast-forwarding.

Checklist

Please tick the items as you have addressed them. Don't remove items; leave the ones that are not applicable unchecked.

I have:

  • followed the project's contributing guidelines and code of conduct.
  • performed a self-review of my code.
  • commented on the particularly hard-to-understand areas of my code.
  • split my work into well-defined, bisectable commits, and I named my commits well.
  • applied the appropriate labels (bug, enhancement, refactoring, documentation, etc.)
  • checked that all my commits can be built.
  • confirmed that my code does not cause performance regressions (e.g., by running the Quake benchmark).
  • added unit tests where applicable to prove the correctness of my code and to avoid future regressions.
  • made corresponding changes to the documentation or the website according to the documentation guidelines.
  • locally verified my website or documentation changes.

@johnnovak johnnovak added regression We broke something 😊 audio Audio related issues or enhancements labels Dec 16, 2023
@johnnovak johnnovak self-assigned this Dec 16, 2023
@johnnovak johnnovak force-pushed the jn/revert-clamp-mixer-frames-needed branch from 8394d31 to 8195543 Compare December 16, 2023 04:10
…ixer

This reverts commit a25af54 because the clamping causes the audio to
become out of sync when using fast-forward mode in release mode. Tested
with OPL and MT-32 output, but probably all sound devices are affected.

The lack of clamping doesn't cause issues in release builds, it just
helped not to trip an assert in debug builds when putting your laptop to
sleep while running Staging, then waking the machine up. But this was
never a proper fix anyway, just a workaround, so it's no great loss.

The problem should be properly addressed when the whole mixer gets
refactored.
@johnnovak johnnovak force-pushed the jn/revert-clamp-mixer-frames-needed branch from 8195543 to 7188fe0 Compare December 16, 2023 04:11
@johnnovak johnnovak changed the title Always clamp 'frames_needed' to the allowed mix/max range in the mixer Always clamp frames_needed to the allowed mix/max range in the mixer Dec 16, 2023
@kcgen kcgen self-requested a review December 16, 2023 04:35
Copy link
Member

@kcgen kcgen left a comment

Choose a reason for hiding this comment

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

Nice mitigation, @johnnovak !
This is perfect because if/when the mixer is refactored more, the clamps can be inspected and checked - until we figure out the root cause for overages (or at least, limit the overage right at the source).

@johnnovak johnnovak merged commit 943f4ae into main Dec 16, 2023
50 checks passed
@johnnovak johnnovak changed the title Always clamp frames_needed to the allowed mix/max range in the mixer Revert clamping frames_needed to the allowed mix/max range in the mixer Dec 17, 2023
@johnnovak johnnovak deleted the jn/revert-clamp-mixer-frames-needed branch January 20, 2024 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audio Audio related issues or enhancements regression We broke something 😊
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants