[Scummvm-devel] Audio::MidiPlayer

[Filippos Karapetis]

On Mon, Mar 28, 2011 at 7:15 PM, Max Horn <max at quendi.de> wrote:

> 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.
>

SCI should follow too, by the looks of it

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.
>

I suppose that a standard median starting value should make sense to make as
default for all engines here.


> * 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.
>

That's because AGI games did not support GM, so that difference is to be
expected


> * 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.
>

Actually, it would be good if there was a standard setVolume method, which
could be overriden by each engine in case of specialized values.

* 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.
>

I noticed that one too and found it odd myself :/


> * 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.
>

Agreed, this should be a bit standardized...


> * 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.
>

Perhaps we could fall back to Adlib in these cases?

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

Is this the responsibility of the engines? Shouldn't this be handled at the
MIDI parser level?

Regards
Filippos


-- 
"Experience is the name every one gives to their mistakes" - Oscar Wilde
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scummvm.org/pipermail/scummvm-devel/attachments/20110328/d0bfaf68/attachment.html>