[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [fluid-dev] Propose changes for drum channels support

From: Matt Giuca
Subject: Re: [fluid-dev] Propose changes for drum channels support
Date: Wed, 26 Jan 2011 11:03:13 +1100

Hi Jimmy,

I am not a FluidSynth developer, just an interested person, so my opinions don't represent the view of the FluidSynth project.

This seems like a valuable generalisation of a previously hard-coded value. Given that your new flag will default to 0 for all channels and 1 for channel 9, it won't change anything but add a useful new feature.

A few points: Firstly, in the future could you attach your patches as attachments instead of pasting them at the end. It makes them much easier to read and apply.

Add a flag to struct:


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

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]