[yoshimi] How to deal with that problem?
- From: Ichthyostega <prg@xxxxxxxxxxxxxxx>
- To: yoshimi@xxxxxxxxxxxxx
- Date: Mon, 28 May 2018 13:30:01 +0200
Hello Will and the others,
sorry for the long mail....
It seems this problem is just the tip of the iceberg and indicates something
we maybe should reconsider (after the release of course). Can anyone recall
any additional details why these global changes in the float rounding behaviour
where introduced?
After some poking around, I've found the following pieces:
Release-Notes to yoshimi-0.060.7-1 (early in 2011)
Still basically just 0.058.1 plus Paul's unison amd reverb enhancements, and
jack session support. The 0.060.X series have been plagued by a strange issue
affecting some but not all 32 bit builds. On some 32 bit systems, ADnotes were
incorrectly pitched. Lars Luthman has identified the problem and the fix.
0.060.7-1 includes just that fix, so with some further testing, maybe we can
all move on :-). All hail Lars Luthman!
Judging from the Git history, Cal started to investigate that problem
starting with 47972f877 (17.12.10). In one of the changesets, I found a note
that he himself did not see the problem on his own machine, but seemingly others
did.
During the next few months, Cal did a lot of clean-up and quite some changes
which give me the impression of someone fishing in the dark. E.g. he added
"f" suffixes to various constants all over the code base. Some of these
where real errors, like in the microtuning code, others did just change
the calculations from double -> float, which means to move the rounding
from the end of the calculation (when re-assigning back into a float)
into the middle of the calculation (e.g. within a division).
Moreover, during that period, Cal introduced the function float2int,
at that time written as:
// For rational, see <
http://www.mega-nerd.com/FPcast/>
inline const int Float2Int::float2int(const float val) const
{ return ((val > 0.0f) ? lrintf(val) : lrintf((val - 1.0f))); }
Actually, something similar was already used inline at some of the
locations touched by this debugging, while other locations used a naive
cast to int. Thus, on the bright side, we have now one single function
to reason about.
However, I came across his kind of functions and the accompanying mega-nerd tip
several times in those past years -- and it always turns on all red warning
lights for me. There are two subtle problems with such functions: for one
the non-obvious dependency on the fp rounding mode, and, also quite often
people are not aware of introducing an asymmetry in the value domain.
Back to topic!
Release 0.060.7-0 came on 19.3.2011
And the /very next commit/, on evening of 19.3.2011 introduced the
global change of the rounding mode in the constructor of Config(),
which causes the runaway problem with float parameters in XML files.
== Fast-Forward to Today ==
Meanwhile the conversion function has been changed to truncate towards Zero:
inline int Float2Int::float2int(const float val) const
{ return (int)((isgreater(val, 0.0f)) ? (int)truncf(val)
: (int)truncf((val - 1.0f))); }
// for rationale, see <
http://www.mega-nerd.com/FPcast/>
...which means, just for *that* function, we would not need the changed rounding
mode anymore. It just truncates, and it is asymmetric at Zero, which causes a
problem with the Vowel sequences in the Formant filter, if I understand correct
(I hoped to be able to discuss that in detail with you, Will, at LAC in Berlin).
However, a quick grep shows that we still do use lrintf() at various places,
most in the GUI, but I see a hit at least in SynthEngine.cpp and Reverb.cpp
But one further concern bogged me: We have now this split in Config(),
where the LV2 plugin uses a different rounding mode than the standalone Yoshimi.
This change was introduced with
35b2e9f7
Andrew Deryabin<andrew@xxxxxxxxxxxxxxxx> 15.01.15 18:25
2015-1-15 Andrew
Don't change floating point rules in case of lv2 plugin. Hosts should do it.
There is that interesting comment by Andrew Deryabin in Config.cpp
fesetround(FE_TOWARDZERO);
//^^^^^^^^^^^^^^^ This call is not needed aymore (at least for lv2 plugin)
//as all calls to lrintf() are replaced with (int)truncf()
//which befaves exactly the same when flag FE_TOWARDZERO is set
For my gut feeling, this looks like a typical anti pattern I've encountered
in various code reviews. Andrew was knowledgeable regarding the LV2 plug-in
and felt confident to judge this half of the picture. However, he probably
shunned to do the change globally, since he didn't understand possible
interferences with the GUI.
But the net result is quite messy: now we have an *Engine*, which does its
*calculations* differently when used as LV2 plugin. This is totally wrong.
Because the one-and-only thing I really expect from a plug-in is, that
it uses the very same engine than the stand-alone executable does.
So my conclusion and *proposal* is:
(a) either we revert the change of Andrew, and use the FE_TOWARDZERO for the
LV2-Plugin as well, where it should (according to Andrew's reasoning)
make no difference (--> and review Reverb.cpp and SynthEngine.cpp)
(b) or we follow up with Andrew's reasoning and get rid of the FE_TOWARDZERO
altogether, in the whole code base. Which means, to round towards the
closest next grid value (which is the default). This would mean to judge
the ramifications for parameter value display in the GUI.
-- Hermann
Yoshimi source code is available from either:
https://sourceforge.net/projects/yoshimi
Or:
https://github.com/Yoshimi/yoshimi
Our list archive is at:
https://www.freelists.org/archive/yoshimi
To post, email to yoshimi@xxxxxxxxxxxxx
Other related posts:
