[Scummvm-devel] Audio::MidiPlayer

[Max Horn]

Hi folks,

as you may have noticed, recently I added a new class Audio::MidiPlayer and converted several engines to use this new class, specifically
  agi, draci, hugo, m4, made, parallaction, saga, tinsel, touche.
Others could possibly follow, e.g. lure, groovie, queen, maybe even agos.

This may have introduced some regressions, so it would be good if people could test these engines, specifically any MIDI related stuff in them (i.e. music, and maybe sound effects in some cases).


Doing that, I noticed various differences in how these engines handle(d) MIDI playback. I already unified some of these, but not all. The problem is that it's often not clear whether an engine which does things differently than others does so for a good reason, or whether it is unimportant (or even a bug). Thus it would be good if people, esp. the people who wrote the MIDI support in the relevant engines in the first place, could take a look at these differences.

Here are some examples of differences. I hope most of them can be removed eventually.

* The default value of the _channelsVolume[] array:
In Audio::MidiPlayer, I use the default value 127 for all channels. Some engines used 0 instead. And lure uses 90, saying "90 is power on default for midi compliant devices"
Actually, it is not even clear to me whether this default value matters at all, but finding a common default value (with justification for it ;) seems desirable.

* The way MidiDriver::detectDevice is used:
  Most engines use "MDT_MIDI | MDT_ADLIB | MDT_PREFER_GM" as flags, but e.g. AGI just uses "MDT_MIDI | MDT_ADLIB". It is in general not evident why one value or another is used. We should unify this, or at least document the difference.

* In most engines, the setVolume method does nothing if the new volume matches the old volume; in some engines, it always does something. This could lead to different behavior in certain cases. 

* A few engines (agi, draci, hugo) handle newly created MidiChannel's differently, taking extra care to call the volume() method of the new channel, thus enforcing the aforementioned default channel volume. Is this really necessary? If yes, then should we introduced it for other engines, too? Otherwise, explain / document the difference.

* Handling pause / resume varies a lot: The current (arbitrary) default implementation for pause() to set _isPlaying to false, and then set the volume to 0 (actually, -1, which is clipped to 0... weird). Resume is done by setting _isPlaying back to true, and setting the volume to match the music volume specified in ConfMan.

One issue with that is that while paused, a MidiPlayer instance returns potentially "confusing" information about itself; e.g. isPlaying() returns false (which can be argued to be right or wrong, depending on what one things isPlaying() should mean), and also getVolume() returns 0.

Some engines implement pause/resume by introducing a dedicated _isPaused boolean. And these generally also act slightly different on resume(), by setting the volume to whatever it was before the pause. 
This may sometimes be "better" (if a game is temporarily modifying the music volume, independently from user settings), or worse (if the music was paused in order to let the user tweak the music volume, then upon resume, we want the midi player to use the new volume!).

Hence it is not necessarily clear which behavior is "correct"; in fact, this is likely to depend on the rest of the engine's code. Still, it would seem to me that we should pick one way, document and implement that consistently, and adapt the engines to it.


* Some engines explicitly detect and handle MT-32 type devices, some totally ignore this possibility. Of course not all engines have games which makes use of an MT-32. I wonder if these then should explicitly error out when an MT-32 type device is detected? Or use some kind of converter? In any case, it seems dangerous to just ignore the possibility.

* Most engines make use of the sendMT32Reset / sendGMReset methods, but not all.




Cheers,
Max