Patch-PolyMono-functionality

[derselbst]

This patch add Poly Mono functionality to Fluidsynth v 1.1.6

Part 1
This part describes Poly/Mono mode:

• This patch handle the MIDI specifications concept of channel basic
• MIDI modes messages in Fluidsynth:Omni On/Off, Poly OnOff.
• New API for channel basic and mode change.
• New shell commands for basic channel and mode change.

Part 2
This part describes mainly the monophonic behavior:

• Type of MIDI input controler (monophonic/polyphonic).
• MIDI CC messages handled
  CC legato(68d) On/Off
  CC portamento(65d) On/Off
  CC portamento time (msb,lsb) (5, 37d)
• Use of sustain/sostenuto pedal in monophonic mode.
• Use of CC Breath.
  New API, shell commands to handle default breath to Attenuation modulator.
• Legato mode playing.
  New API, shell commands to handle legato mode.

Please see PatchFluidPolyMono-0001.pdf

Reported by: jjceresa

Original Ticket: fluidsynth/tickets/160

[derselbst]

This Patch supersedes previous patch:PatchFluidPolyMono-0001

• Correction typos errors thanks to Ben Gonzales for reporting.
• correction to get uniform compilation on Rpi2. Thanks to Ben Gonzales for reporting and testing on
  Rpi2.
• Correction in the method to apply the patch. Thanks to R.L Horn.

Please see PatchFluidPolyMono-0002.pdf

Original comment by: jjceresa

[derselbst]

This Patch supersedes previous patch (PatchFluidPolyMono-0002)
• Many enhancements functionalities (see 1).
• Tutorials examples with commands files (see 2.1 and 3.1)
Please see PatchFluidPolyMono-0003.pdf

Original comment by: jjceresa

[derselbst]

I had a first look into this patch and stumbled over some issues:

• Please avoid abbreviations (like IZ or PZ)
• When adding new functions, please stick to the naming conventions (e.g. prefer fluid_voice_update_release() rather than fluid_update_release())
• Avoid introducing all these little getter macros like IsChanMono(). They might be helpful everyone, so better consider adding them to the public API.

Also I had trouble applying the patch. Was it really done against 1.1.6 release or against some recent git at that time?

Anyway, I appreciate this feature and your commitment for it, and I would like to integrate it. However this feature is a bit too complex to integrate and polish it on my own.

[mawe42]

The lastest patch in the original ticket has a very strage format, with mixed Windows and Unix path separators. Below is a fixed patch file which applies without errors to the latest version from the SourceForge git repository. Still needs a lot of cleanup and separation into separate commits IMHO, but at least there is a working baseline now.

Apply with patch -p2 from the fluidsynth directory.

fluid-polymono-0003.patch.gz

[derselbst]

Great Job! I had trouble applying it against upstream 1.1.6 release, but this worked smoothly. Thanks!

Created a new branch from commit f52597b (latest SourceForge git) and applied his patch as one single commit... well, at least further steps will be tracked with commits.

I think the best strategy now would be to merge master into polymono and then do the polishing. Will try that in the next days, but first need to find time looking into #205 again so this gets finished. BTW: Do you have/need commit access?

[mawe42]

BTW: Do you have/need commit access?

You mean to the polymono branch? I'm not sure I'll have time to really get into that. It's huge and touches so many different and partly unrelated things. Ideally, I would love for JJC to split it into smaller, more manageable chunks. But he said before that he doesn't think it could be done.

[jjceresa]

Hello Tom and Marcus, i am coming step by step.

Tom>Also I had trouble applying the patch. Was it really done against 1.1.6 release or against some recent git at that time?
The PolyMono patch has be done against v1.1.6 From SourceForge git (21/07/2016).
File: fluidsynth-code-git-f52597be038a5a045fc74b6f96d5f9b0bbbbc044.zip
This file was the last commit done by David (diwic) (integrating Sostenuto patch) in May 2015 .

Tom > I had trouble applying it against upstream 1.1.6 release

I don't know which trouble, but In this case it seems that "upstream 1.1.6 release" is different than v1.1.6 fluidsynth-code-git-f52597be038a5a045fc74b6f96d5f9b0bbbbc044.zip .
Anyway if you can help me to watch the troubles, may be i can help.
Again i insist to the fact that PolyMonoPatch is an adding and doesnt' break any v1.0.6 (May 2015) existing functionality.
(Only , parameter "inst_zone" has been added to fluid_synth_alloc_voice() function and all the rest are addings.)

mawe42 > It's huge and touches so many different and partly unrelated things.

It is true that the patch is huge. But it is 90% not complicated to understand for a developer ( and i am ready to help) (but 'Legato' is a bit more complex to understand for the developer).
The patch is very easy to use for musician familiar to monophonic instrument. Please don't be shy.

mawe42 >Ideally, I would love for JJC to split it into smaller, more manageable chunks. But he said before that he doesn't think it could be done.

I think it could be done on a step by step basis. Taking the time we need.
For a starting point,

1. i wish understand which troubles have arrived applying the patch again the recent upstream v1.1.6
   Once the troubles resolved, this working patch need to be verified. (I can do easyly using the tutorials examples described in the pdf).
2. Next, this working patch can be broken in small chunk. I will see how to do that.
   jjc

[derselbst]

Hi jjc,

good to have you here. I gave you commit access and applied your patch on the polymono branch as one commit. And if you ask me I would also leave it as one single commit.

There are a few conflicts I could not resolve so far. One of them is this:

fluidsynth/src/synth/fluid_synth.c

Lines 3530 to 3561 in 96034eb

	<<<<<<< HEAD:fluidsynth/src/synth/fluid_synth.c
	/* default_breath2att_modulator is not a default modulator specified in SF
	it is intended to replace default_vel2att_mod on demand using
	API fluid_set_setbreath_mode() or command shell setbreathmode.
	*/
	{
	unsigned char mono = IsChanPlayingMono(channel);
	if((!mono && IsChanPolyDefaultBreath(channel)) ||
	(mono && IsChanMonoDefaultBreath(channel)))
	/* add breathToAttenuation modulator */
	fluid_voice_add_mod(voice, &default_breath2att_mod, FLUID_VOICE_DEFAULT);
	else /* add default velocityToAttenuation modulator */
	fluid_voice_add_mod(voice, &default_vel2att_mod, FLUID_VOICE_DEFAULT); /* SF2.01 $8.4.1 */
	}
	fluid_voice_add_mod(voice, &default_vel2filter_mod, FLUID_VOICE_DEFAULT); /* SF2.01 $8.4.2 */
	fluid_voice_add_mod(voice, &default_at2viblfo_mod, FLUID_VOICE_DEFAULT); /* SF2.01 $8.4.3 */
	fluid_voice_add_mod(voice, &default_mod2viblfo_mod, FLUID_VOICE_DEFAULT); /* SF2.01 $8.4.4 */
	fluid_voice_add_mod(voice, &default_att_mod, FLUID_VOICE_DEFAULT); /* SF2.01 $8.4.5 */
	fluid_voice_add_mod(voice, &default_pan_mod, FLUID_VOICE_DEFAULT); /* SF2.01 $8.4.6 */
	fluid_voice_add_mod(voice, &default_expr_mod, FLUID_VOICE_DEFAULT); /* SF2.01 $8.4.7 */
	fluid_voice_add_mod(voice, &default_reverb_mod, FLUID_VOICE_DEFAULT); /* SF2.01 $8.4.8 */
	fluid_voice_add_mod(voice, &default_chorus_mod, FLUID_VOICE_DEFAULT); /* SF2.01 $8.4.9 */
	fluid_voice_add_mod(voice, &default_pitch_bend_mod, FLUID_VOICE_DEFAULT); /* SF2.01 $8.4.10 */
	=======
	default_mod = synth->default_mod;
	while (default_mod != NULL) {
	fluid_voice_add_mod(voice, default_mod, FLUID_VOICE_DEFAULT);
	default_mod = default_mod->next;
	}
	>>>>>>> master:src/synth/fluid_synth.c

The replacement of the default velToAtten mod with breathToAtten collides with Marcus' API for manipulating default modulator. If possible I would really like to see some way of achieving this replacement by using this new API, instead of some hard-coded way. Is a replacement even necessary? Couldnt we have both modulators?

Another issue that come to my mind:

(Only , parameter "inst_zone" has been added to fluid_synth_alloc_voice() function and all the rest are addings.)

I understand why you have done it and I currently can't offer a better alternative, but still I think this is a bad idea. fluid_synth_alloc_voice() is part of public API. A voice should only depend on fluid_sample_t (which is public itself). inst_zone however is private making fluid_synth_alloc_voice() not only an API change but basically unusable for a user. Any solution that makes fluid_synth_alloc_voice() use it's old signature would be highly appreciated.

---

Is it necessary to have that struct _fluid_basic_channel_infos_t standing alone or could we bundle it inside fluid_channel_t somehow?

I dont like the fact that fluid_voice_noteoff() returns whether a voice IsSustained. I'll refactor that by using the fluid_voice_is_sustained() API calls.

[jjceresa]

I gave you commit access and applied your patch on the polymono branch as one commit. And if you ask me I would also leave it as one single commit.

I don't yet really know how to work with this things. I don't see the usual strategy .

The replacement of the default velToAtten mod with breathToAtten collides with Marcus' API for manipulating default modulator.
Is a replacement even necessary? Couldnt we have both modulators?

The breathToAtten replacement of the default velToAtten is necessary when keyboardist wants to control dynamic with only a breath controller (but not with both key velocity + breath controller).
Without this possibility the keyboardist suffers of a lack of dynamic control. (pdf ch 3.4.4).

If possible I would really like to see some way of achieving this replacement by using this new API, instead of some hard-coded way.

Theses Marcus' API is useful to define a default modulator list at synth instance level (i.e a list for all channels) (please, correct me if i am wrong) .
The BreathMode functionality allows the replacement at noteOn time: (pdf ch 3.4.5).

1. individually at each channel level
2. independently when the channel is played polyphonic or monophonic
   So this is not really the same functionality that the Marcus' API. I see both complementary when the desired default modulators aren't not yet in the used SoundFont.

Therefore this collide is easy to solve like this:
CollideBreathMode.txt
(I'm sorry, i don't know yet how to insert a list box like in your last comment)

There are a few conflicts I could not resolve so far....

Can you help me to point the others conflicts ?

---

I understand why you have .....making fluid_synth_alloc_voice() not only an API change but basically unusable for a user. Any solution that makes fluid_synth_alloc_voice() use it's old signature would be highly appreciated.

I understand that you don't want any developer to worry about any change in this existing public API. So the only solution, i see is to supply an equivalent function (i.e named fluid_synth_mono_alloc_voice() ) called by fluid_defpreset_noteon() dedicacced to the need a channel actually in mono mode.
Anyway when a voice is running, at noteOn this voice still need to mark the InstrumentZone 'voice to be ignored if necessary. In others word , to get a convenient smooth legato between monophonic notes the addition of the flag field in an luid_inst_zone_t structure still be mandatory.

Is it necessary to have that struct _fluid_basic_channel_infos_t standing alone or could we bundle it inside fluid_channel_t somehow?

Both structure have different meaning.
typedef struct _fluid_basic_channel_infos_t fluid_basic_channel_infos_t;
Is part of the public API of PolyMono functionality (in synth.h). These basic_channel_infos describes any group of MIDI channels intended to work in distincs mode (Poly/Mono, Omni On/off). Theses groups are identified by the first channel of a group which is called Basic Channel. Only the first channel of a group is marked Basic Channel inside fluid_channel_t not the the others. (more Infos in pdf ch 2.1.1, to 2.1.7).

In others word basic_channel_infos informations are set and get by the application into a different codage in fluid_channel_t via Poly Mono API. struct _fluid_basic_channel_infos_t information can be saved by the application in permanent storage if needed.

---

I dont like the fact that fluid_voice_noteoff() returns whether a voice IsSustained. I'll refactor that by using the fluid_voice_is_sustained() API calls.

Good things. Thanks for the hint.

[derselbst]

Ok thanks so far.

Can you help me to point the others conflicts ?

Already resolved them.

[jjceresa]

Hi, tom

understand why you have .....making fluid_synth_alloc_voice() not only an API change but basically unusable for a user. Any solution that makes fluid_synth_alloc_voice() use it's old signature would be highly appreciated.

I propose a straightforward solution:

1. We keep the PUBLIC API fluid_synth_alloc_voice() function intact (like it was prior v 1.0.6).
   However this function cannot benefice of the smooth legato mode (2,3,4) (but only mode 0, 1)

2. For the purpose of legato functionality, we add a new function:

   fluid_voice_t* fluid_synth_alloc_voice_instr_zone(fluid_synth_t* synth, fluid_inst_zone_t *inst_zone,
   int chan, int key, int vel)

   (I don't think this function need to be PUBLIC).
   fluid_synth_alloc_voice_instr_zone() will be called as usual in
   fluid_defsfond.c-fluid_defpreset_noteon() and fluid_ramsfond.c-fluid_rampreset_noteon().

3. We change the signature of fluid_voice_init()
   fluid_voice_init(fluid_voice_t* voice, fluid_sample_t* sample, fluid_inst_zone_t inst_zone,
   fluid_channel_t channel, int key, int vel, unsigned int id,
   unsigned int start_time, fluid_real_t gain)

Your opinion ?

I can do that and send you the result or
may i edit directly the files in the polymono branch with the github web interface ?

[derselbst]

I still dont quite like the idea of requiring a whole instrument zone just for creating a single voice. Let's postpone this to later, I need time thinking about that.

What you could do for now is revising and cleaning up the API documentation so it is usable with doxygen, at least for those parts not likely to be changed. This means:

• getting rid of that redundant "@" at the beginning of each comment line
• instead make sure that every line starts with a "*"
• reading through the wording with respect to understanding
• fix typos and grammar ;)

So for example the doc of fluid_synth_set_basic_channel():

/**
 The API function change the mode of a an existing basic channel or insert a new
 basic channel part.
 -if basicchan is already a basic channel, his mode is changed.
 -If basicchan is not a basic channel, a new basic channel part is inserted
  between the previous basic channel and the next basic channel.
  val value of the previous basic channel will be narrowed if necessary.
 
 * @param synth, FluidSynth instance.
 * @param basicchan is the Basic Channel number (0 to MIDI channel count-1).
 * @param mode is MIDI mode infos for basichan (0 to 3).
 * @param val Number of monophonic channels (for mode 3 only) (0 to MIDI channel count).

 * @return
 * @ FLUID_OK if success
 * @ FLUID_POLYMONO_WARNING 
 * @  1)val of the previous basic channel has been narrowed or
 * @  2)val have a number of channels that overlaps the next basic channel part.
 * @  Anyway, the function does the job and restricts val to the right value.
 * @ FLUID_FAILED 
 * @   synth is NULL.
 * @   chan or val is outside MIDI channel count.
 * @   mode is invalid.
*/

Should become this:

/**
 * Changes the mode of an existing basic channel or inserts a new basic channel part.
 *
 * - If basicchan is already a basic channel, the mode is changed.
 * - If basicchan is not a basic channel, a new basic channel part is inserted between
 * the previous basic channel and the next basic channel. val value of the previous
 * basic channel will be narrowed if necessary. 
 * 
 * @param synth the synth instance.
 * @param basicchan the Basic Channel number (0 to MIDI channel count-1).
 * @param mode the MIDI mode to use for basicchan (0 to 3).
 * @param val Number of monophonic channels (for mode 3 only) (0 to MIDI channel count).
 * 
 * @return 
 * - FLUID_OK on success
 * - FLUID_POLYMONO_WARNING
 *   - 1) if val of the previous basic channel has been narrowed or
 *   - 2) if val has a number of channels that overlaps the next basic channel part.
 *   - Anyway, the function does the job and restricts val to the right value.
 * - FLUID_FAILED
 *   - synth is NULL.
 *   - chan or val is outside MIDI channel count.
 *   - mode is invalid.
 */

This would be enormously helpful.

I can do that and send you the result or may i edit directly the files in the polymono branch with the github web interface ?

The web interface might be a good point to start doing this. However I highly recommend doing it sooner or later via command line.

[jjceresa]

What you could do for now is revising and cleaning up the API documentation so it is usable with doxygen
This would be enormously helpful.

Yes, cleaning is necessary i will do that. The same is true for the pdf documentation. Writing in english is "un tour de force" for me ;)

I still dont quite like the idea of requiring a whole instrument zone just for creating a single voice. Let's postpone this to later, I need time thinking about that.

Yes, the whole instrument zone isn't required to create the voice !. For creation , only sample field is sufficient. But Playing legato is more complex. Please read.

Anyway, good Instruments are rarely composed of only one voice (Instrument Zone are often layered).
Soundfont preset are often designed with multiple key ranges and velocity ranges at Instrument Level.
That means that when playing a monophonic passage of notes n1,n2,n3 in a legato manner. n1 been the first:

1. following notes n2, n3 each make use of n1 voices, if n2 or n3 are still in the KeyRange, VelRange of InstrumentZone or PresetZone (IZ,PZ) of the running voices.
2. If n2,n3 are outside VelRange and KeyRange of some running voices, theses voices are forced in release section.
3. Next, following eligible voices need to be allocated but not the ones already running in step 1.

So, on noteOn, the legato logic must do two things:

• the legato job (step 1 above )and also
• it must respect the keyRange or velRange switching as instructed by the SoundFont designer (steps 2 and 3 above).

So an example should clarify the situation:
Let n1 with layered voices v0,v1,v2 .each represented by their respective VelRange,KeyRange ( instructed inside PresetZone/InstrumentZone).

• At n1On, voices v0,v1,v2 are allocated and started for this note n1 using fluid_defpreset_noteon().
• At n2On , assuming that a legato passage as been detected, the following occurs in fluid_synth_noteon_mono_legato() :

1. n2 is found in the VelRange,KeyRange of voice v0, so voice v0 is used (without allocation) to
   play n2.
2. n2 is found in the VelRange,KeyRange of voice v1.This the same situation as voice v0, so voice v1
   is used (without allocation) to play n2 .
3. n2 is outside the VelRange,KeyRange of voice v2.Voice v2 can't be used to play n2, so voice v2 need to be forced in release section.
4. Now all the previous voices of n1 have been explored. However, it is possible that n2 will be able to enter in the Velrange,KeyRange of a new elligible Preset Zones or Instrument Zones. In this case new voices need to be allocated to play this new Preset Zones or Instrument Zones.
   This allocation is done using fluid_defpreset_noteon(). During this new allocation, the Instrument Zones corresponding to the already running voices v0,v1 (revided at step 1 and 2) must be ignored.
   So step 1 and 2 need to mark the corresponding Instrument Zone to be ignored during step 3.

