Mixer MIDI channel mapping support

[jjceresa]

This PR addresses #669 point 1.1.
It proposes 3 API functions for mixer MIDI channels mapping:

• fluid_synth_mixer_set_mapping()
• fluid_synth_mixer_reset_mapping()
• fluid_synth_mixer_get_mapping()
  And the appropriate shell commands:
• chanmap [chan1 chan2..] Print all or some MIDI channels mapping.
• resetchanmap [chan1 chan2..] Set MIDI channels default mapping.
• setchanmapout chan0 out0 [chan1 out1..] Set MIDI channels mapping to dry output.
• setchanmapfx chan0 out0 [chan1 out1..] Set MIDI channels mapping to fx unit.
• setchanfxmapout chan0 out0 [chan1 out1..] Set fx unit mapping to dry output.

[derselbst]

@mawe42 Could you pls. comment before I do?

[mawe42]

@mawe42 Could you pls. comment before I do?

Sure, but I won't have time until tomorrow evening.

[mawe42]

Overall this looks very nice. But I would suggest to split the public API into two parts:

1. for setting and getting channel to dry buffer and fx buffer/fx unit mappings
2. for setting and getting fx buffer/fx unit to output buffer mappings.

I think that would make it clearer that the fx unit to output buffer mapping is not necessarily local to a MIDI channel. Consider the following case: fx unit 1 is set to write to output 1 and MIDI channels 1 and 2 both feed fx unit 1. Now the user changes the fx output buffer for MIDI channel 1 by issuing the command

fluid_synth_mixer_set_mapping(synth, -1, -1, -1, -1, 1, 2);

It might be surprising to the user that the fx from MIDI channel 2 is now also suddenly on output buffer 2. So by issuing a command for a particular MIDI channel, it's possible that many other MIDI channels are also affected. That doesn't feel quite right to me.

Also, the missing symmetry on the API calls is bothering me. fluid_synth_mixer_get_mapping returns the mappings for a single MIDI channel, fluid_synth_mixer_set_mapping can operate on three different MIDI channels.

I didn't look too deep into the actual implementation, yet. But I'll write some general comments about this feature into #669.

[jjceresa]

Thanks for your feedback.

I think that would make it clearer that the fx unit to output buffer mapping is not necessarily local to a MIDI channel.

Yes, I understand that it would be clearer that this mapping type isn't instructed from a MIDI channel source.
On the other hand, if we separate this mapping from a MIDI channel source as suggested, the setting of
a fxunit-to-output mapping will requires to scan and verify that this fxunit must be already mapped to a MIDI channel source. With the actual implementation, the verification is done directly without requiring scanning,.

So by issuing a command for a particular MIDI channel, it's possible that many other MIDI channels are also affected. That doesn't feel quite right to me.

As far the user has decided to map MIDI channels 1 and 2 both feeding the same fx unit means that the user is now aware that these MIDI channels are dependent of this fx unit. Changing the mapping fxunit1-to-output will affect both MIDI channels 1 and 2. This is a normal situation and splitting the API will lead to the same result.

Also, the missing symmetry on the API calls is bothering me. ........fluid_synth_mixer_set_mapping can operate on three different MIDI channels.

Actually fluid_synth_mixer_set_mapping allows to do the 3 mapping type at the same time on the same MIDI channels or on distinct MIDI channels. To enhance the symmetry API, perhaps this API could be splited in 3 distinct API, (one API for each mapping type), keeping the same parameters semantic) ?

[jjceresa]

To enhance the symmetry API, perhaps this API could be splited in 3 distinct API, (one API for each mapping type), keeping the same parameters semantic) ?

However, I would prefer keeping this API allowing to do the 3 mapping type at the same time. This ensures that the required mapping type(s) requested by the caller will be executed atomically among the arrival of MIDI note inside the synth instance.

[mawe42]

However, I would prefer keeping this API allowing to do the 3 mapping type at the same time. This ensures that the required mapping type(s) requested by the caller will be executed atomically among the arrival of MIDI note inside the synth instance.

But why is that important? And if it is important, why do the new shell commands not enforce it as well?

[jjceresa]

But why is that important? And if it is important, why do the new shell commands not enforce it as well?

The API is intended to be called in a realtime context playing (or not). Usually, shell command executed by human aren't use for realtime context playing. For simplicity for human, I have chosen to split the shell command mapping to reduce the number of arguments (6) . If you think that it would be useful to add a shell command (allowing 3 mapping) intended to be called in context playing, I am ready to do this, please feel free to request ?.

[jjceresa]

But why is that important?

Sorry I missed your first question. For example, if I a musician triggers a predefined mapping change composed of 2 mapping (chan-mapto-out + chan-mapto-fx), making this mapping change atomic insures that even a note is played at the same time (i.e between chan-mapto-out and chan-mapito-fx), this will not produce an unexpected audible result.

[mawe42]

Usually, shell command executed by human aren't use for realtime context playing. For simplicity for human, I have chosen to split the shell command mapping to reduce the number of arguments (6) .

I completely understand. But the API is also consumed by humans, more specifically by the programmer. So in my opinion the API should provide functions in such a way that they are simple to understand and that their effects are obvious and ideally also predictable.

If you think that it would be useful to add a shell command (allowing 3 mapping) intended to be called in context playing, I am ready to do this, please feel free to request ?.

If atomic changes to the channel mappings are important and possible, then yes: I think shell users should also benefit from atomic changes. But if I understand your proposal correctly, then guaranteed atomic changes are only possible for chan_to_out and chan_to_fx. As multiple channels can use the same fx unit, making changes to fx_to_out will always have the possibility of also affecting other channels.

But maybe I didn't express my concerns about the symmetry and parameter structure well enough. Let me give an example of how I think it might be easier to understand for API users:

int fluid_synth_mixer_get_channel_mapping(fluid_synth_t *synth, int chan, int *buffer_idx, int *fx_unit);
int fluid_synth_mixer_set_channel_mapping(fluid_synth_t *synth, int chan, int buffer_idx, int fx_unit);
int fluid_synth_mixer_reset_channel_mapping(fluid_synth_t *synth, int chan);

int fluid_synth_mixer_get_fx_mapping(fluid_synth_t *synth, int fx_unit, int *buffer_idx);
int fluid_synth_mixer_set_fx_mapping(fluid_synth_t *synth, int fx_unit, int buffer_idx);
int fluid_synth_mixer_reset_fx_mapping(fluid_synth_t *synth, int fx_unit);

And then make the shell commands follow the same layout.

Split this way, it makes it clear to the user of the API that fx_units and channels are separate objects that are manipulated separately. And that changing the output buffer of an fx_unit might affect more than one MIDI channel.

And if we went down the path of removing the modulo channel mapping with audio-channels, audio-groups etc as I proposed in #669, I guess the reset functions would not be necessary anymore. But that is a different conversation and as Tom rightly said, probably one we should have on the mailing list.

[derselbst]

Please note that many synth API calls are only "atomic" when executed from synthesis context. So, just like we have different functions for CC and Prog changes, we should split up the fluid_synth_mixer_set_mapping(), at least in a way as suggested by Marcus.

[jjceresa]

Split this way, it makes it clear to the user of the API that fx_units and channels are separate objects that are manipulated separately. And that changing the output buffer of an fx_unit might affect more than one MIDI channel.

I am fine for split and clarifying the API usage (and to abandon the atomic property that could be solved at application level).
It is true that the user must consider that channel and fx are "separate" objects, however when using this mapping API he must also be aware that these objects should connected each to other to get coherent paths from channel source to final destination when calling each split API.

• path 1 (channel-to-buff) : channel =======>buffer_idx
• path 2 (channel-to-fx): channel ==>fx
• path 3 (fx-to-buff): channel ---->fx===>buffer_idx

For path 3, having an API int fluid_synth_mixer_set_fx_mapping(synth, fx_unit, buffer_idx) allows to set
a link between fx_unit and buffer_idx. But this link is a non sense in the case of fx unit have no channel source already linked to its input. To deal with this case, the API have to scan all channels and must return FLUID_FAILED.

So, I suggest rather to use this signature (that does the same job) .fluid_synth_mixer_set_chanfx_mapping(synth, chanfx, buffer_idx) . chanfx designate the fx unit that must be linked to buffer_idx. This makes the user aware that the API will fail in the case of channel chanfx isn't actually linked to any fx unit.

So I suggest to split the proposed fluid_synth_mixer_set_mapping API in 3 APIs:

int fluid_synth_mixer_set_chan_to_buf_mapping(fluid_synth_t *synth, int chan, int buffer_idx);
int fluid_synth_mixer_set_chan_to_fx_mapping(fluid_synth_t *synth, int chan, int fx_unit);
int fluid_synth_mixer_set_chanfx_to_buf_mapping(fluid_synth_t *synth, int chanfx, int buffer_idx);

Of course shell commands should follow the same layout.
I am fine also to split fluid_synth_mixer_get_mapping in 3 API, but I feel this is not probably not necessary ?.
Having default mapping all to buffer 0 is simpler to understand for user than the modulo stuff.

I guess the reset functions would not be necessary anymore.

Not sure. For users who are lost during their mapping settings, maybe it would be helpful and faster to have a reset function that set things in a well know state (i.e the default state) .

[mawe42]

This is really strange. I keep getting notifications from GitHub that you have commented on this PR. And it's always the same comment. Sometimes I get only 2-3, sometimes 10+.

[jjceresa]

This is really strange. I keep getting notifications from GitHub that you have commented on this PR. And it's always the same comment.

You means many notifications for only one message ? or many duplicates of the same message!!! ?.

The former is probably due because I had fixed some grammar mistake several times on the same message ;). I am sorry for this disorder.

[jjceresa]

Assuming we agree to split the set mapping API in 3. I must revert my though about my proposed API in previous message:

• fluid_synth_mixer_set_chanfx_to_buf_mapping(fluid_synth_t *synth, int chanfx, int buffer_idx);

and I prefer Marcus's version fluid_synth_mixer_set_fx_mapping(fluid_synth_t *synth, int fx_unit, int buffer_idx) that is easier to understand. Marcus's version have be renamed fluid_synth_mixer_set_fx_to_buf_mapping below::

After split, this could lead to the 3 APIs:

int fluid_synth_mixer_set_chan_to_buf_mapping(fluid_synth_t *synth, int chan, int buffer_idx);
int fluid_synth_mixer_set_chan_to_fx_mapping(fluid_synth_t *synth, int chan, int fx_unit);
int fluid_synth_mixer_set_fx_to_buf_mapping(fluid_synth_t *synth, int fx_unit, int buffer_idx);

Of course these functions name here can be changed.
Having only 2 parameters makes each API easier to understand especially when the API return FLUID_FAILED.

Any opinions about split in 3 APIs ?

[lgtm-com]

This pull request introduces 1 alert when merging 2d4d132 into 90ba627 - view on LGTM.com

new alerts:

• 1 for Wrong type of arguments to formatting function

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
5 Code Smells

0.0% Coverage
0.3% Duplication