add expanders

[magnetophon]

[magnetophon]

I just fixed an oversight; maxHold, the maximum hold time was internal and needed to be exposed.

[sletz]

// * thresh: dB level threshold below which expansion kicks in

==> shouldn't you add _db on function names for more naming coherency?

[magnetophon]

Ah, yes, thanks. Done!

[magnetophon]

Could this internal function be part of a with {...} section?

OK, done. I moved the level function to where it actually belongs; inside peak_axpansion_gain_mono.

I guess N is known at compile time ? If yes, then please add it in the comment.

Good catch, done.

[DBraun]

I would like to see this merged in eventually, but I have some suggestions.

I think N should be the first argument since it's a compile-time constant. Similarly maxHold should be the second argument.

Personally I don't feel the need for there to be dedicated functions for mono. It's pretty easy to just pass 1 as the N argument. This would allow the names to be simplified more. I would prefer for them to not have N, chan or db in their names. Is the contribution essentially a "peak expander" and a "feed-forward expander"? Should we then have them be called "expander_peak" and "expander_ff"? Maybe @sletz should weigh in after @magnetophon one more time. Thanks.

[DBraun]

I see that we will have a consistency issue because of the way most of the functions in compressors.lib are written.

[sletz]

What do you mean by "consistency issue" ?

[DBraun]

Now I realize that these functions are already in compressors.lib. By consistency issue I meant that the functions in compressors.lib don’t put compile constants as earlier arguments. I also suggested ways to improve the function names but that is not what the current library looks like.

[sletz]

I guess we would prefer "compile constants as earlier arguments", but we don't want to break existing code. So what can be done: keep current functions and define a fixed new set with "name" ==> "name2" (or similar) ?

[DBraun]

I say do nothing for now. It can always happen later. And I think this PR is no longer needed?

[sletz]

@magnetophon is this PR still needed ?

[magnetophon]

I think N should be the first argument since it's a compile-time constant. Similarly maxHold should be the second argument.

Personally I don't feel the need for there to be dedicated functions for mono. It's pretty easy to just pass 1 as the N argument. This would allow the names to be simplified more. I would prefer for them to not have N, chan or db in their names. Is the contribution essentially a "peak expander" and a "feed-forward expander"? Should we then have them be called "expander_peak" and "expander_ff"? Maybe @sletz should weigh in after @magnetophon one more time. Thanks.

I fully agree with this and as you note later on, my other compressors have the same issue. There's also a few more subtle issues with them, for example the link is after the smoothers, it should be before them.

I say do nothing for now. It can always happen later.

Yes, let's hope the new gsoc package manager by @shehab299 will have some features to help us out here!

And I think this PR is no longer needed?

Indeed, it looks like it got merged at some point, but I didn't know about it, as it is not indicated in this thread.

[sletz]

I d'ont remember clearly about a possible merge.. But I can close right ?