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

Expected XG System ON sysex message is wrong #1092

Closed
jjazzboss opened this issue May 2, 2022 · 8 comments · Fixed by #1171 or #1195
Closed

Expected XG System ON sysex message is wrong #1092

jjazzboss opened this issue May 2, 2022 · 8 comments · Fixed by #1171 or #1195
Labels

Comments

@jjazzboss
Copy link
Contributor

jjazzboss commented May 2, 2022

XG specs page 42 says XG System Mode ON sysex depends on the deviceId n coded on 4 bits:
F0H, 43H, 1nH (0001nnnn), 4CH, ...

That's why everywhere on the web the XG System ON sequence is described as F0H, 43H, 10H, 4CH, ..., assuming that most of the time deviceId is 0.

However in method fluid_synth_sysex() in fluid_synth_.c, the data[1] comparison is directly done on the deviceId value:

if(data[0] == MIDI_SYSEX_MANUF_YAMAHA
            && (data[1] == synth->device_id || data[1] == MIDI_SYSEX_DEVICE_ID_ALL)
            && data[2] == MIDI_SYSEX_XG_ID)

I understand it's consistent with this doc, but doing so it ignores the standard XG initialization sequence found in most of the XG midi files out there.

@jjazzboss jjazzboss added the bug label May 2, 2022
@jjazzboss
Copy link
Contributor Author

jjazzboss commented May 2, 2022

An easy improvement would be to update the doc of fluid_synth_sysex() to detail the expected XG ON sysex sequence, which is not 100% standard. A workaround for users wanting to be able to react to the "standard XG ON sequence" is to set their synth.device-id=16.

@derselbst
Copy link
Member

Ok, I found some time looking into this and proposed a fix in #1171, pls. have a look. @kode54 originally implemented this XG sysex handling, he might be interested as well.

@derselbst
Copy link
Member

Will be fixed in 2.3.1

@ReinholdH
Copy link

ReinholdH commented Nov 29, 2022

I took the liberty to add the fix for Attempt to fix XG System ON sysex message parsing #1171
bc8ddc1
in the code. Indeed MIDI XG files are properly detected as XG files now.

But this change has a wide consequence. There are lots of MIDI XG files out there which use channel 10 for drums but have MSB == 0 or <= 120.

In fluid_chan.c

   if(style == FLUID_BANK_STYLE_XG)
    {
        /* XG bank, do drum-channel auto-switch */
        /* The number "120" was based on several keyboards having drums at 120 - 127,
           reference: https://lists.nongnu.org/archive/html/fluid-dev/2011-02/msg00003.html */
        chan->channel_type = (120 <= bankmsb) ? CHANNEL_TYPE_DRUM : CHANNEL_TYPE_MELODIC;
        return;
    }

with the correct detection of the XG format by the change in #1171 and MSB <= 120, the default CHANNEL_TYPE_DRUM is changed to CHANNEL_TYPE_MELODIC.

The consequence is that before the fix of #1171 a XG format was NOT correctly detected. The drums have been used from the soundfont bank 128 and have sounded properly as drums. With the fix #1171 the XG format is properly detected but with MSB == 0, the drum channel is identified as CHANNEL_TYPE_MELODIC and the drums sound as piano.

One can debate whether the XG format was not correctly used with MSB == 0 in those MIDI files, but it is what it is. XG files with MSB <= 120 exist. Before #1171 the drums of certain MIDI files sounded as drums. With #1171 all of a sudden the drum tracks sound as piano. Usually, Yamaha keyboard owners are familiar that MSB has to be == 127, but people who just play MIDI files on PCs are not.

I am attaching such a MIDI file in the zip folder where the drum track is with MSB == 0.

Before #1171 becomes officially available the consequences should be clear that the drum track of certain MIDI files won't sound correctly any longer and potentially need a correction, or the /* XG bank, do drum-channel auto-switch */ logic in fluid_chan.c needs to be re-considered.

Oh_When_The_Saints_Go_Marching_In.zip

@derselbst derselbst reopened this Nov 29, 2022
@derselbst
Copy link
Member

Thanks @ReinholdH, I'll try to look into this in the upcoming days. @jjazzboss When you came across this issue, did you acutally had a MIDI that did not behave correctly or were you just "digging" in the specs? And if you have a MIDI which did not behave correcly before the fix in #1171 but now does, would you be able to share that MIDI as well? (that would potentially help to understand the issue and make a design decision here)

@jjazzboss
Copy link
Contributor Author

jjazzboss commented Nov 29, 2022

@derselbst I'm not familiar with XG Midi files, I ran into the issue while trying to control FluidSynth from my JJazzLab app in XG mode.

I checked the attached Midi file from @ReinholdH: it has first a GM System ON sysex message, then a XG System ON sysex. And because all tracks use BankSelect MSB=0 and LSB=0, indeed it makes fluidsynth believe all tracks are melodic.

I was curious so I Googled "XG midi files" and randomly downloaded 10 files on different web sites: all files were XG-correct, ie they used MSB=127 for the drums channel. But maybe I was lucky...

One solution could be to add a setting for the fluidsynth Midi player which says "treat the Midi file as GM", i.e. ignore the (non-GM) sysex messages and bank select messages. This way users could get the same behavior as before introducing #1171.

@derselbst
Copy link
Member

I had some time to look into this and can basically confirm what Jerome already said. However, I'm not in favor of introducing yet another setting for controlling this tiny aspect. Instead I would fallback to Jermoe's suggestion in his second comment that people are advised to set deviceID to 16 (or whatever is used in their SysEx initialization sequence). Hope that's ok for you guys.

@ReinholdH
Copy link

Ok, we can live with that.
Meanwhile we added a code change in our app that for an XG file a drum channel's MSB=0 is changed to MSB=127. This change comes with our next version when we will use fluidsynth 2.3.1 or higher.
The discussion here was very fruitful. Thx.

derselbst added a commit that referenced this issue Dec 20, 2022
This reverts commit 4bd635e as it
breaks many MIDI files, see discussion in #1092.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants