Add a flag to struct:
_fluid_channel_t
to indicate it is a drum channel, using int type, with value:
0: melodic channel
1: drum channel
Since this is a "boolean" (the name is is_drum_channel), your code should use the constants FALSE and TRUE instead of 0 and 1 (existing FluidSynth code uses these constants, but uses the type 'int' instead of 'bool').
Might it be better to generalise to an enum?
enum fluid_channel_type
{
CHANNEL_TYPE_MELODIC,
CHANNEL_TYPE_DRUM
};
And call the field "fluid_channel_type channel_type".
It's questionable whether there would ever be more channel types in the future other than melodic and drum, but at least it would make code clearer to see, for instance, "chan->is_drum_channel = (9 == num) ? CHANNEL_TYPE_DRUM : CHANNEL_TYPE_MELODIC;" instead of "chan->is_drum_channel = (9 == num) ? 1 : 0;". Can anybody think of any reason why, in the future, there would be more channel types than melodic or drum?
+
+ /* Drum channel flag, 0 for melodic channel, 1 for drum channel.
+ * Previously, even for 32, 48, 64 channels only channel 9 can be drum,
+ * channel 25, 41, 57 were not treated as drum channel at all, even though
+ * ALSA shows 2, 3, 4 ports (each with 16 midi channels).
+ * We can optionally initialize channel 25, 41, 57 be GM drum channel if we
+ * want for each of those 16 midi channels -- need to change channel
+ * initialization, but may break existing apps unless they explicitly set
+ * these channels.
+ * Moreover, GM2 allows 2 drum channels 10 and 11 (9, 10 with zero-indexing).
+ * Some older XG may use drum channels 9 and 10 (8, 9 with zero-indexing).
+ * Added at end of structure, hopefully existing apps using this structure may
+ * still work without having to recompile for the time being. */
+ int is_drum_channel;
+
This comment reads more like a commit log than a comment -- the changes you have made and the rationale for them (and discussion of breakages) don't belong in comments in the code itself. I would delete all but the first line of this comment.
As for the issues raised in there, I think we should definitely only set channel 9 to drum, for backwards compatibility.
- *
- * FIXME - Shouldn't hard code bank selection for channel 10. I think this
- * is a hack for MIDI files that do bank changes in GM mode. Proper way to
- * handle this would probably be to ignore bank changes when in GM mode. - JG
*/
I don't think this FIXME has been fully resolved. I don't quite understand it, but it says that the proper way to handle it would be to ignore bank changes, which isn't what your patch is doing. Can you explain what this comment means and why it's OK to remove it?
Good work on that patch.
Matt