So, for legato playing, each simultaneous created voice needs also to remember the KeyRange,VelRange and also need a flag to mark each instrument zone corresponding to all voices already revived by the legato logic. Actually this remembering is done in each Instrument Zone concerned.
Thanks for reading so far.

[jjceresa]

Hi, Tom
2 files have been updated on branch 'polymono'

• fluid_synth_mono.c - Cleaning up the documentation
• fluid_synth_poly_mono.c - cleaning up the API documentation

Good evening.

[jjceresa]

Sorry, i have clicked the wrong button :(

[derselbst]

Thank you, that looks better now. I made a hacky change to the git history of the polymono branch (you became author of badddc5 in hindsight). Not sure if a simple git pull will be sufficient to update that (if not please re-clone the whole repo via git clone).

In some public synth functions I've seen that you're doing custom checks to ensure chan is within range. E.g. here:

fluidsynth/src/synth/fluid_synth_polymono.c

Lines 382 to 383 in d5b9523

	nChan = synth->midi_channels; /* MIDI Channels number */
	if(chan < 0 || chan >= nChan) return FLUID_FAILED;

synth->midi_channels is mutex protected and for consistency you should better use FLUID_API_ENTRY_CHAN for these checks.

Meanwhile I'll try to find a time slot reading through your pdf, so we can get into more technical discussion before doing too much potentially unnecessary code related cleanup.

@mawe42 In case it gets too annoying: You know how to unsubscribe from this thread? I wouldnt mind :)

[jjceresa]

I made a hacky change to the git history of the polymono branch (you became author of badddc5 in hindsight).

I've seen that badddc5 seems result of the original patch polymono (isn't it ?). Now polymono branch is made of (d5b9523 and 72d9cdf) (6 h ago) and this retrograded commit badddc5 (7 days ago)(correct me if i'm wrong) . Sry, I'm lost, and don't understand the intend ?

---

synth->midi_channels
is mutex protected and for consistency you should better use FLUID_API_ENTRY_CHAN for these checks.

Thanks for the hint !

(Hey! what is the format you have used in your previous comment: fluidsynth/src/synth/fluid_synth_polymono.c Lines 382 to 383 in d5b9523) ?

---

Meanwhile I'll try to find a time slot reading through your pdf, so we can get into more technical discussion before doing too much potentially unnecessary code related cleanup.

Good for me.
The pdf is large and doesn't contain all technical aspect. , if i can help you finding the information you need without lost of time don't hesitate.

[derselbst]

I consider replacing FLUID_POLYMONO_WARNING with FLUID_FAILED as the only way to go. Again: What the user wants to do is ambiguous. The only consequence can be rejecting the command. Additionally we could use FLUID_LOG(FLUID_INFO,..) to inform the user what went wrong specifically.

[jjceresa]

Ok, I will replace FLUID_POLYMONO_WARNING with FLUID_FAILED .

[derselbst]

Thanks.

After having read the pdf now: Do we really need breathmode inside the synth? It seems like the only purpose of it is to replace the default vel2att mod on demand. This feels like a hack. Why cant the user add a custom breath2att modulator to the soundfont and hardcode the noteon velocity (e.g. by using the midi_router)?

[jjceresa]

This feels like a hack. Why cant the user add a custom breath2att modulator to the soundfont and hardcode the noteon velocity (e.g. by using the midi_router)?

This is usually what an ewi player will do. Often its ewi device allows to hard code the velocity to a fixed value.This way with the right breath2att modulator already in the soundfont, this setting is fine. So in this case no need of breathmode.

Now, when the player is a keyboardist willing to play its instrument sometimes polyphonic (with velocity) and sometimes monophonic (with CC Breath as a ewi player does) during the same song (i.e in real time), this is possible using thelegato pedalduring the performance. Here the breathmode can be useful (pdf 3.4.5). As the pdf doesn't explain this well please see the figure.

Note that Breath Sync mode also is useful only for MIDI keyboard on input (pdf 3.4.5).

In short, breathmode (breath2att and Breath Sync) was thought to add monophonic behavior functionality when the soundfont was thought for polyphonic , MIDI input controller is polyphonic (i.e keyboard) and legato pedal (on/off) is used.

[derselbst]

Ok. So we need it.

My main problem is that this patch adds huge complexity to the code base, which I think is at the brink of being unmaintainable. E.g.:

• understanding how a simple noteoff works has become incredibly difficult
• using all those CustomCamelCaseNamed functions and helper macros that eventually expand to other helper macros again which are however defined in a different file doesnt really help understanding what's going on
• functions are defined where one would not expect them (e.g. fluid_channel_add_monolist() in fluid_synth_mono.c, what is the purpose of this monolist btw?)

Frankly speaking, looking at the code as it currently is often makes me wanting to run away. If there's anything not really necessary, perhaps consider removing it. Hopefully this gets clearer after it will have been shaped.

[jjceresa]

My main problem is that this patch adds huge complexity to the code base, which I think is at the brink of being unmaintainable.

Yes this patch adds a large bunch of functionality in a whole. So, i understand this adding seems make the code base very hard to maintain.

understanding how a simple noteoff works has become incredibly difficult

what is the purpose of this monolist btw?)

Yes, both noteOff (and noteOn ) input events on a mono channel are difficult to understand due to the necessary legato detector existence.

The possible events chain are :
When the musician plays legato:

• MIDI NoteOn on mono channel--->fluid_synth_noteon_mono_LOCAL()-Legato Detector (using the monolist)-->fluid_synth_noteon_mono_legato().
• MIDI NoteOff on mono channel--->fluid_synth_noteoff_mono_LOCAL()-Legato Detector (using the monolist)--> fluid_synth_noteon_mono_legato().

When the musician plays staccato :

• MIDI NoteOn on mono channel--->fluid_synth_noteon_mono_LOCAL()-Legato Detector (using the monolist)-->fluid_synth_noteon_mono_staccato().
• MIDI NoteOff on mono channel--->fluid_synth_noteoff_mono_LOCAL()-Legato Detector (using the monolist)--> fluid_synth_noteoff_monopoly(...,1).

using all those CustomCamelCaseNamed functions and helper macros that eventually expand to other helper macros again which are however defined in a different file doesnt really help understanding what's going on

I could review this and place the macros in the correct file.

Frankly speaking, looking at the code as it currently is often makes me wanting to run away. If there's anything not really necessary, perhaps consider removing it. Hopefully this gets clearer after it will have been shaped.

For a better understanding, may the best would be to propose functions fluid_synth_noteon_mono_LOCAL() and fluid_synth_noteoff_mono_LOCAL() both empty and than adding functionalities step by step taken the time we need ?
cheers :)

[derselbst]

I could review this and place the macros in the correct file.

Yes please.

For a better understanding, may the best would be to propose functions fluid_synth_noteon_mono_LOCAL() and fluid_synth_noteoff_mono_LOCAL() both empty and than adding functionalities step by step taken the time we need ?

No need to strip that away. The often I see it the more likely it is I'll understand it.

Back to the monolist: So you use it to store any pending noteon event which can then be used by fluid_synth_noteoff_mono_LOCAL() to do a fluid_synth_noteon_mono_legato():

fluidsynth/src/synth/fluid_synth_mono.c

Lines 398 to 405 in 71f7c35

	if(IsValidNote(iPrev)) { /* legato playing detection */
	/* legato from key to iPrev key */
	/* the voices from key number are to be used to
	play iPrev key number. */
	status = fluid_synth_noteon_mono_legato(synth, chan,
	key, channel->monolist[iPrev].note,
	channel->monolist[iPrev].vel);
	}

Couldnt you have also used fluid_synth_get_voicelist() to achieve something similar?

[jjceresa]

fluid_synth_get_voicelist() seems not appropriate to do the job of the monolist.

When the musician intends to play n1,n2,n3 in legato manner.
On noteOn/noteOff on a mono channel the monolist is managed like that:

On n1On, n1 is inserted at the end in the monolist, the result in monolist is: n1.

• there aren't previous note in the monolist, so it is not a legato playing, the note n1 is started normally.

On n2On, n2 is inserted at the end in the monolist, the result in monolist is: n1,n2.

• legato playing from n1 to n2 is detected,so n1' voices (i.e voice with key n1) are used to play note
  n2.

On n3On, n3 is inserted at the end in the monolist, the result in monolist is: n1,n2,n3

• legato playing from n2 to n3 is detected,so voices with key n2 are used to play note n3.

---

Now the musician intends to play a legato passage in the reverse way from n3 to n1 but without n2. So to forget n2, the musician release n2 (before releasing n3).
On n2Off the note n2 is removed from the monolist, the result is: n1,n3. (voices,'n3 are still sounding).
On n3Off note n3 is removed and a legato from n3 to n1 is detected.

• voices with key n3 are used to play note n1. The result in mono list is: n1.

Note:When playing in the reverse order, the monolist keeps trace of the note value (the key) and also the velocity of the note.

Notes:

• The monolist memory cost is very small (40 bytes per Channel).
• Insertion/removing is very performant at noteOn/noteOff.

[derselbst]

When the musician intends to play n1,n2,n3 in legato manner.

I dont see any noteoffs here, though I was expecting them. So a basic question: Does playing legato require the musician to keep the keys depressed?

The monolist memory cost is very small (40 bytes per Channel). Insertion/removing is very performant at noteOn/noteOff.

I'm mostly concerned about code reusability.

Btw: you missed a few things here on github. Please pay attention to the little bell sign on the top right, that informs you about any new activity related to you.

[jjceresa]

I dont see any noteoffs here, though I was expecting them. So a basic question: Does playing legato require the musician to keep the keys depressed?

On monophonic instrument.
At n2On, key n1 must be still depressed to hear a good legato beetween n1 and n2. After n2On the musician doesn't need to keep n1 depressed. So for each couple of notes (n1,n2) , (n2,n3), (n3,n4), the 1st note of the couple needs only to be slightly overlapped by the second one.

• Playing this: n1On,n2On,n1Off,n3On,n2Off,....
• produce the same result that playing this: n1On,n2On,n3On..... (keys keep depressed)

But, in the case the musician intends playing legato in ascendant order (example above the line in my previous comment) followed by descendant order (the example continues below the line) he need to keep the keys depressed.

Imagine a flutist (a flute with holes) player who wants to play the whole passage in legato playing: n1,n2,n3,n2,n1
He need to do:

• Press key n1, n1 is heard. (1st note the legato passage)
• Press key n2, n2 is heard (using voices of n1)
• Press key n3, n3 is heard (using voices of n2)
• Depress n3, n2 is heard (because key n2 was still depressed) (using voices of n3)
• Depress n2, n1 is heard (because key n1 was still depressed) (using voices of n2)
• Then depress n1, n1 is stopped. (end of the legato passage).

Btw: you missed a few things here on github. Please pay attention to the little bell sign on the top right, that informs you about any new activity related to you.

Thanks, to advise me.

[derselbst]

Ok thanks, now it gets clearer to me.

So since you never know how many keys may still be depressed, wouldnt it be more appropriate to use fluid_list_t for monolist instead?

[jjceresa]

Sorry, i don't see the little bell !

[jjceresa]

So since you never know how many keys may still be depressed, wouldnt it be more appropriate to use fluid_list_t for monolist instead ?

The number of keys is still limited by the numbers of fingers on both hands. 10 entries are usually sufficient (20 max if 2 musicians play on the same keyboard) . Each entry are fast linked (forward and backward) as circular buffer. Does entries in fluid_list_t can be linked forward and backward ?.

[derselbst]

(20 max if 2 musicians play on the same keyboard)

And I will get the first bug report whenever this happens :)

Does entries in fluid_list_t can be linked forward and backward ?.

Currently it's a forward only list. But I still think it's a better approach than a fixed size array.

[jjceresa]

Sorry, even after reloading the page there is still no bell. This is Curious ?!.

[derselbst]

Then please pay attention to your emailbox:

3babbed#commitcomment-25057744

#251

[jjceresa]

Then please pay attention to your emailbox

I have assumed that you are waiting for comment on this subject ?. This has be done.

---

There is still no little Bell at the top right !. I assume this is a notification device. Any idea on this issue ?. May be i need to be registered somewhere ?.

---

But I still think it's a better approach than a fixed size array.

It is a fixed array (i.e 10 entries) that works like a circular buffer. So when the musician play legato n1On,n2On,n3On,....n10,n11,n12.... Any notes will be always played. The only risk is that when the legato passage is longer than the fixed size (i.e 10) most ancient notes will be n3,n4, notes n1,n2 are lost and will be never able to sound when the musician will continue playing legato on the reverse order. During this the reverse order, only notes n12,n11,n10,n9,n8, to n4,n3 will be hear (10 most recent notes).

• To avoid a limitation in the size we can add a setting monolist-size so the user can set the size to a sufficient value. For example with a settings of monolist-size = 20 the situation is very comfortable for playing long legato passage. The monolist would be allocated at synth creation.
• We must absolutely avoid any solution doing memory allocation (alloc) at noteOn time as the well know resul, it is very very poor in performance.

This is the most reasonable approche i see.

[derselbst]

Then please pay attention to your emailbox

I have assumed that you are waiting for comment on this subject ?. This has be done.

I wanted to say that any github activity is also sent as mail.

There is still no little Bell at the top right !. I assume this is a notification device. Any idea on this issue ?. May be i need to be registered somewhere ?.

You only need to be signed in at github. Perhaps update your browser?

I dont think introducing a setting is a way to go. Even with documentation most users would not know how to use it correctly. Avoiding allocs on noteon is reasonable ofc. Currently cant think of any better solution.

[jjceresa]

You only need to be signed in at github. Perhaps update your browser?

In fact, only email notification setting was enabled by default, but not web notifications setting. Theses settings can be changed in the menu Personal settings/notifications.
Now the little Bell is displayed. The response to my question was in the GitHub documentation.

Now about FluidSynth settings, why did you think that Even with documentation most users would not know how to use it correctly ? ;)

In fact, one major difficulty is to write good documentation because this is a hard task. For example
explaining the purpose of synth.parallel-render settings is not an easy task but anyway, the author of the deprecated information in this description has done well. Thanks to him, it was well worth it. ;)

[derselbst]

In fact, only email notification setting was enabled by default, but not web notifications setting.

Oh dear, they must have changed the default behaviour of that...

why did you think that Even with documentation most users would not know how to use it correctly ? ;)

To a user all that matters is "I want to play legato... oops it doesnt work. Reporting bug upstream." The idea of having a setting that would just need to be slightly increased in order to fix this "bug" is what I would consider to be quite unconventional. So I'm afraid most users wouldnt simply pay attention to this setting or being able to draw a connection between the setting and buggy legato playing.

the author of the deprecated information in this description has done well.

Good to know... I'm to blame ;)

[jjceresa]

To a user all that matters is "I want to play legato... oops it doesnt work.

It is not truly the case "legato playing" always continue working even when the monolist overflows (because it works circular).

So I'm afraid most users wouldnt simply pay attention to this setting or being able to draw a connection between the setting and buggy legato playing.

Anyway, i understand and agree you argue
Good evening.

[jjceresa]

Just for information.
A new chapter has been added in FluidPolyMono-0003.pdf (at the end) : Part 4 - Appendices for understanding implementations in FluidSynth . In the hope this will be useful.

[derselbst]

Yes, that flow chart is nice, thanks. However it brings up something else that strikes me. In fluid_synth_noteon_LOCAL() there is this else branch for handling non leagto notes on polyphonic channels.

fluidsynth/src/synth/fluid_synth.c

Lines 1050 to 1062 in 2efbfa9

	else { /* channel is poly and legato Off) */
	/* play the noteOn in polyphonic */
	/* Set the note at first position in monophonic list */
	fluid_channel_set_onenote_monolist(channel, (unsigned char) key,
	(unsigned char) vel);
	/* If there is another voice process on the same channel and key,
	advance it to the release phase. */
	fluid_synth_release_voice_on_same_note_LOCAL(synth, chan, key);
	return fluid_synth_noteon_mono_legato(synth, chan, INVALID_NOTE, key, vel);
	}

However you're calling fluid_synth_noteon_mono_legato(), a function whose name suggests the pretty opposite. I know you want to reuse functionality, but couldnt you just call fluid_preset_noteon directly here? (let me, guess: that would break portamento?) Or perhaps come up with a more general naming for fluid_synth_noteon_mono_legato().

BTW: The build of the polymono branch currently fails, indicated by a red cross next to the commits in the commit history view: https://github.com/FluidSynth/fluidsynth/commits/polymono

[jjceresa]

I know you want to reuse functionality,but couldnt you just call fluid_preset_noteon directly here? (let me, guess: that would break portamento?)

Calling fluid_preset_noteon directly here will break CC Portamento On/Off but also will break CC portamento control (PTC).
So we need to call fluid_synth_noteon_mono_legato() this will allow

• CC portamento On/Off and also
• CC Portamento Ctrl (PTC) (pdf ch 4.3 3.4.11) which can benefit of the legato logic inside the function (if a CC PTC occurs).

Or perhaps come up with a more general naming for fluid_synth_noteon_mono_legato().

Yes you are right ,actually the function naming is incorrect and should be now called fluid_synth_noteon_monopoly_legato()

---

BTW: The build of the polymono branch currently fails, indicated by a red cross next to the commits in the commit history view:

I don't understand what has happened ?.

[derselbst]

I don't understand what has happened ?

We use Travis as continuous integration system. For every commit it tries to automatically build fluidsynth with different configurations on linux and macosx. The polymono branch currently fails to build. Just click on that red cross of the most recent commit in the commit history to get to Travis and select one of the build configurations that fails (currently all) to view the build log.

[jjceresa]

we use Travis as continuous integration system. For every commit it tries to automatically build..

Interesting feature. Could we tell to Travis to stop the build or restart a build after it has failed ?

The polymono branch currently fails to build ...

Thanks for the tip, but now the red cross has automatically disappeared. May i suppose that Travis have retried and now the build is success ?.

[derselbst]

Could we tell to Travis to stop the build or restart a build after it has failed ?

At least I can cancel a build, not sure if you have privileges, dont know how to grant them either. But why would you want to restart a failed build? New commits will be built automatically. Perhaps you mean an errored build, like in #269, indicated by a red exclamation mark in the travis view? Errored builds have a problem with the build process itself. Often travis itself is to blame, you can ignore that, they'll fix it... or I do.

Thanks for the tip, but now the red cross has automatically disappeared.

They were replaced with a yellow dot, because travis is currently building. When clicking it you will also be redirected to the travis build status page for that commit and see that it still fails.

You can also see that on the branches overview page: https://travis-ci.org/FluidSynth/fluidsynth/branches