Created attachment 86990 [details] [review]
DavOrgan port, patch part

Changes (against r4335) necessary to make the C++ version of DavOrgan work.

Created attachment 86991 [details]
Actual C++ implementation

C++ plugin

Created attachment 86992 [details]
DavOrgan IDL file

ok, let's talk about the FIXMEs first then, the ones that can be fixed right now, afaics are:

+ /* FIXME: warning without cast */
please elaborate here, it's not clear from the comment what needs to be fixed. e.g. describe the warning, and if possible why it's triggered, then we can discuss resolutions.

+/* FIXME: we discussed about the wording, but I don't remember the conclusion; original (C) = "Organ frequency as note, converted to Hertz according to the current musical tuning"??? */
beast/plugins/ has lots of examples to copy the wording from, here's one:
  "Drum frequency as note, converted to Hertz according to the current musical tuning".

(In reply to comment #4)
> ok, let's talk about the FIXMEs first then, the ones that can be fixed right
> now, afaics are:
> 
> + /* FIXME: warning without cast */
> please elaborate here, it's not clear from the comment what needs to be fixed.
> e.g. describe the warning, and if possible why it's triggered, then we can
> discuss resolutions.

It has been a warning about implicit floating point to integer conversion, I meanwhile remembered about Birnet::dtoi32. So I resolved this by replacing

    freq_256 = BSE_FREQ_FROM_VALUE (ifreq[0]) * transpose * fine_tune * 256 + 0.5;

with

    freq_256 = dtoi32 (BSE_FREQ_FROM_VALUE (ifreq[0]) * m_transpose_factor * m_fine_tune_factor * 256);

> 
> +/* FIXME: we discussed about the wording, but I don't remember the conclusion;
> original (C) = "Organ frequency as note, converted to Hertz according to the
> current musical tuning"??? */
> beast/plugins/ has lots of examples to copy the wording from, here's one:
>   "Drum frequency as note, converted to Hertz according to the current musical
> tuning".
> 

Ok, I now use the wording we use elsewhere (as in DavSynDrum). New patch follows.

Created attachment 87083 [details] [review]
DavOrgan C++ification, new patch

Patch with improvements (as described in comment #5). I've included all files in the patch now (instead of uploading 3 files), as suggested on irc.

(In reply to comment #5)
> (In reply to comment #4)
> > ok, let's talk about the FIXMEs first then, the ones that can be fixed right
> > now, afaics are:
> > 
> > + /* FIXME: warning without cast */
> > please elaborate here, it's not clear from the comment what needs to be fixed.
> > e.g. describe the warning, and if possible why it's triggered, then we can
> > discuss resolutions.
> 
> It has been a warning about implicit floating point to integer conversion, I
> meanwhile remembered about Birnet::dtoi32. So I resolved this by replacing
> 
>     freq_256 = BSE_FREQ_FROM_VALUE (ifreq[0]) * transpose * fine_tune * 256 +
> 0.5;
> 
> with
> 
>     freq_256 = dtoi32 (BSE_FREQ_FROM_VALUE (ifreq[0]) * m_transpose_factor *
> m_fine_tune_factor * 256);

first, note that dtoi() is a method of class SynthesisModule, so you're not calling a Birnet:: function here. second, bse_dtoi() implements *rounding*, you can't just pick *any* float->int conversion and expect it to magically do what you want. what is the intended behavior here? if the code was literally copied from C, you probably need truncating, not rounding. take a look at birnetmath.hh, that file implements the various conversions. if you're worried about Birnet::ceil/floor/round still calling math.h functions, you could also use something like dtoi32(f-0.5), but in any case, *know* what you're doing. putting a single line comment next to the code that describes the desired intention would also help. (e.g. "round away from 0.5 to yield the closest match for interpolation" or "truncate towards 0 to never overflow array index" etc.)

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > ok, let's talk about the FIXMEs first then, the ones that can be fixed right
> > > now, afaics are:
> > > 
> > > + /* FIXME: warning without cast */
> > > please elaborate here, it's not clear from the comment what needs to be fixed.
> > > e.g. describe the warning, and if possible why it's triggered, then we can
> > > discuss resolutions.
> > 
> > It has been a warning about implicit floating point to integer conversion, I
> > meanwhile remembered about Birnet::dtoi32. So I resolved this by replacing
> > 
> >     freq_256 = BSE_FREQ_FROM_VALUE (ifreq[0]) * transpose * fine_tune * 256 +
> > 0.5;
> > 
> > with
> > 
> >     freq_256 = dtoi32 (BSE_FREQ_FROM_VALUE (ifreq[0]) * m_transpose_factor *
> > m_fine_tune_factor * 256);
> 
> first, note that dtoi() is a method of class SynthesisModule, so you're not
> calling a Birnet:: function here. second, bse_dtoi() implements *rounding*, you
> can't just pick *any* float->int conversion and expect it to magically do what
> you want. what is the intended behavior here? if the code was literally copied
> from C, you probably need truncating, not rounding.

You may not have noticed, but if you reread the code I quoted, you'll see that the original code was XXX = YYY + 0.5, whereas my code is XXX = dtoi32 (YYY) - so I did realize that rounding is done by dtoi32.

> putting a single line comment next to the code that describes the desired
> intention would also help. (e.g. "round away from 0.5 to yield the closest
> match for interpolation" or "truncate towards 0 to never overflow array index"
> etc.)

Now that you mention it, I think there is a problem in the original code: if there is a negative signal, then using an unsigned variable to store the array index, and then perform truncation with

      *paccu += freq_256;
      while (*paccu >= mix_freq_256)
        *paccu -= mix_freq_256;

will be expensive. I tried it, and one DavOrgan voice with a normal frequency will take 1.0% computation power, whereas with a negative frequency it takes about 3.0%. Hm. I think either CLAMPing to a range like [0, 20000] should be used, or ABS. Do we have a policy for what happens if you use a negative frequency as input for at a module? If our policy was like: it will break, but we don't guarantee for anything (except maybe that it doesn't crash or eat unreasonable amounts of CPU power), then CLAMP would be ok.

The problem of CLAMP is that you wouldn't hear a negative frequency (good), but it would usually generate a DC offset (bad). The problem with ABS is that you it wouldn't generate a DC offset (good) but that maybe you wouldn't realize that you did something bad to the frequency variable, which could lead to problems if you exchanged DavOrgan with another module later on (like DavXTalStrings) which doesn't handle negative frequencies.

Personally I would go for CLAMPing to the [0, 20000] range, and decide that our policy is that if you use a negative frequency as input for a module, it doesn't need to do something reasonable.

Created attachment 87733 [details] [review]
DavOrgan C++ification with frequency range code

After our face to face discussion on the subject of negative frequencies, I made the code handle negative frequencies using fabs rather than CLAMP, to avoid DC offsets.

Another way to avoid DC offsets would have been to add a multiplication factor for the output signal that is 1.0 for everything between 20 and 18000 Hz, and fades out the signal in the 20..0 Hz range as well as the 18000..mix_freq/2 range. The implementation complexity of this made me choose the other approach.

As for frequencies > mix_freq/2, the new implementation now ensures that all of them get mapped to mix_freq/2 (this is like the CLAMP thing I suggested before). I didn't clip at 20000 Hz (as I originally intended), because you might as well run beast with sample rates much higher than 48 kHz, and then, generating frequencies > 20000 Hz may make sense.

The DavOrgan C++ code has been merged 20110508, so I'm closing the bug.