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:
>
> _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
_______________________________________________
fluid-dev mailing list
fluid-dev@nongnu.org
http://lists.nongnu.org/mailman/listinfo/fluid-dev
