[yoshimi] Re: How to deal with that problem?

  • From: Will Godfrey <willgodfrey@xxxxxxxxxxxxxxx>
  • To: yoshimi@xxxxxxxxxxxxx
  • Date: Mon, 28 May 2018 18:47:46 +0100

On Mon, 28 May 2018 13:30:01 +0200
Ichthyostega <prg@xxxxxxxxxxxxxxx> wrote:

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

Just got back from a nice long walk after getting that release out (it's
surprising how much 'housework' is needed for it).

Anyway some extra info.

The original problem that had so worried Cal only seemed to show up with Intel
Atom N270 processors, this is why hardly any of us could reproduce it. It
apparently resulted in a weird note pitch error.

I agree that having (potentially) different rounding behaviour is dangerous. I
never was happy about the LV2/standalone split, but didn't feel I knew enough
about it to make changes. I am concerned about handing what we to *internally*
over to an external program. As I said before, I got the feeling Ardour was
doing something different to everyone else. I'm not saying that wrong -
I don't know. It may be that we are making assumptions we're not entitled to.

There has always been a mixture of float management. I personally believe
adjustments should go to the nearest, on the basis that hopefully over time
errors would average out - however I still don't know enough to be confident on
this.

In the GUI there has been a mixture of int() and lrint() float conversions.
I've been changing them all to lrint() as I come up against them. My reasoning
is that if that turns out to be wrong it's only one type that has to be
modified.

In particular where on/off switches were involved, I got so confused about all
the potential errors I ended doing a little dance of:

bool {someswitch} = {somefloat} > 0.5f;

I fully accept that may be overkill!

It appears a lot of FLTK returns are floats even when quite unnecessary.

-- 
Will J Godfrey
http://www.musically.me.uk
Say you have a poem and I have a tune.
Exchange them and we can both have a poem, a tune, and a song.
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: