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

Add Fluidsynth/FluidLite #2250

Merged
merged 15 commits into from
Aug 24, 2020
Merged

Add Fluidsynth/FluidLite #2250

merged 15 commits into from
Aug 24, 2020

Conversation

Ghabry
Copy link
Member

@Ghabry Ghabry commented Jun 17, 2020

Fix #1322
Fix #834
Fix #2029

  • TODO: Update Makefiles - Do some console tests

Depends on FS/1 because this would cause too much rebase pain otherwise.

Features:

  • Refactored MIDI Api. Is now managed by a Create function MidiDecoder (similiar how AudioDecoder works) which handles instantiation of an appropriate subclas. A GenericMidiDecoder implements most of the API already and manages a FmMidi Sequencer and an underlying MIDI library.
  • When the MIDI library supports external sequencers only FillBuffer and Midi messages must be implemented. Ticks, loops etc. are managed by GenericMidiDecoder.
  • The Api also handles libraries that lack external sequencer support (WildMidi). GenericMidiDecoder is aware of it and will call appropriate functions and do sample counting. For WildMidi this means that Seek to implement Loops will work!
  • FluidSynth/Lite support (Synth has priority in detection)
  • The Fluid synth instance is recycled for BGMs to avoid long delays when loading the soundfont
  • VIO for soundfonts through Filesystem API

TBD: Good default filenames/search locations for Soundfonts

(based on work of carstene1ns)
The FmMidi Sequencer is now mandatory and used by all libraries if applicable (all but WildMidi).
Support looping for WildMidi.
@Ghabry Ghabry added Refactor Has PR Dependencies This PR depends on another PR MIDI labels Jun 17, 2020
@Ghabry Ghabry changed the title WIP: Add Fluidsynth WIP: Add Fluidsynth/FluidLite Jun 17, 2020
The AudioDecoder will destroy it because it is stuck at "IsFinished" but the BGM must be kept alive to match RPG_RT.

Fix EasyRPG#834
@fmatthew5876
Copy link
Contributor

TBD: Good default filenames/search locations for Soundfonts

We may want to punt on this question until we tackle engine configuration more generally in the config PRs sequence. The midi engine and soundfont are definitely a big part of that.

I'd be fine with some env var or command line parameter hack for now. Maybe also some hard coded default to search like RPG_RT.sf[23] in the game directory for game devs. I wouldn't want this important PR to get bogged down in that somewhat orthogonal discussion.

@Ghabry Ghabry marked this pull request as ready for review August 16, 2020 11:39
@Ghabry Ghabry removed the Has PR Dependencies This PR depends on another PR label Aug 16, 2020
@Ghabry Ghabry added the Audio label Aug 16, 2020
@Ghabry
Copy link
Member Author

Ghabry commented Aug 16, 2020

With FS1 merged this is now open for review.

Right now the soundfont is hardcoded to "easyrpg.soundfont", will keep it this way for now.

@Ghabry Ghabry changed the title WIP: Add Fluidsynth/FluidLite Add Fluidsynth/FluidLite Aug 16, 2020
@fdelapena fdelapena requested review from fmatthew5876 and carstene1ns and removed request for fmatthew5876 August 16, 2020 22:07
src/audio_midi.h Show resolved Hide resolved
src/decoder_fluidsynth.cpp Show resolved Hide resolved
@fmatthew5876
Copy link
Contributor

Other than the minor nit, looks ok to me.

@fdelapena fdelapena added this to the 0.6.3 milestone Aug 20, 2020
@Ghabry
Copy link
Member Author

Ghabry commented Aug 22, 2020

@BSzili for you information.
I didn't touch the Amiga Makefile. You are responsible for adding FluidLite or FluidSynth - I recommend FluidLite - to your Makefile if you want.

@Ghabry Ghabry force-pushed the fluidsynth branch 2 times, most recently from 1803d23 to 6402452 Compare August 22, 2020 12:42
@elsemieni
Copy link
Member

Windowsx32 build triggers an assertion failure when a melody without ev111 finishes playing. Unable to obtain a dump of this, but I will post an screenshot just in case.
image

@Ghabry
Copy link
Member Author

Ghabry commented Aug 22, 2020

There is only one location where this can happen, will take a look. thanks :)

@Ghabry
Copy link
Member Author

Ghabry commented Aug 22, 2020

Problems (via IRC):

  • Because I recycle the same FluidSynth instance old notes are playing when having a MIDI on the title screen, then returning to the Game Browser, then starting a game. Other cases that likely fail: Midi -> BGM Stop -> BGM Play other Midi.
  • Pitch doesn't work correctly

@BSzili
Copy link
Contributor

BSzili commented Aug 23, 2020

@BSzili for you information.
I didn't touch the Amiga Makefile. You are responsible for adding FluidLite or FluidSynth - I recommend FluidLite - to your Makefile if you want.

That's fine, I'll update them when the next version is due. Thanks for the tip about FluidLite!

src/audio_midi.h Outdated
#include "audio_decoder.h"
#include "midisequencer.h"

#define EP_MIDI_FREQ 22050
Copy link
Member

@elsemieni elsemieni Aug 24, 2020

Choose a reason for hiding this comment

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

Watch out. I know 22050 is less process usage but this preset (at least in my windows x86) is making melodies sounds a bit "uncoordinated" in difference with 44100.

Here's a little example with Battle3.mid and scc1t2 soundfont (sorry, github doesn't like audios =( )
With 44100:
https://drive.google.com/file/d/1ODdgq7aU0tljQwopjRmnyjY-p2kokUv_/view?usp=sharing
With 22050:
https://drive.google.com/file/d/1ti2VP5uFELNgIE7TgTlJJiJuPeV-VO1P/view?usp=sharing

Copy link
Contributor

@fmatthew5876 fmatthew5876 Aug 24, 2020

Choose a reason for hiding this comment

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

Would like to know how much less CPU usage really is this? Especially on desktop and mobile platforms?

I'm all for saving CPU cycles but 22khz is sacrificing audio quality in a big way. I would prefer to default to "reasonable" audio quality that "sounds like RPG_RT or better" and only cut back if we find we really need to on limited hardware.

Also on modern systems, using low frequencies like this for music is very uncommon and so downstream software / OS audio stacks may up-sample it poorly.

For all these conjectures about processing costs of audio features, we really need some benchmarks and performance numbers. Some of them could be costing much less or much more CPU than we think.

That being said, I also really want to see some initial version of this merged to master so I can being playing with it while testing other PRs. So I'm fine collecting the outstanding questions into a github issue, getting this into a base working state, and merging it.

@elsemieni
Copy link
Member

Also a little observation with RTP2003 MIDIs.
Most (if not all) RTP2003 MIDI's uses these commands in the beginning of each track:

image

In channel 10 (drums) this causes troubles and FluidLite fallbacks to a non-drum instrument. If you manually remove these events from MIDI, melodies play normally.

I don't know if it's related with FluidLite directly (I don't have other software reference who uses FluidLite to test), or with the interface that "connects" FluidLite and player (at least I tested the PR without soundfont and works fine).

@Ghabry
Copy link
Member Author

Ghabry commented Aug 24, 2020

I will only:

  • Recheck pitch
  • undo the 22050 change
  • Apply fdelas suggestions to fix missing Notes

Then this is ready.

The stuck instruments when doing BGM Stop -> BGM play require some work I would postbone to another PR. They only happen in Fluid, not in Wildmidi or FmMidi.

@Ghabry
Copy link
Member Author

Ghabry commented Aug 24, 2020

I'm done here. I was able to fix the incorrectly pitched Midi. This was a design issue in the Resampler (the pitch stuff was unused for years) which explains why FmMidi always had the wrong pitch. Midis play now correctly for FmMidi and FluidLite/Synth.

Can't be fixed in WildMidi because it is not Midi Message based. (at least I was able to implement Midi ticks through tricks here)

I postbone the following three bugs:

  • Investigate wrong CC command
  • Stuck instruments when BGM_Stop -> BGM_Play
  • Further performance investigations

@fmatthew5876
Copy link
Contributor

Sounds good to me, lets get those items into a github issue so we don't forget them

@fdelapena fdelapena merged commit b0a58a9 into EasyRPG:master Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants