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

new reverb #380

Merged
merged 29 commits into from
Dec 1, 2018
Merged

new reverb #380

merged 29 commits into from
Dec 1, 2018

Conversation

jjceresa
Copy link
Collaborator

@jjceresa jjceresa commented May 7, 2018

This PR proposes a new reverb which is a late reverb (like the actual reverb) which is convenient for Fluidsynth sound engine.
The parameters are the same than the actual reverb. Also the default response of these parameters are rather the same.
The intended goal was:

  • producing a better quality reverberation tail than actual reverb with far less "ringing" using rather no more cpu load if possible.
  • reducing the memory size consumed by the delay lines ( results give: -110 kbytes(-54%) to -144 kbytes(-72%)).

The macro NBR_DELAYS allows to choose 8 or 12 delays lines.
Although 8 lines give good result, using 12 delay lines brings the overall quality a bit higher. This quality augmentation is noticeable particularly when using long reverb time (roomsize = 1) on solo instrument with long release time. Of course the cpu load augmentation is +50% relatively to 8 lines.

As a general rule the reverberation tail quality is easier to perceive by ear when using:

  • percussive instruments (i.e piano and others).
  • long reverb time (roomsize = 1).
  • no damping (damp = 0).

@mawe42
Copy link
Member

mawe42 commented May 7, 2018

Cool, looking forward to testing this! Have you done any performance measurements yourself?

@jjceresa
Copy link
Collaborator Author

jjceresa commented May 7, 2018

Have you done any performance measurements yourself?

Yes, using NBR_DELAYS = 8 (default) you should expect rather the same performance than actual reverb.
The default MACROS should be sufficient to give an immediate possibility of compare with actual reverb.
The full description of compare with the actual reverb is given in a table in the top of fluid_rev.c.

@mawe42
Copy link
Member

mawe42 commented May 7, 2018

Would it maybe be beneficial to be able to disable the denormal handling with a compile-time switch and rely on -ffast-math (and maybe also -msse -mfpmath=sse)? See http://carlh.net/plugins/denormals.php for more info.

@jjceresa
Copy link
Collaborator Author

jjceresa commented May 7, 2018

Thanks for the link.

Would it maybe be beneficial to be able to disable the denormal handling ?

Actual denormal handling introduces a loss of -6% of the reverb cpu load (i.e 20 % of only one voice cpu load ) (on cpu with math co-processor).
This should be tested on others platform. I haven't an ARM without math co-processor . May be it worth to test this ?

@derselbst
Copy link
Member

Nice work. For me a difference is barely audible. Hope this is intended?

@jjceresa
Copy link
Collaborator Author

jjceresa commented May 7, 2018

For me a difference is barely audible.

Please, could you develop a bit which difference ?

@derselbst
Copy link
Member

Please, could you develop a bit which difference ?

No, because I dont really hear a difference (barely == almost not).

@mawe42
Copy link
Member

mawe42 commented May 7, 2018

With a roomsize of 1, I can hear quite a difference. The existing reverb has a lot of high frequency ringing, probably an artifact of the comb filters. The new implementation has no unnatural ringing that I can hear. So much better in that regard!

But the new implementation has more "wobble" than the old one. I set roomsize to 1 and leave all other settings unchanged. Then I play a single short note of piano and listen to the reverb. The new reverb sounds a little bit like there is a phaser or chorus on it. The existing reverb sounds cleaner and more natural, in my opinion. But maybe it's just that the default parameters need tweaking?

@derselbst
Copy link
Member

But the new implementation has more "wobble" than the old one.

Yes, the only difference I noticed so far. I think it's the sinus_modulator built into each delay line. A little less "wobble" might be good.

@jjceresa
Copy link
Collaborator Author

jjceresa commented May 8, 2018

The existing reverb has a lot of high frequency ringing,probably an artifact of the comb filters.

Any reverb with only 8 comb filters will produces audible resonant frequencies (ringing). This is due to the fact that there not enough numbers of resonant frequencies and the frequency are heard individually. If the reverb will have at least 30 combs filters (with sufficient lengths) (as it should have !) the resonant frequencies will be so near each other that they could not be heard individually.

The new implementation has no unnatural ringing that I can hear.

Using only 8 comb filters a possible solution to cancel the resonant frequencies (without putting damp to >0) is to modulate a bit the comb filter (this simulates more peaks of frequency moving very near each to other). This allows to hear the full spectrum when intended (putting damp to 0) without hearing the building of unwanted resonant frequencies.

The new reverb sounds a little bit like there is a chorus on it.

I think it's the sinus_modulator built into each delay line

This is the inconvenient of a modulated delay line. It produces an unwanted frequency modulation (like chorus). However if the modulation depth (MOD_DEPH) is very low(< 6 samples) and the frequency of modulation (MOD_FREQ) is very low( < 2 Hz) but sufficient ( > 0.5 Hz) this should be acceptable when we intend to listen the full spectrum (damp to 0).
When using damp to > 0, we intend to diminish the reverb time of high frequency and in this case the unwanted chorus becomes more perceptible (i.e less acceptable).

A little less "wobble" might be good.

In summary:
When full spectrum is intended (damp at 0):

  • Using NBR_DELAYS to 8, MOD_DEPTH must be > 4 to cancel the unwanted "ringing".
  • Using NBR_DELAYS to 12, MOD_DEPTH to 3 is sufficient to cancel the unwanted "ringing".

When less reverb time is intended for high frequency (damp > 0):

  • high frequency are less often heard so maybe MOD_DEPTH could be diminished a bit. So may be MOD_DEPTH should be dependent of damp to get acceptable chorus.

Will see what can be done.

@jjceresa
Copy link
Collaborator Author

jjceresa commented May 8, 2018

Since now modulation depth (MOD_DEPTH) gives a compromise between both the unwanted resonant frequencies ("ringing") and the unwanted chorus. However, the chosen value depend also of the instrument under reverberation. So it appears that a fixed value for MOD_DEPTH isn't appropriate.

Although i am fan of less parameters as possible, i think it worth that modulation depth should be chosen by the user via a new parameter that could called moddepth . Any thoughts ?

@derselbst
Copy link
Member

One important issue: I'm missing a copyright notice. Did you wrote this completely by yourself or is it based on some implementation? If you did it yourself, please add your name + copyright + hint for LGPL 2.1+.

i think it worth that modulation depth should be chosen by the user via a new parameter that could called moddepth

Wouldnt go that far for now. Let us continue rehearsal first.

@jjceresa
Copy link
Collaborator Author

jjceresa commented May 9, 2018

One important issue: I'm missing a copyright notice. Did you wrote this completely by yourself or is it based on implementation?.

All the concepts and ideas comes completely from all the research papers we found on w.w.w on this subject since 40 years. In this source there are no ideas or concepts that come from me. I am responsible of the translation of theses ideas in C language.
I have chosen a suited reverb model described here:

I must admit that my knowledge about copyright statements is very limited. Also i don't intend to claim any copyright of my own. You can probably help on this copyright subject.

@derselbst
Copy link
Member

Right, I'm aware that the concepts exists and are public domain as it seems. I was only referring to the source code.

I am responsible of the translation of theses ideas in C language.

So does that mean that you wrote the source code completely yourself, without using or translating any existing source? In this case, just add the existing copyright notice found in fluidsynths other source files to the top of fluid_rev.c, replacing Peter Hanappe with your name. This is just to make evident that you (the creator and therefore copyright holder of this source code) license this code to fluidsynth under LGPL2.1+.

...Simply to avoid this issue we had at last year with Juergen Muellers chorus implementation.

@jjceresa
Copy link
Collaborator Author

jjceresa commented May 9, 2018

Let us continue rehearsal first.

Sure. Here the conditions and results of many experiments:
Conditions:
For many instruments under reverberation (roomzize=1, damp=0). Only one instrument for each test:

  • Minimum Keyboard range (if possible) C2 to C6 octaves. A simple test is playing each white key consecutively (one pass at low velocity, then another pass at high velocity).
  1. 2 pianos (one percussive then one less percussive).
  2. Then no percussive instrument ( oboe, flute,sax,..)

Results.
For many instruments, the range for MOD_FREQ and MOD_DEPTH should be:

  • MOD_FREQ: [0.5 ..2.0] (in Hz).
  • MOD_DEPTH: [3..6] (in samples).

Values below the lower limits are often not sufficient to cancel unwanted "ringing"(resonant frequency).
Values above upper limits augment the unwanted "chorus".

@jjceresa
Copy link
Collaborator Author

jjceresa commented May 9, 2018

So does that mean that you wrote the source code completely yourself, without using or translating any existing source?

Not completely because i have read a lot of research papers and read in this papers examples of implementations written in others languages. So in this regard the source i have written (3 years ago) is based on the works of the author of this papers.
So if i understand correctly the copyright rules, we should pay attention to mention the initial authors of this initial works (as it is the case in the actual fluidsynth fluid_rev.c that mention: "Written by Jezar...." translated to C by Peter Hannape) ? . am i correct ?.

@derselbst
Copy link
Member

So if i understand correctly the copyright rules, we should pay attention to mention the initial authors of this initial works

Yes, the example implementations that you've used must be credited and they must compatible with LGPL (e.g. public domain).

I'm not sure if it's necessary to do this that detailed. If the examples you've used are part of the papers you've already cited it's probably ok to silently use them by translating them to C. Just look them through again so we dont miss anything.

In every case I'd suggest to apply fluidsynth's copyright note.

@jjceresa
Copy link
Collaborator Author

jjceresa commented May 9, 2018

I'm not sure if it's necessary to do this that detailed. If the examples you've used are part of the papers you've already cited it's probably ok to silently use them by translating them to C. Just look them through again so we dont miss anything.

I have looked in the papers. The code examples i have read are just only part of the papers. There are no copyright mention, no licence mention. There are only the author name of the paper.

Should be a good idea to contact the author of the papers ?

In every case I'd suggest to apply fluidsynth's copyright note.

You mean, of course just add the existing copyright notice found in fluidsynths other source files to the top of fluid_rev.c.

@derselbst
Copy link
Member

Should be a good idea to contact the author of the papers ?

No, I dont think that's necessary.

You mean, of course just add the existing copyright notice found in fluidsynths other source files to the top of fluid_rev.c.

Yes please.

{
FLUID_FREE(allpass->buffer);
#ifdef WITH_DENORMALISING
FLUID_MEMSET(dl->line, DC_OFFSET, dl->size * sizeof(fluid_real_t));
Copy link
Member

Choose a reason for hiding this comment

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

memset sets bytes, not floats. You have to use a for loop.

{
comb->damp1 = val;
comb->damp2 = 1 - val;
long i;
Copy link
Member

Choose a reason for hiding this comment

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

Why long?

/*
Number of samples to add to the desired length of a delay line. This
allow to take account of modulation interpolation.
1 is sufficient with MOD_DEPTH egal to 6.
Copy link
Member

Choose a reason for hiding this comment

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

equal to

Modulator for modulated delay line
-----------------------------------------------------------------------------*/
#define TWO_PI 6.28318530717958647692528676655901
#define PI_DIVIDED_BY_2 1.57079632679489661923132169163975
Copy link
Member

Choose a reason for hiding this comment

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

I dont think these defines are necessary. We could just write (2*M_PI) and (M_PI/2.0) below respectively. Constant folding will remove the multiplication at compile time.

/*-------------------------*/
/* variable rate control of center output position */
long index_rate; /* index rate to know when to update center_pos_mod */
long mod_rate; /* rate at which center_pos_mod is updated */
Copy link
Member

Choose a reason for hiding this comment

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

Why using long for those members?

}

FLUID_FREE(rev);
delete_fluid_rev_late(&rev->late);
Copy link
Member

Choose a reason for hiding this comment

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

Please add NULL-guards to destructor functions: fluid_return_if_fail(rev != NULL);

}
else
{
FLUID_FREE(rev);
Copy link
Member

Choose a reason for hiding this comment

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

Call delete_fluid_revmodel() pls.

delay_length[i], MOD_DEPTH, MOD_RATE );
if (result == FLUID_FAILED)
{
delete_fluid_rev_late(late);
Copy link
Member

Choose a reason for hiding this comment

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

Remove this delete_* call pls. Cleanup should be done by parent constructor function new_fluid_revmodel().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All cleanup done.

Please add NULL-guards to destructor functions: fluid_return_if_fail(rev != NULL);

Testing rev to not NULL but not using fluid_return_if_fail(), as this is an internal reverb interface API , not a public API. Hope it is correct ?

Copy link
Member

Choose a reason for hiding this comment

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

as this is an internal reverb interface API , not a public API. Hope it is correct ?

That does the same thing ofc, but it looks more consistent if you used fluid_return_if_fail(). I've done that for all destructor functions some time ago.

Also please add the NULL-guard to delete_fluid_rev_late(). I know, it should never be called with NULL, but I prefer to have those destructors as failsafe as possible.

@jjceresa
Copy link
Collaborator Author

jjceresa commented Jun 4, 2018

It looks like your tweaks were correct, now it sounds better to me.

Thanks for checking.

However your latest change has made the wobble become more dominant again
I suggest to reduce the MOD_RATE to 4, which still sounds fine to me.

You mean probably about MOD_DEPTH change (not MOD_RATE) ?

Do you agree?

Talking about MOD_DEPTH, of course i agree because it depends of the instrument, ears and taste. So it can be reduced to 4.

@jjceresa
Copy link
Collaborator Author

jjceresa commented Jun 7, 2018

However your latest change has made the wobble become more dominant again.

Please, as i dont know how to reproduce the "wobble" you have heared, i guess (and hope) the latest change you refer (while the wobble domination augmentation was observed) is Change on internal gain scale(dead0ef) ?.
Thanks for confirming this previous observation. (This will help to note the interaction or singularity between presumed distinct inner parameters).

@derselbst
Copy link
Member

Yes I was referring to the internal gain scale. Specifically audible for high frequent samples. Here's the midi demo attached. Use it with this soundfont.

DK64-JJ-Underground.zip

@derselbst
Copy link
Member

All in all I'm fine with this. Ringing is gone, code is well documented, tiny wheeny drawback is the whobble, but that's only audible for a very few sounds, so nevermind. However I dont want to merge this for 2.0 as it already brings a lot new features. Instead I want to keep this for whatever comes after 2.0.

@jjceresa Could you perhaps sooner or later bring this feature to the mailing list?

@derselbst derselbst mentioned this pull request Jun 21, 2018
@jjceresa
Copy link
Collaborator Author

jjceresa commented Jun 22, 2018

tiny wheeny drawback is the whobble, but that's only audible for a very few sounds, so nevermind.

Yes, i understand. This compromise between ringing tone and wobble (know also as flutter) is really difficult and doesn't work in all situation.

However I dont want to merge this for 2.0 as it already brings a lot new features. Instead I want to keep this for whatever comes after 2.0.

Yes of course. I understand this.
Also tweaking isn't not yet finished, i have noticed that:

  • low input gain (FIXED_GAIN < 1.0) should be reintroduced.
  • suppressed stereo gain should be reintroduced.
    Both these should enhance flatness of frequency response.

Could you perhaps sooner or later bring this feature to the mailing list?

Yes, i think it is necessary that more ears give a try. For example, i already know that jOrgan-user was interested to try this. Graham Goode (jOrgan-user) have said that it should be easier for them to try a fluidsynth executable (Windows). Did you know if CI travis is able to build executable of new-reverb branch ? (i don't remember).

@derselbst derselbst modified the milestones: 2.0-post, 2.1 Jun 24, 2018
derselbst and others added 2 commits June 25, 2018 17:19
 1)low input gain (FIXED_GAIN = 0.1) reintroduced.
   FIXED_GAIN is lowered from 1.0 to 0.1.
   SCALE_WET augmented from 0.6 to 5.
 2)stereo gain reintroduced.
 Both enhances flatness comb filter frequency response.
@derselbst
Copy link
Member

Did you know if CI travis is able to build executable of new-reverb branch ? (i don't remember).

Sorry, just realized I haven't respond to this. Travis and AppVeyor continiously build all branches that are being pushed to. Currently Travis does not produce build artifacts. However as it seems you want windows binaries, you can just use the build artifacts from AppVeyor, e.g.: https://ci.appveyor.com/project/derselbst/fluidsynth/build/1192/job/8adfyve7lrsfgw4g/artifacts

@jjceresa
Copy link
Collaborator Author

However as it seems you want windows binaries, you can just use the build artifacts from AppVeyor.

I already know that jOrgan-user was interested to try this. Graham Goode (jOrgan-user) have said that it should be easier for them to try a fluidsynth executable (Windows).

Note that the binaries build by AppVeyor doen't run on Windows XP. These probably run on Windows 7 and above.

@derselbst
Copy link
Member

Note that the binaries build by AppVeyor doen't run on Windows XP.

I've adjusted the visual studio toolset to be windows xp compatible (hopefully). Could you please try this build:

https://ci.appveyor.com/project/derselbst/fluidsynth-g2ouw/build/970/job/ut2rvpvic96bftfl/artifacts

@jjceresa
Copy link
Collaborator Author

Could you please try this build:https://ci.appveyor.com/project/derselbst/fluidsynth-g2ouw/build/970/job/ut2rvpvic96bftfl/artifacts

This build starts on XP but unfortunately it introduces new unknow dlls (with a very complicated dlls dependencies). A lot of these are missing (listed in upper case). This dependency is:

libfluidsynth-2.dll depends of:

  • glib-2.dll , and missing dll
    VCRUNTIME140.DLL, API-MS-WIN-CRT-HEAP-L1-1-0.DLL, API-MS-WIN-CRT-STRING-L1-1-0.DLL, API-MS-WIN-CRT-MATH-L1-1-0.DLL, API-MS-WIN-CRT-CONVERT-L1-1-0.DLL, API-MS-WIN-CRT-STDIO-L1-1-0.DLL, API-MS-WIN-CRT-UTILITY-L1-1-0.DLL, API-MS-WIN-CRT-RUNTIME-L1-1-0.DLL.

glib-2.dll depends of:

  • pcre.dll, libiconv.dll, libint.dll and missing dll:
    API-MS-WIN-CRT-TIME-L1-1-0.DLL, API-MS-WIN-CRT-LOCALE-L1-1-0.DLL, API-MS-WIN-CRT-FILESYSTEM-L1-1-0.DLL, API-MS-WIN-CRT-ENVIRONMENT-L1-1-0.DLL, , API-MS-WIN-CRT-PROCESS-L1-1-0.DLL, API-MS-WIN-CRT-MATH-L1-1-0.DLL.
  • and some missing dll already listed above

pcre.dll depends of: some missing dll already listed above

libint.dll depends of:

  • libcharset.dll and some missing dll already listed above.

On my own build for XP the classic minimum know dependency is:

libfluidsynth-2.dll depends of:

  • glib dll (manually installed): LIBGTHREAD-2.0-0.DLL, LIBGLIB-2.0-0.DLL, INTL.DLL,
  • windows dll (already part of OS):DSOUND.DLL (audio API) ,WINMM.DLL (MIDI driver API), WS2_32.DLL (API), KERNEL32.DLL(API), USER32.DLL(API), MSVCR100.DLL (runtime library).

Note that i am not looking for AppVeyor binaries for my own. However, this could be useful for others users (not familiar with compilation) for a quick try.
I don't know if others people (may be Marcus @mawe42 or Carlo @carlo-bramini ) have already ran AppVeyor windows binaries on Win 7, 8, 10 ?

@derselbst
Copy link
Member

@jjceresa
Copy link
Collaborator Author

jjceresa commented Jul 29, 2018

Strange. Please try this one as well: https://ci.appveyor.com/project/derselbst/fluidsynth/build/job/bdbhogdn4v1qwqda/artifacts

For this build, libfluidsynth-2.dll now depends of the same dll that for my build but unfortunately still dependent of missing dll:
VCRUNTIME140.DLL, API-MS-WIN-CRT-HEAP-L1-1-0.DLL, API-MS-WIN-CRT-STRING-L1-1-0.DLL, API-MS-WIN-CRT-MATH-L1-1-0.DLL, API-MS-WIN-CRT-CONVERT-L1-1-0.DLL, API-MS-WIN-CRT-STDIO-L1-1-0.DLL, API-MS-WIN-CRT-UTILITY-L1-1-0.DLL, API-MS-WIN-CRT-RUNTIME-L1-1-0.DLL.

Please, see
dll_dependency

@derselbst
Copy link
Member

Ok, seems like VS2015 pollutes the binaries with unnecessary dependencies even in xp compatibility mode :/

@jjceresa
Copy link
Collaborator Author

jjceresa commented Jul 29, 2018

Ok, seems like VS2015 pollutes the binaries with unnecessary dependencies even in xp compatibility mode

Yes, it seems. Just for infos, below the dependency of my build. We see the presence of MSVCR100.DLL instead of the missing dlls described in previous comment.
jjc_build_dependency

@derselbst derselbst changed the base branch from master to 2.1-testing December 1, 2018 13:54
@derselbst derselbst merged commit 749ce44 into 2.1-testing Dec 1, 2018
@derselbst derselbst deleted the fdn-reverb branch December 1, 2018 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants