Handle early the "set" commands from the configuration file.

[mchehab]

As discussed at issue #724, if no-realtime set commands are used inside
a configuration file, fluidsynth can be on an invalid state, where the synth will be using a different setting than the modules that are initialized after parsing the command.

In order to solve it, parse earlier the set commands.

There are 4 patches on this series.

Patch 1 changes the function which handles the set command: it will now verify if the value actually changed. That prevents printing realtime warnings when the value was not changed;

Patch 2 adds a new CMD parser that handles only set commands, and is called before initializing the synth;

Patch 3 avoid prints a warning when the commands from config files are parsed.

Patch 4 ensures that the new early parsing logic is only used for settings that can't be changed in real time. I'm not sure if this is needed, but it should prevent backward-compatibility issues, if some config file was using set for realtime parameters, and are relying on them being processed only after other commands. Probably overkill, but it shouldn't hurt to add this one to the series.

This solves issue #724. This replaces the approach taken at PR #725.

[lgtm-com]

This pull request introduces 1 alert when merging fc693b2 into 4bfeff5 - view on LGTM.com

new alerts:

• 1 for Comparison result is always the same

[lgtm-com]

This pull request introduces 1 alert when merging 04c98d3 into 4bfeff5 - view on LGTM.com

new alerts:

• 1 for Comparison result is always the same

[derselbst]

Ok, thanks. Patches 1 and 3 are fine. Patch 4 won't work, because fluid_settings_is_realtime only returns true after the settings object has been used for creating a synth. In your case the settings object has been freshly created, no settings have set a callback, hence it will always return false. Also, I don't see a reason why we would need patch 4 either.

And I don't quite like patch 2 because

• of its hackish fluid_synth_t synth = { .settings = settings }; approach, and
• it shadows the existing API around fluid_source and new_fluid_cmd_handler.

I'll make a counter proposal for it upcoming weekend (hopefully).

[mchehab]

Ok, thanks. Patches 1 and 3 are fine.

Good!

Patch 4 won't work, because fluid_settings_is_realtime only returns true after the settings object has been used for creating a synth. In your case the settings object has been freshly created, no settings have set a callback, hence it will always return false. Also, I don't see a reason why we would need patch 4 either.

Ok. Let's drop it.

And I don't quite like patch 2 because

* of its hackish `fluid_synth_t synth = { .settings = settings };` approach, and

* it shadows the existing API around `fluid_source` and `new_fluid_cmd_handler`.

I opted for this strategy due to a few reasons:

• It made the patch less intrusive, in the sense that the code won't change much, and that the already-existing logic will be called. So, no code duplication.
• As the new cmd handler will be called just to parse files (and not to be used on interactive sections), from API PoV, it seemed better to place everything (file open, file parse and free resource logic) altogether.
• From the PoV of the API callers, the arguments that makes more sense are fluid_settings_t *settings and char *config_file;
• Currently, fluid_source() expects that settings is inside synth struct. It is possible to add a settings struct at fluid_cmd_handler_t, in order to keep using fluid_source() without creating first fluid_synth_t, but this could break ABI compatibility if not done properly.
• From API PoV, I was not sure if fluid_cmd_handler_t is an opaque type or not. So, I opted to be more conservative, in the sense that the existing APIs won't be affected.

I'll make a counter proposal for it upcoming weekend (hopefully).

Ok. Whatever works best.

[derselbst]

I've implemented my original idea in #739. Briefly tested it, seems to work. Pls. have another test.

Currently, fluid_source() expects that settings is inside synth struct. It is possible to add a settings struct at fluid_cmd_handler_t, in order to keep using fluid_source() without creating first fluid_synth_t, but this could break ABI compatibility if not done properly.
From API PoV, I was not sure if fluid_cmd_handler_t is an opaque type or not. So, I opted to be more conservative, in the sense that the existing APIs won't be affected.

fluid_cmd_handler_t is defined in fluid_cmd.c itself. I don't see a possibility to break ABI here.

[mchehab]

I've implemented my original idea in #739. Briefly tested it, seems to work. Pls. have another test.

Worked fine, but I had to add this patch:

8c49121

More details on my comment to PR #739.

Currently, fluid_source() expects that settings is inside synth struct. It is possible to add a settings struct at fluid_cmd_handler_t, in order to keep using fluid_source() without creating first fluid_synth_t, but this could break ABI compatibility if not done properly.
From API PoV, I was not sure if fluid_cmd_handler_t is an opaque type or not. So, I opted to be more conservative, in the sense that the existing APIs won't be affected.

fluid_cmd_handler_t is defined in fluid_cmd.c itself. I don't see a possibility to break ABI here.

OK!

Yeah, this solution is indeed more elegant than this original PR.

[derselbst]

Worked fine, but I had to add this patch:

Ok, I see. This problem is not limited to settings commands. It could happen with router commands as well. I fixed that.

More details on my comment to PR #739.

Sorry, but there is no comment.

[mchehab]

Worked fine, but I had to add this patch:

Ok, I see. This problem is not limited to settings commands. It could happen with router commands as well. I fixed that.

More details on my comment to PR #739.

Sorry, but there is no comment.

Sorry! I forgot to click at the "submit review" button. Just submitted it.

Anyway, your solution of registering a NULL handler for the other commands seem more elegant. I'll re-test again with the new patch probably tomorrow.

[derselbst]

Superseded by #739.