[fluid-dev] Calls to fluid_log()

[Carlo Bramini]

Hello,
I have seen that these sources:

src\midi\fluid_seq.c
src\midi\fluid_seqbind.c
src\rvoice\fluid_chorus.c
src\synth\fluid_event.c

are calling fluid_log() function directly instead of calling FLUID_LOG macro.
Probably, they should be changed to FLUID_LOG, like in all other sources of FluidSynth.

What do you think?

Sincerely.

[Tom M.]

The question how to clean this up somewhat depends on fluid_synth_error(). This function returns the global buffer used by fluid_log and is therefore not thread safe. One could try to make it thread safe (perhaps with some fancy macro hack, havent come up with a proper solution though) or deprecate fluid_synth_error(), remove FLUID_LOG and call it directly.

Deprecating fluid_synth_error() should be ok, as the user can register custom logging functions anyway, that allow to use error messages arbitrarily.

Tom

[Carlo Bramini]

Hello,

Post by Tom M.
The question how to clean this up somewhat depends on fluid_synth_error(). This function returns the global buffer used by fluid_log and is therefore not thread safe. One could try to make it thread safe (perhaps with some fancy macro hack, havent come up with a proper solution though) or deprecate fluid_synth_error(), remove FLUID_LOG and call it directly.

In my opinion, it won't be a bad idea to keep the FLUID_LOG macro everywhere, because it allows to remove the log messages from the sources quite easily. The user can decide to use an empty macro or an option can be added to cmake, with something like an ENABLE_LOGGING macro inside config.h, in a similar manner it has been already done for profiling.

I also agree that fluid_synth_error() may be deprecated.

Sincerely.

[Tom M.]

Post by Carlo Bramini
In my opinion, it won't be a bad idea to keep the FLUID_LOG macro everywhere, because it allows to remove the log messages from the sources quite easily. The user can decide to use an empty macro or an option can be added to cmake, with something like an ENABLE_LOGGING macro inside config.h, in a similar manner it has been already done for profiling.

Why? If logging is not appreciated, one should disable it via the API,
rather than fiddling with the source code:

fluid_set_log_function(FLUID_DBG, NULL, NULL);
fluid_set_log_function(FLUID_WARN, NULL, NULL);
etc...


Tom

[Carlo Bramini]

Hello!

Post by Tom M.

Post by Carlo Bramini
In my opinion, it won't be a bad idea to keep the FLUID_LOG macro everywhere, because it allows to remove the log messages from the sources quite easily. The user can decide to use an empty macro or an option can be added to cmake, with something like an ENABLE_LOGGING macro inside config.h, in a similar manner it has been already done for profiling.

Why? If logging is not appreciated, one should disable it via the API,

That's true, the user can disable the logging by making NULL callbacks.
I was thinking that if the user want to remove completely the logging, he may be also able to do it by changing FLUID_LOG macro. 99% of the work is already done, there are just few points to fix, inside the sources that I listed in my first message. Since it comes for free, I do not see good reasons for not using FLUID_LOG macro also into those missing points. The extra option in cmake was just an idea that came out by looking the code, everything can stay as it is right now, if you want.

Sincerely.

[Tom M.]

Post by Carlo Bramini
Since it comes for free, I do not see good reasons for not using FLUID_LOG macro also into those missing points.

I don't see good reasons for keeping it. IMO it's redundant and could be removed. Yet, I would accept a PR where FLUID_LOG is called everywhere. But you should add one of those "good reasons" as comment pointing out why to keep and use the macro rather than a direct function call. Otherwise it might get removed in a future cleanup.

And "removing logging completely" is not quite the reasoning I was hoping for, as it raises the question why anyone should do that.

Tom

[Carlo Bramini]

Hello!

Post by Tom M.

Post by Carlo Bramini
Since it comes for free, I do not see good reasons for not using FLUID_LOG macro also into those missing points.

I don't see good reasons for keeping it. IMO it's redundant and could be removed. Yet, I would accept a PR where FLUID_LOG is called everywhere. But you should add one of those "good reasons" as comment pointing out why to keep and use the macro rather than a direct function call. Otherwise it might get removed in a future cleanup.
And "removing logging completely" is not quite the reasoning I was hoping for, as it raises the question why anyone should do that.

