Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEVICES: blepsynth/blepsynth.cc: introduce flexible ADSR model #6

Merged
merged 2 commits into from
Nov 11, 2023

Conversation

swesterfeld
Copy link
Collaborator

From the commit message:

  • volume envelope can now be analog (exponential) or flexible
  • flexible envelope has adjustable attack/decay/release slope
  • support changing volume/filter envelope params while note is playing

To be honest, the difference between analog and flexible envelope is not that big in practice. I could add a digital ADSR, but I guess since analog vs. flexible is not too different, it is probably not that important, flexible should cover configurable slope cases quite well.

Comments:

  • attack is exponential now too, in the analog version of the envelope (which was the initial starting point for this PR)
  • layout looks broken now, because Osc1 | Osc2 get different layout
  • volume envelope layout: A D S R should be in the same row, AS DS RS params should be inactive if model is analog
  • AS DS RS could somehow be graphically showing the slope
  • should we make filter envelope also flexible? - takes a bit more cpu power
  • speaking of cpu power, how can we test device cpu usage? new envelope could be somewhat more expensive since it does block processing? could also be cheaper? no idea?
  • do you want tools used to design polynomials in repo?
  • do you want tools used to test envelope in repo?

I've put the tools here: https://space.twc.de/~stefan/download/nadsr/ - if you want to I can clean these up a bit.

@swesterfeld
Copy link
Collaborator Author

As discussed previously, to support this:

AS DS RS params should be inactive if model is analog

commit 9b0dd6a now calls a set_parameter_used function to enable/disable the AS DS RS parameters; this should be implemented in the base class and propagate the used flag to the gui.

@tim-janik
Copy link
Owner

* volume envelope can now be analog (exponential) or flexible
* flexible envelope has adjustable attack/decay/release slope
* support changing volume/filter envelope params while note is playing

Thank you, that sounds very interesting.

* speaking of cpu power, how can we test device cpu usage? new envelope could be somewhat more expensive since it does block processing? could also be cheaper? no idea?

Ok, I'm working on finishing up a branch that supports headless and possibly offline rendering.
Maybe we should finish that first, so we can make performance assessments...

* do you want tools used to design polynomials in repo?
* do you want tools used to test envelope in repo?

As previously discussed, it would probably make sense to clean such design scripts up and put them into a dedicated "research" repo.


double level_ = 0;
a_ = slope * ( 1.0135809670870777f + slope * (-1.2970447050283254f + slope * 7.2390617313972063f));
b_ = slope * (-5.8998946320566281f + slope * ( 5.7282487210570903f + slope * -15.525953208626062f));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be interested to see some performance numbers if it really helps to make the calculations here carried out on floats instead of doubles...

Copy link
Collaborator Author

@swesterfeld swesterfeld Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a benchmark here:

https://github.com/tim-janik/dsp-research/blob/trunk/flexadsr/perffloat.cc

Results on my Ryzen 7:

ryzen7$ time perffloat float

real    0m0,555s
user    0m0,555s
sys     0m0,000s
ryzen7$ time perffloat double

real    0m0,657s
user    0m0,656s
sys     0m0,001s

My experience is that floats are not faster than doubles, but on modern systems conversion between floats and doubles has a performance cost. So since we in the inner loop of the ADSR we need floats for a_, b_ and c_, and since if we were modulating these parameters we would likely have a source buffer of floats, it is best to avoid converting to doubles and back for evaluating the polynomials.

Of course it could well be that for modulation we would evaluate the polynomials only every 16 samples (or so) then the conversion cost would not be too bad

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, on a 11th Gen Intel(R) Core(TM) i7-11800H @ 2.30GHz:

$ time ./perffloat "float"
real	0m0,364s
user	0m0,364s
$ time ./perffloat "double"         
real	0m0,455s
user	0m0,455s

@tim-janik
Copy link
Owner

Please update/rebase the patch.

@swesterfeld
Copy link
Collaborator Author

Please update/rebase the patch.

I've rebased the patch.

@swesterfeld
Copy link
Collaborator Author

  • new envelope could be somewhat more expensive since it does block processing? could also be cheaper? no idea?

Btw, I did some performance tests in SpectMorph and in the dsp-research repo. I also found a few opportunities to optimize the code. The result is that I wouldn't worry about the FlexADSR cpu usage at all, it is a lot cheaper than the other parts of the bleposc (namely the filters and the osc code).

@tim-janik tim-janik self-assigned this May 19, 2023
@tim-janik tim-janik added the next Next in line to be applied label May 19, 2023
@tim-janik tim-janik removed the next Next in line to be applied label Oct 27, 2023
@tim-janik
Copy link
Owner

Please rebase

@tim-janik tim-janik assigned swesterfeld and unassigned tim-janik Nov 1, 2023
 - volume envelope can now be analog (exponential) or flexible
 - flexible envelope has adjustable attack/decay/release slope
 - support changing volume/filter envelope params while note is playing

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
@swesterfeld
Copy link
Collaborator Author

Please rebase

Done. Great to see that you found some time to merge the other BlepSynth PR.

@swesterfeld swesterfeld assigned tim-janik and unassigned swesterfeld Nov 2, 2023
@tim-janik tim-janik added the next Next in line to be applied label Nov 10, 2023
@tim-janik tim-janik closed this in 409e87d Nov 11, 2023
@tim-janik tim-janik merged commit 409e87d into tim-janik:trunk Nov 11, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next Next in line to be applied
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants