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

[jimmy]

--- On Tue, 1/25/11, Matt Giuca <address@hidden> wrote:
> 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.
> 

I'm not a FS dev either.  Just seeing something I may want to use isn't there, 
so I just take a crack at it.  Everyone on this [fluid-dev] list is one way or 
another interested in the coding/development side of FS one way or another.  
That's why I ask for opinions instead of contacting just one of the FS 
developers and try to push through some new features.


> 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.
> 

There are some pro's and con's of this.  Attachments are cleaner so folks don't 
have to cut and paste them.  However, most mailing lists will not archive 
attachments, just the messages.  From time to time I have seen archived 
messages (sometimes from mirrored/relayed lists) referring to attachments 
that's just not there.  I don't think Yahoo lists would archive attachments 
either.  Such have been pointers to nowhere for me, and I don't like that.  
Perhaps, I could attach and include them (2 copies in the message).


> 
> 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".

Such are coding styles, I don't mind either way.  Sometimes in printing 
debugging messages, enum/boolean make it extra work to print out info.  I often 
use simple int type, just personal preference.  No big deal for me here, 
however the FS dev's want to commit the code.


> 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?
> 

For musical use, I don't know.  For MIDI lightings, stage control, video 
mixing, animation...  who knows, perhaps those things may become more common.


> 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.
>  

Comments were just my rationalization, if it's not needed, or could be shorten, 
let it be.

That's right, the patch doesn't break any backward compatibility, I did put 
such consideration in the comment so later on, folks can go back and see why.


> -   * 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?
> 

>From what I understand, and strictly speaking, GM mode doesn't allow bank 
>changes at all, 128 instruments and some drum kits, that's it.

Even later comments on GM specs, it's up in the air whether hardware and 
software synths should disallow bank changes at all.  It was noted that some 
implementations automatically switch out of GM-mode when a bank change message 
is received.

So the first half of the comment is handled by this patch, the later part of 
that comment is a wash.


> Good work on that patch.
> 

Thanks for sharing your thoughts, much appreciated.

Jimmy