Re: [fluid-dev] Re: Patch for channel_type, also XG drum-channel autoswitch

[David Henningsson]

On 2011-02-10 02:45, jimmy wrote:
> > Hey jimmy,
> >
> > Thanks for the research. I've committed the patch now (with
> > some trivial
> > changes). Thanks for your contribution!
> >
> > And to the rest of you - this bank select handling seems to
> > be a never
> > ending debate, and it's not my area of expertise, so let me
> > know if this
> > change screwed something up for you.
> >
> > // David
>
>
> Your change is not correct in both MSB, and LSB handling for XG bank 
> calculation.

Yes, I let it stay the way Pedro wrote it. Maybe I should have mentioned 
that explicitly, sorry.

Basically the problem is that Soundfont files can have bank numbers up 
to 128 only, and that bank numbers 0-127 are melodic and bank 128 is 
percussion. At least that's the way SWAMI works. I checked Unison.sf2, 
and it follows this as well. I assume it's somewhere in the sf2 standard.

So bank numbers above 128 usually make no sense, which is why MSB is 
ignored for XG (and why MMA style is not the default...). We're kind of 
stuck between sf2's standard and XG's standard, and need to figure out 
how to mediate between them.

> -----
>
> The problem with fluid_channel_set_bank_lsb():
>
>    if (style == FLUID_BANK_STYLE_XG)
>        newval = (oldval&  ~BANK_MASKVAL) | (banklsb<<  BANK_SHIFTVAL);
>
> is that any existing MSB values will be zerroes out (your code is using 
> ~BANK_MASKVAL, instead of ~BANKLSB_MASKVAL).  So it should be:
>
>    if (style == FLUID_BANK_STYLE_XG)
>        newval = (oldval&  ~BANKLSB_MASKVAL) | (banklsb<<  BANK_SHIFTVAL);
>
> which is the same as MMA style calculation.
>
> My patch also saves the LSB with XG drum-channels.  The current flow ignores 
> drum channels LSB for XG mode.

Good point. Probably it makes sense to remember the banknum changes even 
if they're currently "hidden" when they're drum channels. This is 
changed in r406.

> ------
>
> And in fluid_channel_set_bank_msb() you modified it as:
>
>    if (style == FLUID_BANK_STYLE_XG)
>    {
>      /* XG bank (128*MSB+LSB), save MSB, do drum-channel auto-switch */
>      /* The number "120" was based on several keyboards having drums at 120 - 
> 127,
>         reference: 
> http://lists.nongnu.org/archive/html/fluid-dev/2011-02/msg00003.html */
>      chan->channel_type = (120<= bankmsb) ? CHANNEL_TYPE_DRUM : 
> CHANNEL_TYPE_MELODIC;
>      return;
>    }
>
> Don't "return" there, it has not save MSB yet.  That's why the "if-statement" in my patch 
> was written without the "return", so it would flow through to the code below.
>
> So if a "return" is preferred from within the block, then the code bock above 
> should be changed to:
>
>    if (style == FLUID_BANK_STYLE_XG)
>    {
>      /* XG bank (128*MSB+LSB), save MSB, zero out LSB, do drum-channel 
> auto-switch */
>      /* The number "120" was based on several keyboards having drums at 120 - 
> 127,
>         reference: 
> http://lists.nongnu.org/archive/html/fluid-dev/2011-02/msg00003.html */
>      chan->channel_type = (120<= bankmsb) ? CHANNEL_TYPE_DRUM : 
> CHANNEL_TYPE_MELODIC;
>      oldval = chan->sfont_bank_prog;
>      newval = (oldval&  ~BANKMSB_MASKVAL) | (bankmsb<<  (BANK_SHIFTVAL + 7));
>      chan->sfont_bank_prog = newval;
>      return;
>    }
>
> My original patch shares the MMA calculation, using "~BANKMSB_MASKVAL" to 
> update the MSB value.
>
>
> ----
>
> There are some midi files which use 2 drum channels in XG mode in:
>
>     psrtutorial.com/songs/Yamaha/XGCurrent.zip
>
> Trying Fluidsynth in XG mode with Unison.sf2:
>
>     fluidsynth  -o synth.midi-bank-select=xg  Unison.sf2
>
> Playing the midi file "JazzJung Yamaha '96.mid".  Without this patch, using 
> latest SVN code, the message I get from fluid command interface:
>
>     fluidsynth: warning: Instrument not found on channel 6 [bank=0 prog=1], 
> substituted [bank=0 prog=0]
>
> with this patch, the message is:
>
>     fluidsynth: warning: Instrument not found on channel 6 [bank=16128 prog=1], 
> substituted [bank=16128 prog=0]
>
> Which will help figuring the real midi processing behind the scene, 16128 = 
> (126 * 128).  So that's [MSB=126,LSB=0,prog=1] it is looking to use.

I did look over the fluid_synth_program_change function and tried to 
clear it up a little. It's also in r406. With that patch and your 
example I now get:

fluidsynth: warning: Instrument not found on channel 6 [bank=128 
prog=1], substituted [bank=128 prog=0]

...which is what it actually did, both before and after r406.

// David