Well, for people who wants to strip a bit the size of the binary, I discovered that by commenting the FLUID_LOG, I'm able to reduce the size of the executable from 1.26MB to 1.18MB, without changing the current code. I tried to do also the few missing fixes into the sources that still need to replace fluid_log() with FLUID_LOG macro and I commented the content of the logging functions. The size decreased again from 1.18MB to 1.17MB, not so much but actually the biggest part of the code is already using FLUID_LOG macro, so it is normal.

So, it depends... if someone will like the idea to cut the size without affecting the the quality of the sound rendering (like me), probably he won't disregard the chance to have this FLUID_LOG macro. Besides the space gain and the fact that it is 99% already done, I do not other good reasons. Afterall, since logging is a slow code, it not called in vital points and removing it does not seem to provide performance improvements.

Sincerely.

[Ceresa Jean-Jacques]

Hi,

 

if someone will like the idea to cut the size without affecting the the quality of the sound rendering (like me),....

I like this idea too for future fanless embedded projects in mind (with high memory constraints).

In this regard, i hope this should be a good new. It seems it should be possible to reduce also the RAM size consumption by 588000 bytes (this is actually the maximum size of chorus's lfo table ).

This table (https://github.com/FluidSynth/fluidsynth/blob/7517c175240b31dd4470376575d9b4ac7cd2fba4/src/rvoice/fluid_chorus.c#L130) could be completelly removed and replaced by lfo wave fast computed on the fly (of course at the slight expense of cpu cycles (i hope)). I could propose a PR to address this "RAM size reduction" later (perhaps with further improvements (i.e stereo chorus)).

Cheers.

jjc

 

Message du 13/10/18 11:35
De : "Carlo Bramini"
A : "FluidSynth mailing list"
Objet : Re: [fluid-dev] Calls to fluid_log()
Hello!

Post by Tom M.

Post by Carlo Bramini
Since it comes for free, I do not see good reasons for not using FLUID_LOG macro also into those missing points.

I don't see good reasons for keeping it. IMO it's redundant and could be removed. Yet, I would accept a PR where FLUID_LOG is called everywhere. But you should add one of those "good reasons" as comment pointing out why to keep and use the macro rather than a direct function call. Otherwise it might get removed in a future cleanup.
And "removing logging completely" is not quite the reasoning I was hoping for, as it raises the question why anyone should do that.

Well, for people who wants to strip a bit the size of the binary, I discovered that by commenting the FLUID_LOG, I'm able to reduce the size of the executable from 1.26MB to 1.18MB, without changing the current code. I tried to do also the few missing fixes into the sources that still need to replace fluid_log() with FLUID_LOG macro and I commented the content of the logging functions. The size decreased again from 1.18MB to 1.17MB, not so much but actually the biggest part of the code is already using FLUID_LOG macro, so it is normal.
So, it depends... if someone will like the idea to cut the size without affecting the the quality of the sound rendering (like me), probably he won't disregard the chance to have this FLUID_LOG macro. Besides the space gain and the fact that it is 99% already done, I do not other good reasons. Afterall, since logging is a slow code, it not called in vital points and removing it does not seem to provide performance improvements.
Sincerely.
_______________________________________________
fluid-dev mailing list
https://lists.nongnu.org/mailman/listinfo/fluid-dev

[Tom M.]

for people who want to strip a bit the size of the binary, I discovered that by commenting the FLUID_LOG, I'm able to reduce the size of the executable from 1.26MB to 1.18MB

Ok then, let's use FLUID_LOG. Feel free to draft a PR and add this hint regarding reducing the binary size as comment to the FLUID_LOG macro.

This table (https://github.com/FluidSynth/fluidsynth/blob/7517c175240b31dd4470376575d9b4ac7cd2fba4/src/rvoice/fluid_chorus.c#L130) could be completelly removed and replaced by lfo wave fast computed on the fly (of course at the slight expense of cpu cycles (i hope)). I could propose a PR to address this "RAM size reduction" later (perhaps with further improvements (i.e stereo chorus)).

Yes please, you're welcome.

Tom