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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

MIDI: native midiout system #2302

Merged
merged 29 commits into from Jul 19, 2021
Merged

Conversation

tyrone-sudeium
Copy link
Member

@tyrone-sudeium tyrone-sudeium commented Aug 25, 2020

Todo

  • Readd checks whether native API was initialized
  • Ordering of Midi: FluidSynth, WildMidi, Native, FmMidi
  • Fix WildMidi
  • Resampler does not forward certain api calls 馃憥

What do we get?

  • RPG_RT-like looping, fading, and tempo support with the native synthesizer.
  • Also attempted to implement the "skip silence at start" behaviour from RPG_RT. I've tested it to work OK with RTP2000, and some MIDI tracks from some games I have on hand, such as Legion Saga III and my own private games.
  • Lets us ditch SDL_mixer 2 and all of its problems. We mainly used it for its ability to utilise the OS MIDI synthesizer, this PR lets us do that without it.

Known problems / discussion points (all resolved by now)

Duplicated code

There's a lot of very similar code between GenericMidiOut and GenericMidiDecoder. Both these features were developed in parallel, and I was rapidly changing the structure of my work at the same time that work on GenericMidiDecoder was changing. They do pretty different things but it should still be possible to factor out a common base class, or to have some kind of "MIDI Sequencer Controller" class that both of these utilise. It's also possible that most of the code in these could live directly in the MIDI sequencer itself.

Off by default

Currently, this PR shouldn't change behaviour of a production build, which will still use SDL_mixer 2 where it can, thus disabling the midi out. If you want to try out this stuff, it automatically becomes active when you disable SDL_mixer 2, so just set PLAYER_WITH_SDL2_MIXER to False and it should become active, if you're on a supported OS. Speaking of supported OS...

Mac and Windows only

I've written midiout device implementations for Windows and macOS. It should theoretically be possible for iOS, Linux, and libretro, but I haven't written these myself.

Ticks

There's also quite a lot of duplicated code to support ticks. Honestly this is just my C++ skills letting me down. I also don't completely understand how RPG Maker's GetTicks works and don't have any games that use it. See the MidiTempoData struct and related nastiness in audio_midiout.cpp, which is suspiciously similar to stuff in decoder_midigeneric.[h,cpp].

src/audio_sdl.cpp Outdated Show resolved Hide resolved
src/audio_generic.cpp Outdated Show resolved Hide resolved
@fdelapena fdelapena added the MIDI label Aug 25, 2020
@fdelapena fdelapena added this to the 0.6.3 milestone Aug 25, 2020
src/audio_generic.cpp Outdated Show resolved Hide resolved
src/audio_midiout.cpp Outdated Show resolved Hide resolved
src/midisequencer.cpp Outdated Show resolved Hide resolved
src/midisequencer.cpp Outdated Show resolved Hide resolved
src/audio_sdl.h Outdated Show resolved Hide resolved
src/audio_midiout.h Outdated Show resolved Hide resolved
src/audio_midiout.cpp Outdated Show resolved Hide resolved
src/audio_midiout.h Outdated Show resolved Hide resolved
src/audio_midiout.cpp Outdated Show resolved Hide resolved
src/audio_generic.cpp Outdated Show resolved Hide resolved
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Aug 25, 2020

We should think about if it's possible for MidiOut to just inherit from AudioDecoder. Then a lot of my review nits go away as the audo_generic.cpp code mostly won't change.

@Ghabry
Copy link
Member

Ghabry commented Aug 25, 2020

Thanks for opening the PR. Will think about ways to unify the logic but your code is a good base 馃憤 (and there shouldn't be conflicts for a while as my audio work is finished)

To explain the std::thread and std::mutex situation: They are broken on 3ds, Wii, emscripten and (I think?) Vita. So for the non-major targets only the Switch implements them. The good news is here that on the slow platforms (emscripten can't support it because SharedArrayBuffers are off for spectre mitigations) this isn't useful anyway. Sooo tif you want to use std::thread the realtime MIDI stuff must be properly abstracted to avoid a ifdef-mess in AudioGeneric.

The motivation for spawning a thread here is that you need a resolution of 1ms. With my FillBuffer approach I use for the Midi Decoders the resolution is the buffer size, which is ~4ms. So for fast MIDI tracks the notes are off by up to 3 ms. This PR fixes this for the "real Midi devices" but it would be also very useful for the softsynths FluidSynth and FmMidi (not useful for WildMidi but wouldn't hurt - is unaffected by this issue).

@Ghabry
Copy link
Member

Ghabry commented Aug 25, 2020

When we agree on the general implementation and style here the next steps are:

  1. Decide how to do threading best
  2. "Merge" MidiOut and AudioDecoder (common base class) to reduce code duplication. Imo the only real difference is "Update" vs "FillBuffer", otherwise the interface is almost the same. I could do this as I'm more familiar with the codebase.

@Ghabry Ghabry added the Audio label Aug 25, 2020
@tyrone-sudeium tyrone-sudeium force-pushed the native-synthesizers branch 2 times, most recently from a00b8de to 0fd46b8 Compare August 26, 2020 00:06
@tyrone-sudeium
Copy link
Member Author

One thing I haven't handled yet is: If there's an easyrpg.soundfont present, I would assume we should make the highest priority MIDI renderer Fluidlite, right? At the moment on Windows and macOS, if we don't compile sdl_mixer, it'll always use MidiOut. I think eventually we'd want to deprecate sdl_mixer entirely so we'll need to handle this. Does this order of precedence sound reasonable?

  1. Fluidlite (if easyrpg.soundfont available, or otherwise configured)
  2. MidiOut (if on supported platform)
  3. WildMidi (if patches available)
  4. FmMidi (last resort)

@Ghabry
Copy link
Member

Ghabry commented Jul 15, 2021

There was a bug in the silence skipping when there was tempo-data encountered. This broke the tick and the sample calculation.

Also WildMidi learned now how to skip initial silence and I can confirm that the looping works (again) 馃憤

@tyrone-sudeium
Copy link
Member Author

I learned something fairly cursed today about win32 midi out. According to the documentation, whether midiOutLongMsg is synchronous or asynchronous is apparently left up to the MIDI driver for the current device, and if it's asynchronous, then what I'm doing here (calling midiOutUnprepareHeader immediately after midiOutLongMsg) will always result in a failure and small memory leak. When it's asynchronous you have to poll the midiOutUnprepareHeader call until it results in MMSYSERR_NOERROR 馃う.

No idea how relevant this is in 2021, since I'm reasonably sure these days you can't even provide your own MIDI drivers any more, everything goes through Microsoft's built-ins now, and the behaviour in Windows 10 is these calls act synchronous.

@Ghabry
Copy link
Member

Ghabry commented Jul 16, 2021

More to fix: Fluidsynth changed the function parameter in there VIO interface from long to fluid_long_long_t

Add WildMidi fast-path and fix bug introduced in the WildMidi silence skip.
Address review comments
@Ghabry
Copy link
Member

Ghabry commented Jul 17, 2021

no idea how I calculated the 512 for the samples. This number is totally wrong. It must be around 64 for ~1.5 ms. Fixes this now.

@Ghabry
Copy link
Member

Ghabry commented Jul 17, 2021

This should finally address everything...

鈥l_synth unique_ptr is depending on the compiler.

Add a shutdown flag to prevent accessing a deleted pointer on Windows
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@carstene1ns carstene1ns left a comment

Choose a reason for hiding this comment

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

Works for me.

@Ghabry
Copy link
Member

Ghabry commented Jul 19, 2021

Jenkins: test this please

@carstene1ns
Copy link
Member

Unfortunately 3a19bb6 was not enough, there are still traces of SDL_mixer around. Is only dead code, since the HAVE_SDL_MIXER definition should be missing, however, should still be deleted.

https://github.com/EasyRPG/Player/search?q=have_sdl_mixer

@carstene1ns
Copy link
Member

Another thing broken: fmmidi fallback mode (autotools build). Will prepare a patch for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants