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

MIDI mute improvements #2318

Merged
merged 22 commits into from
Mar 9, 2023
Merged

MIDI mute improvements #2318

merged 22 commits into from
Mar 9, 2023

Conversation

johnnovak
Copy link
Member

@johnnovak johnnovak commented Mar 8, 2023

Currently, the when muting the audio, all MIDI devices are sent Note Off messages, which is problematic when long-held notes are used in a composition. E.g. load up Space Quest V and toggle mute every 1-2 seconds during the logo music, and then at the start of the intro and you'll hear what I mean (you'll get missing notes after unmuting; the slower the tempo of the music is, the more annoying this problem becomes...)

This is unnecessary for built-in MIDI synthesizers, and not ideal for external devices. For internal synths, we don't need to send note offs at all; it's better to keep feeding them MIDI data and simply let the mixer handle the muting.

For external devices, we need to be a bit more clever. Instead of sending Note Offs and stopping sending the MIDI data, we keep sending the data and perform MIDI muting by setting all 16 MIDI channel volumes to zero, then unmute by restoring their current volume level. Of course, to be able to restore the volumes we need to track Channel Volume Control Change messages in the new MidiState thingy I added in my recent PR. In my view, this is a rather neat solution because we continue feeding the external device with MIDI data so the "music keeps running" and we're literally just muting it! It's very similar to the situation when your CD player keeps playing and you only mute/unmute the amplifier, but don't mess about with the CD player!

The PR also contains tons of cleanup changes. Specifically, I found the API of the MIDI module a bit too abstract which made it difficult to control things precisely (e.g. different vaguely related but ultimately different things, such as stopping/restarting/muting/resetting/initialising, were conflated into the abstract MIDI_HaltSequence / MIDI_ResumeSequence functions). I've also simplified the mixer's interface; the mixer's own internal state is no one else's business now, you can only instruct it to mute or unmute from the outside and that's it. Plus lots of other improvements, changes, fixes, etc... They all belong together, and I did not want to raise 10 small PRs...

Every commit should be individually compilable and testable, and reviewing commit by commit should make the thought process behind these changes clear.

If you're wondering how these changes affect the pause function, well, it's badly broken in main currently in a variety of interesting ways anyway... My next PR will fix all that stuff up, and I will make further tweaks so pausing and muting work together correctly as they should.

@johnnovak johnnovak marked this pull request as ready for review March 8, 2023 13:21
@johnnovak johnnovak requested a review from kcgen March 8, 2023 13:21
src/midi/midi.cpp Outdated Show resolved Hide resolved
src/midi/midi.cpp Show resolved Hide resolved
src/midi/midi.cpp Outdated Show resolved Hide resolved
src/midi/midi.cpp Outdated Show resolved Hide resolved
@kcgen kcgen added the refactoring Code refactoring without any functional changes label Mar 8, 2023
@kcgen
Copy link
Member

kcgen commented Mar 8, 2023

@johnnovak - great improvements and cleanup!

Comment here is purely function testing.

I'm using:

[sdl]
mute_when_inactive = true
pause_when_inactive = false

(I realize pause_when_inactive is in your queue, so we'll leave it false)

When I start a game, the DOSBox window opens up active however the music is muted. If I Alt+Tab to send the window to the background then the music starts playing (maybe default state is just flipped?).

If I do the same test (start a game, the DOSBox window in foreground) but instead of Alt+Tab, I press Ctrl+F8 to switch "mute on", then the log shows MIXER: Unmuted and music plays. Then, with the music playing, I Alt+Tab to background the window, then the music keeps playing. I then Alt+Tab again to bring the window to the foreground, at which point the then the music mutes.

If you can try main with the above conf settings:

  • The window should open up with music playing by default.
  • backgrounding the window should always mute (per the conf setting)
  • foregrounding the window should always yield to the user's mute-state (on/off) switch state (via Ctrl+F8 switch).

@johnnovak
Copy link
Member Author

When I start a game, the DOSBox window opens up active however the music is muted. If I Alt+Tab to send the window to the background then the music starts playing (maybe default state is just flipped?).

Yep, my bad; I flipped MIXER_Mute & MIXER_Unmute in sdlmain.cpp and haven't explicitly tested it because I was gonna address the various pause & "on inactive" issues in my next PR.

But this is a regression actually, so just pushed a fix.

@johnnovak
Copy link
Member Author

...and thanks for testing it, @kcgen 👍🏻

@kcgen
Copy link
Member

kcgen commented Mar 9, 2023

(small tweak to commit wording.. MIDI output is now always running even when muted -> Always run MIDI output, even when muted)

@johnnovak
Copy link
Member Author

(small tweak to commit wording.. MIDI output is now always running even when muted -> Always run MIDI output, even when muted)

Done! Pushed a few more small and hopefully non-controversial cleanups.

@kcgen
Copy link
Member

kcgen commented Mar 9, 2023

When I start a game, the DOSBox window opens up active however the music is muted. If I Alt+Tab to send the window to the background then the music starts playing (maybe default state is just flipped?).

Yep, my bad; I flipped MIXER_Mute & MIXER_Unmute in sdlmain.cpp and haven't explicitly tested it because I was gonna address the various pause & "on inactive" issues in my next PR.

But this is a regression actually, so just pushed a fix.

Retesting - 👍 Starts up playing now - so that's done!

For the Ctrl+F8 manual control, that state is lost across the active/inactive window transition. Repro:

  1. Start the game, confirm playing
  2. Ctrl+F8 to mute
  3. Alt+Tab send window to background
  4. Alt+Tab to bring window to foreground (music is unmuted, trumping the user's Ctrl+F8 setting)

(No worries if the plan is to include that handling in the next round!)

@kcgen
Copy link
Member

kcgen commented Mar 9, 2023

UBSAN also running clean.

@johnnovak
Copy link
Member Author

For the Ctrl+F8 manual control, that state is lost across the active/inactive window transition.

Thanks again, that was an oversight on my part. Now I understood why you needed MIXER_IsManuallyMuted! 😂

I've reinstated the correct manual muting behaviour in my last commit.

@kcgen
Copy link
Member

kcgen commented Mar 9, 2023

I've reinstated the correct manual muting behaviour in my last commit.

Confirmed and running nice.
MIDI is looking in great shape! Awesome PR, @johnnovak.

@kcgen kcgen self-requested a review March 9, 2023 03:49
@johnnovak johnnovak merged commit 7826947 into main Mar 9, 2023
@johnnovak johnnovak added the enhancement New feature or enhancement of existing features label Mar 9, 2023
@kcgen kcgen deleted the jn/mute-improvements branch March 29, 2023 17:40
@johnnovak johnnovak added audio Audio related issues or enhancements midi MIDI related features and issues labels Dec 11, 2023
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 enhancement New feature or enhancement of existing features midi MIDI related features and issues refactoring Code refactoring without any functional changes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants