#241 Introduce optional sysex delay to MIDI (MT-32 rev00 fix)
This patch introduces an optional delay to work around the buffer overflow issues seen on first generation MT-32 devices. The length of the delay is calculated based on the size of the sysex payload plus a configurable constant number of milliseconds (based on code from ScummVM which always adds this delay although the constant is only applied for MT-32 devices: http://scummvm.svn.sourceforge.net/viewvc/scummvm?revision=47336&view=revision ).
Rather than introduce the delay when the sysex message is first sent, the time at which it is "safe" to send the next MIDI command is stored, and the delay is before the next MIDI command is processed, allowing any other processing the game may be doing in the meantime to continue (some games may already be introducing a delay to work around this issue and this method avoids introducing a double delay). In my testing I often saw individual delays of < 10 ms introduced, while fixing all problems I was encountering.
I've tested this on Mac OS X 10.6.6 using a real MT-32 rev00 connected via an M-Audio USB interface.
I tested using the following games:
- Sword of the Samurai (worked fine before adding delay)
- Worlds of Ultima: Savage Empire (introduction sometimes worked without delay, but always caused error when starting actual game and "Initializing Roland..." was visible on screen)
- Prince of Persia (failed without delay)
- Prince of Persia 2 (failed without delay)
- Circuit's Edge (good for seeing impact of delay, as sysex payload is sent whenever you enter/exit a location)
I also tested with the samples supplied in this thread: http://vogons.zetafleet.com/viewtopic.php?t=15761
Hi Jason,
I just noticed your patch. While
reading your description, I noticed you mention another
Vogons thread in which I also posted a patch, similar to
yours. I was wondering if we should maybe combine forces :)
While in the basis pretty much similar, I noticed the
calculated delay you are using is
((midi.sysex.used+2)*1000/3125) + midi.sysexDelayExtra
While my patch has
midi.sysex.used / 40.0 * 16.7
Could you explain your calculation? Did you maybe find any
documentation about the required delays? I found that some
particular sysex commands require a longer delay, so my
patch is more complicated, but it was all trial and error,
not based on documented MT-32 behavior :-/
Kindest regards,
Ronald (rcblanke)
The calculation I used came from this ScummVM change set, which implies that there's something more that guesswork behind it, but I couldn't track down an explanation for what that was:
http://scummvm.svn.sourceforge.net/viewvc/scummvm?revision=47336&view=revision
You'll note that ScummVM always uses the calculated delay, MT-32 or not, while the additional 40 ms constant delay is only added if it thinks it's using a real MT-32.
After that I started testing it with my most problematic test cases to ensure that delay worked for me in those cases (it did), but I noticed some very noticeable slowdowns (Circuit's Edge was very noticeable when exiting to the street). These I solved by moving the delay to only be run if the delay window hadn't already been exceeded by the time the next MIDI command was sent. It didn't seem sensible to block other non-MIDI processing from occurring if possible.
I saw mention of your patch in that thread, but was unable to find a link to the patch itself or I would have tested that first. I assumed (perhaps wrongly so) that you'd removed or withdrawn it for some reason. I'd be interested to know if my patch solves the problem for you on Windows as well.
It works partly on my system. POP1 and 2 work fine with your patch, but many others still show buffer overflows. Please see the end of the Vogons thread for the games I tested on; for example, Speedball 2 is picky, and Prophecy the Viking Child, Lemmings 2 and Dark Sun are good test cases.
I plugged "MIDI" and "3125" into Google, and I see that the specified baud rate for MIDI is 31250, which explains where that calculation came from (and why it's always used by ScummVM, not just for working around the MT-32 issue).
I'm pretty sure I have Dark Sun, if I can find my discs. I'll try and take a look.
Also, I didn't spell it out in my initial patch description, but in my patch this feature is controlled by the use of two new parameters in the [midi] section:
sysexdelayenable (bool): defaults to 'false', set to 'true' to enable MIDI delay.
sysexdelayextra (int): defaults to '40'. Constant value to be applied to calculated delay timeout. You can try increasing this to deal with specific issues (or theoretically setting it to '0' for a later rev MT-32). I wanted this to be configurable, as it seems variations in hardware (different USB<->MIDI interfaces) may need some fine tuning.
Ok, makes sense. I tried my patch with your sysexlength-calculation (I left the command-specific exceptions in). Most games work, but some still fail (Colonization and Fire&Ice), unfortunately.
For reference: my patch is available at http://vogons.zetafleet.com/viewtopic.php?p=210606#210586
OK, so I've looked over your patch. I have a few comments:
I don't know enough about the receive ready stuff to comment.
Perhaps it would be worth while to collect a set of midi files using the DosBox capture feature that are known to cause the issue.
If you increase the extra delay (using my patch) does it fix Colonization for you? I can't get it, or Dark Sun, to fail with the defaults, but maybe tweaking this would help you:
Hello again jason,
True, I did that because I assumed different bahavior amonst different OS' (note the wait lock code in the windows playsysex implementation).
But the lock doesn't really seem to wait (much) so your approach may be better, indeed.
I didn't notice any problems with my approach, but your statement seems sound.
The idea here is that the game itself checks for the return value of MPU401_ReadStatus() to check
if the midi device that is being used is ready to receive the next midi command. Many games seem to work
with this status bit solution, but others do not. And it doesn't really seem to give additional benefits
or anything, while complicating things for the end-user.
I can help you with that, but what would you do with them?
I can get everything working using "sysexdelayextra=85" but that leads to excessively long loading times. And games like X-Wing load sysex messages while playing music,
with the effect that music is interrupted because of the sysexes.
Stuff gets much better when I add the
if (midi.sysex.buf[5] == 0x7F) {
delay = 290; // All Parameters reset
}
case. Using this fixed delay for the "All Parameters reset" sysex command, I can use your patch successfully with "sysexdelayextra=10". Well, almost succesfully.
Viking Child is problematic, and requires "sysexdelayextra=65" still. All in all, I keep returning to the 'special case' delays in my original patch to achieve
minimum loading times while still preventing buffer overflows.
I've altered my patch somewhat, maybe you can check it out. I removed the delayreadystate solution, as it didn't provide any benefits. Moved some stuff around based
on your implementation. And used a calculated delay based on your code, but without an optional extra delay.
The DOSBox authors do not fancy new config options, that's why it would be better to use the existing [midi]config option. And if we can prevent the necessity to configure
any configurable extra delays, that would be neat.
This new patch works for all games in the list mentioned in the Vogon's thread.
Kindest regards,
Ronald
Index: src/gui/midi.cpp
--- src/gui/midi.cpp (revision 3674)
+++ src/gui/midi.cpp (working copy)
@@ -19,6 +19,8 @@
#include <assert.h>
#include <string.h>
#include <stdlib.h>
+#include <sdl.h>
+#include <timer.h>
#include "dosbox.h"
#include "cross.h"
@@ -107,12 +109,19 @@
struct {
Bit8u buf[SYSEX_SIZE];
Bitu used;
+ Bitu delay;
+ Bit32u start;
} sysex;
bool available;
MidiHandler * handler;
} midi;
void MIDI_RawOutByte(Bit8u data) {
+ if (midi.sysex.start) {
+ Bit32u passed_ticks = GetTicks() - midi.sysex.start;
+ if (passed_ticks < midi.sysex.delay) SDL_Delay(midi.sysex.delay - passed_ticks);
+ }
+
/ Test for a realtime MIDI message /
if (data>=0xf8) {
midi.rt_buf[0]=data;
@@ -126,7 +135,27 @@
return;
} else {
midi.sysex.buf[midi.sysex.used++]=0xf7;
- midi.handler->PlaySysex(midi.sysex.buf,midi.sysex.used);
+
+ if ((midi.sysex.start) && (midi.sysex.used >= 4) && (midi.sysex.used <= 9) && (midi.sysex.buf[1] == 0x41) && (midi.sysex.buf[3] == 0x16)) {
+ LOG_MSG("MIDI:Skipping invalid MT-32 SysEx midi message (too short to contain a checksum)");
+ } else {
+ LOG(LOG_ALL,LOG_NORMAL)("Play sysex; address:%02X %02X %02X, length:%4d, delay:%3d", midi.sysex.buf[5], midi.sysex.buf[6], midi.sysex.buf[7], midi.sysex.used, midi.sysex.delay);
+ midi.handler->PlaySysex(midi.sysex.buf, midi.sysex.used);
+ if (midi.sysex.start) {
+ midi.sysex.delay = (Bitu)(((float)(midi.sysex.used) * 1.25f) * 1000.0f / 3125.0f) + 2;
+ if (midi.sysex.buf[5] == 0x7F) {
+ midi.sysex.delay = 290; // All Parameters reset
+ }
+ if (midi.sysex.buf[5] == 0x10 && midi.sysex.buf[6] == 0x00 && midi.sysex.buf[7] == 0x04) {
+ midi.sysex.delay = 145; // Viking Child
+ }
+ if (midi.sysex.buf[5] == 0x10 && midi.sysex.buf[6] == 0x00 && midi.sysex.buf[7] == 0x01) {
+ midi.sysex.delay = 30; // Dark Sun 1
+ }
+ midi.sysex.start = GetTicks();
+ }
+ }
+
LOG(LOG_ALL,LOG_NORMAL)("Sysex message size %d",midi.sysex.used);
if (CaptureState & CAPTURE_MIDI) {
CAPTURE_AddMidi( true, midi.sysex.used-1, &midi.sysex.buf[1]);
@@ -163,10 +192,19 @@
MIDI(Section configuration):Module_base(configuration){
Section_prop * section=static_cast<Section_prop *="">(configuration);
const char * dev=section->Get_string("mididevice");
- const char * conf=section->Get_string("midiconfig");
+ std::string fullconf=section->Get_string("midiconfig");
/ If device = "default" go for first handler that works */
MidiHandler * handler;
// MAPPER_AddHandler(MIDI_SaveRawEvent,MK_f8,MMOD1|MMOD2,"caprawmidi","Cap MIDI");
+ midi.sysex.delay = 0;
+ midi.sysex.start = 0;
+ if (fullconf.find("delaysysex") != std::string::npos) {
+ midi.sysex.start = GetTicks();
+ fullconf.erase(fullconf.find("delaysysex"));
+ LOG_MSG("MIDI:Using delayed SysEx processing");
+ }
+ remove(fullconf.begin(), fullconf.end(), ' ');
+ const char * conf = fullconf.c_str();
midi.status=0x00;
midi.cmd_pos=0;
midi.cmd_len=0;
Index: src/dosbox.cpp
===================================================================
--- src/dosbox.cpp (revision 3674)
+++ src/dosbox.cpp (working copy)
@@ -480,6 +480,8 @@
" See the README/Manual for more details.");
#if C_DEBUG
I've also tested this latest sysex6.patch on a brand new macbook air using the same USB midi interface (Cakewalk UM-1G) and it worked flawlessly.
Ronald
The ESI RoMI/O II midi interface also works fine with the patch in OSX.
Ronald
I finally got around to testing Ronald's patch this weekend with very good results. My initial set of test all passed, and without the lag introduced to Circuit's Edge.
My one question is on the section that skips over messages that are "too short to contain a checksum". Are this messages invalid for all MIDI devices, or just the MT-32? Apologies if I misunderstood something there.
Hi Jason,
Thanks for looking into the patch, and nice to see that you got good results!
About the minimum size of the sysexes: I used http://www.2writers.com/eddie/TutSysEx.htm and http://www.xs4all.nl/~hanwen/personal/synth/roland_supp_notes.html for information. The minimum size is at least appropriate for MT-32 (which we know is being used when the [midi]midiconfig delaysysex was set) but may also apply to other devices, I've no idea really. The minimum size may be even a bit larger (11 bytes judging by the information from the aforementioned websites) but I'm not sure. The check at least prevents some invalid sysexes from being executed in a couple of games (at least Mortal Kombat 1). Prince of Persia 1 sends very short sysexes for a device other than (midi.sysex.buf[1] == 0x41) && (midi.sysex.buf[3] == 0x16) which does not result in checksum problems on my MT-32, that's why I added this additional condition.
Kindest regards,
Ronald
updated patch from rcblanke
Makes sense. I guess I was somewhat leery of assuming no other devices would use the sysexdelay functionality since it seems to be compensating for the documented MIDI Baud rate. But maybe the MT-32 is the only known device that has issues when the data-rate exceeds the MIDI Baud rate.
Attaching your patch (sysex6.patch).
Added a slightly modified version to the SVN (changed the delay if if if to if else if else if else delay; sdl.h> to "SDL.h" as is common in all dosbox code)
Thanks Jason and Ronald.