Real time limiter UI #6395

saintmatthieu · 2024-05-07T08:12:02Z

This PR wraps Daniel Rudrich's SimpleCompressor in two new real-time capable built-in effects, namely "Real-time Compressor" and "Real-time Limiter".

The limiter is the same as the compressor but with a reduced set of parameters, ratio and attack time being hard-coded to infinity and 0ms.

The legacy, offline compressor files were renamed, but for now stays found in the same place in the menu (Volume and Compression > Compressor and Volume and Compression > Limiter). Removal from default list is planned in #6319. At that stage the new versions should also find their final place in the menu.

UI still misses the transfer function, which may be added to this PR or as follow-up.

I signed CLA
The title of the pull request describes an issue it addresses
If changes are extensive, then there is a sequence of easily reviewable commits
Each commit's message describes its purpose and effects
There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

Each commit compiles and runs on my machine without known undesirable changes of behavior

QA:

Changing parameters in real time works
Latency compensation works (use look-ahead for this to be non-trivial)
Effect preview works
Channel routing tests:
Mono track is processed properly
Stereo track is processed properly (use a clip with contrasting left/right content)
Compression history only appears when used as real-time effect
Adding an instance on the real-time effect stack and rendering the track yields output that's bit-exact with applying the effect as destructive effect.

vsverchinsky · 2024-05-08T08:38:50Z

libraries/lib-dynamic-range-processor/SimpleCompressor/GainReductionComputer.cpp

@@ -0,0 +1,143 @@
+/*


May be lib-src is better place for this library?

I've been considering, yes, but I'll probably be modifying the code in some follow-up work. Is that a good reason?

Up to you. I think it's not a big deal if you modify code added to lib-src, I recall there are already such cases

vsverchinsky · 2024-05-08T09:06:51Z

src/effects/CompressorEditor.cpp

+                                         std::make_shared<ParameterWrapper>(
+                                            releaseMs),
+                                      } } }
+    , mSettings { std::move(settings) }


std::move is redundant here, better pass settings parameter by reference

Is it? The given settings object is originally a right value. If I passed by reference in the end there'd be a call to the copy ctor, wouldn't there?

Not the case with POD-types I guess. It will simply create an extra copy.

vsverchinsky · 2024-05-08T09:13:08Z

src/effects/CompressorEditor.cpp

+                                    services,
+                                    access,
+                                    { {
+                                         mSettings.inCompressionThreshDb,


mSettings is not initialized at this point. While in that particular case nothing bad will happen because CompressorSettings is trivially constructible, I'd still suggested to move initialization into constructor's body.

Well spot, thanks.

vsverchinsky · 2024-05-08T10:35:05Z

src/effects/DynamicRangeProcessorEditor.cpp

+   wxWindow* parent, const DynamicRangeProcessorHistoryUPtr& history,
+   DynamicRangeProcessorOutputs& outputs, EffectSettingsAccess& access)
+{
+   const CompressorSettings* settings = access.Get().cast<CompressorSettings>();


I don't understand why should LimiterSettings inherit CompressorSettings. I really do not see any benefits from doing so, but I see downsides in many if(...) else { ... } and typechecks. I'd suggested to eliminate LimiterSettings type, but instead introduce:

static constexpr DefaultCompressorSettings = CompressorSettings {...}; static constexpr DefaultLimiterSettings = CompressorSettings {...};

I'm not sure that this would work - not that I wouldn't like it to!
I think it's because of this guy:

The EffectSettingsAccess class initially creates a default-constructed struct. Creating a new type is the only way I found of circumventing that.

Hm, didn't think of it. EffectWithSettings<>::MakeSettings is virtual and can be overriden, will that help?

dozzzzer · 2024-05-14T16:20:05Z

@saintmatthieu

When changing parameters during playback, abrupt volume changes of high amplitude occur:

(Warning: it may get loud)

Screen.Recording.2024-05-14.at.17.54.40.video-converter.com.mp4

saintmatthieu · 2024-05-14T17:58:03Z

@petersampsonaudacity was reporting this here for a chirp and with other parameters, too.
I expect this to happen. We do not have any kind of parameter change smoothing in place, so it does so in steps. In this particular case, you are changine the threshold for the limiter, which is like changing the input gain.

petersampsonaudacity · 2024-05-15T10:00:21Z

Testing on W11 with @saintmatthieu 's latest branch build: audacity-win-3.6.0-alpha-20240514+2d767ac-x64

This shows that with both the new effects the Presets and Settings fail to work:
a) the Factory Presets/Default fails to restore the default settings,
b) whilst the user can set and recall a user preset, these also fail to apply when invoked.

This is technically a regression on 3.5.1 behavior with the old, replaced, effects.

vsverchinsky · 2024-05-15T10:13:38Z

src/effects/DynamicRangeProcessorHistoryPanel.cpp

-         [this](AudioIOEvent evt) {
-            if (evt.on)
-               ClearHistory();
+    , mInitializeProcessingSettingsSubscription { instance.Subscribe(


instance will outlive editor, so that should be safe

And if it didn't, would there be harm in a dangling subscription ?

No, it stores pointer in weak_ptr, should be safe anyway

vsverchinsky · 2024-05-15T10:53:09Z

src/effects/DynamicRangeProcessorHistoryPanel.cpp

+   // empirically, but with a relatively large audio playback delay. Maybe it
+   // will be found to lag on lower-latency playbacks. Best would probably be to
+   // make it playback-delay dependent.
+   constexpr auto displayDelay = 0.2f;


Is it possibly depend on ring buffer size (which is usually 0.25ms)?

The size of the low-latency playback thread buffer you mean? Yes.

vsverchinsky · 2024-05-15T11:55:19Z

src/effects/DynamicRangeProcessorHistoryPanel.cpp

-      prev = it;
-   }
+   if (std::unique_ptr<wxGraphicsContext> gc { wxGraphicsContext::Create(dc) })
+      DrawHistory<wxPoint2DDouble>(*gc, GetSize(), history, *mSync);


wxPoint2DDouble makes graph move smoother?

No, it's more for anti-aliasing. Will comment.

vsverchinsky · 2024-05-15T12:27:43Z

libraries/lib-realtime-effects/RealtimeEffectState.cpp

      // Process trivially
      for (size_t ii = 0; ii < chans; ++ii)
         memcpy(outbuf[ii], inbuf[ii], numSamples * sizeof(float));
+      if (pInstance)


I guess it will be better to always call RealtimeProcess, but instead effects could implement bypass mode, so that effects could accumulate samples if it's required for processing. For VST's that should be done trivially. WDYT?

We'd then have to review all implementations and make sure none of the actually does the processing when bypassed. So I think this is safer.

vsverchinsky · 2024-05-15T15:28:25Z

src/effects/CompressorInstance.h

 class CompressorInstance final :
    public PerTrackEffect::Instance,
    public EffectInstanceWithBlockSize,
-    public Observer::Publisher<std::optional<InitializeProcessingSettings>>
+    public InitializeProcessingSettingsPublisher,


CompressorInstance could remember it's state after init/suspend/resume/finalize, OnTimer then could check that state. I have no doubt that observers will do the thing, I'm just a bit sceptical about effect instance being a publisher.

The sample rate may change between playback states, and all this time the panel may stay open, so the sample rate update is a must.
I tried other things before, and the observer-publisher pattern seemed like the simplest in the end (although there is no fundamental difference between injecting a callback lambda, I think).

saintmatthieu · 2024-05-16T07:57:15Z

Testing on W11 with @saintmatthieu 's latest branch build: audacity-win-3.6.0-alpha-20240514+2d767ac-x64

This shows that with both the new effects the Presets and Settings fail to work: a) the Factory Presets/Default fails to restore the default settings, b) whilst the user can set and recall a user preset, these also fail to apply when invoked.

This is technically a regression on 3.5.1 behavior with the old, replaced, effects.

@petersampsonaudacity thanks for your report!
On my side it's also buggy, but less so: changing an effect parameter by applying a preset does not update the slider positions. Everything else works, though.
Could you double-check if this is consistent with your experience?
Fix commit pushed.

petersampsonaudacity · 2024-05-16T12:37:01Z

On my side it's also buggy, but less so: changing an effect parameter by applying a preset does not update the slider positions.

@saintmatthieu testing on W11 with your two latest branch builds;
audacity-win-3.6.0-alpha-20240516+b7b392b-x64 and
audacity-win-3.6.0-alpha-20240516+13bcfb5-x64

I confirm that
a) the default slider settings get restored (slider positions) with the use of the Factory >default
b) the user can set a user preset and when recalled it restores the users slider positions

So looks like this works properly now.

Everything else works, though.
Could you double-check if this is consistent with your experience?

I can't really vouch for the actual accuracy in use of compressor or limirter as I have no real experience of using such effects - you might want to approach @SteveDaulton for that.

One thing I do note is that the dialogs for the new replacement effects are narrower than the ones they replace - the upshot being that the sliders are narrower than previously.

This can be mitigated by expanded the dialog rightwards with click&drag on its right edge
BUT I am un-convinced that this will be readily "discoverable" by many users.

SteveDaulton · 2024-05-16T15:29:47Z

Am I testing the correct version? I am using Audacity 3.6.0-alpha commit ID: 4e4fdb but the new Limiter does not behave like any other limiter I've ever used.

saintmatthieu · 2024-05-16T15:35:06Z

That's the correct version. I'm listening :)

SteveDaulton · 2024-05-16T16:06:59Z

An "ideal" (but impossible) limiter prevents the audio from going above a specified threshold level, without affecting audio below that level (other than optional makeup gain), and without introducing distortion.

Easiest to demonstrate with a generated test tone, though it also applies to real-world audio.
Here is a 10 second 2kHz sine tone, which fades up to a peak level of 0dB over 5 seconds, then fades back down to silence:

After limiting to -5dB (chosen to match the new limiter default), I would expect the waveform to look similar to this:

Using the new Limiter with default settings, the result looks like this:

which, after normalizing to 0dB looks like this:

saintmatthieu · 2024-05-16T16:23:52Z

Strange. I get this:

That was, like you, with default settings:

That looks correct to me.

SteveDaulton · 2024-05-16T16:34:03Z

Strange. I get this:

That does not look like a sine wave. I'm guessing it is a square wave, but even so I don't get that result.
(I am using the Ubuntu 22.04 AppImage on Ubuntu 22.04)

LWinterberg · 2024-05-16T16:34:48Z

I cannot reproduce your post-normalization case either.

The way this thing works is somewhat similar to the Waves L2, the Avid Pro Limiter and various other mastering limiters. The idea of these is that you have a ceiling (which we call "make-up target") which already would be a normalized output to whatever you set as your target - assuming material actually hits the threshold. For the case where you don't want any make-up, the target and threshold need to be identical.

SteveDaulton · 2024-05-16T16:59:35Z

Also tested the same AppImage in a clean install of Debian in VirtualBox and I get the same result.

saintmatthieu · 2024-05-16T17:29:26Z

Right, it was with a square wave. Now with a sine:
@SteveDaulton any way you could try the exact same on another OS ? Looks like we have a problem on Linux.

SteveDaulton · 2024-05-16T23:45:42Z

Curiously, on one attempt the effect did change the dynamics, but much more than it should have. I have not been able to repeat this.

When I try now, if I set the Threshold and Make-up Target to the same value, the effect does nothing (effectively a no-op).

saintmatthieu · 2024-05-17T08:42:05Z

Thanks for your early report, @SteveDaulton , @dozzzzer has confirmed this. Looking into this.

dozzzzer · 2024-05-17T08:42:54Z

(Ubuntu 22.04) I also see a wrong result after applying Limiter with the default parameters to a sine tone with fade in & fade out, although different from @SteveDaulton's one:

saintmatthieu · 2024-05-17T09:31:09Z

I was using abs instead of std::abs, which on Linux takes an int argument ...
Fix pushed, please @SteveDaulton @dozzzzer would you give it another shot once the build is ready ?

dozzzzer · 2024-05-17T09:42:50Z

When resizing the window during playback, the graph drawing stops until the LMB is released:

2024-05-17_11-38-35.mp4

saintmatthieu · 2024-05-17T09:44:36Z

Yeah, I haven't found how to solve this. I don't get updates when resizing is under way (wxWidgets stuff). At best I could get the straight line to disappear. Then it'd look like when you bypass and un-bypass.

saintmatthieu · 2024-05-17T10:42:31Z

@dozzzzer actually that was an easy fix. Please re-test when latest build is ready.

SteveDaulton · 2024-05-17T11:35:21Z

Testing with Commit Id: d40c0e I am still seeing the same bug.

saintmatthieu · 2024-05-17T12:11:08Z

I beg your pardon: I messed up force-pushing from different machines. 0d0f371 is the fix commit.

petersampsonaudacity · 2024-05-17T12:36:07Z

Testing on W11 with: audacity-win-3.6.0-alpha-20240517+d40c0e6-x64

Like @dozzzzer I get a break when the dilaog is resized until the LMB is released.

But unlike @dozzzzer my break is blank withou the joing line that he has:

saintmatthieu · 2024-05-17T12:45:30Z

@petersampsonaudacity yes. The fact that you don't see a line is the improvement brought by 8c2b772. I don't think I can possibly get the data updates during resizing, as this seems to block the main thread, where the updates otherwise happen.

petersampsonaudacity · 2024-05-17T14:10:13Z

The fact that you don't see a line is the improvement ...

Yes, I think it's better with the blank rather than the line.

I don't think I can possibly get the data updates during resizing, as this seems to block the main thread, where the updates otherwise happen.

I'm pretty sure folk can live with that, resizing the dialog is not something most users will be doing a lot of.

SteveDaulton · 2024-05-17T15:57:52Z

I beg your pardon: I messed up force-pushing from different machines. 0d0f371 is the fix commit.

OK, that one works, though it introduces a lot more distortion than the Nyquist limiter, even with the "Hard Limit" algorithm. The distortion is particularly noticeable on the 3rd harmonic.

dozzzzer · 2024-05-17T17:37:12Z

During playback, when moving one of the following: Threshold, Attack or Release in the Compressor effect, it produces abrupt volume changes. The Limiter effect doesn't have such a problem anymore. Check this out:

2024-05-17_19-26-49.mp4

I think this is pretty serious, so I'd like this to be fixed within this PR. However, given the circumstances, I'd want to know what you guys think about it and whether we would want to address this in the next patch release @Tantacrul @LWinterberg

Also, @saintmatthieu I understand this isn't going to be an easy fix, is it?

LWinterberg · 2024-05-17T17:52:47Z

Oof. Yeah that's pretty bad.

saintmatthieu · 2024-05-17T19:22:17Z

Thanks @dozzzzer, it is a bug you caught indeed.
Note that each time you change the release time, the graph shows the actual attenuation goes back to zero before it converges back towards the target, in a time lapse that I suspect equals your look-ahead (120ms).
The mistake I did was to reset the look-ahead state when any parameter gets changed. Fix was easy and is pushed. I hope it does the job.

dozzzzer · 2024-05-20T09:46:31Z

@saintmatthieu I've discovered that the graph isn't being painted as long as the "Presets & Settings" menu is opened. And then when the menu is closed, a straight line is painted all over the gap. Could you fix that?

DavidBailes · 2024-05-20T10:08:55Z

@saintmatthieu . In the compressor dialog, the panel containing the transfer function doesn't need to be in the tab order.
cf Autoduck effect, where in Autoduck.h, the class definition for the panel it uses includes the following lines:

bool AcceptsFocus() const override { return false; }
// So that wxPanel is not included in Tab traversal - see wxWidgets bug 15581
bool AcceptsFocusFromKeyboard() const override { return false; }

saintmatthieu · 2024-05-21T08:39:54Z

@saintmatthieu I've discovered that the graph isn't being painted as long as the "Presets & Settings" menu is opened. And then when the menu is closed, a straight line is painted all over the gap. Could you fix that?

@dozzzzer thanks. It appears that it's not only when opening that particular menu, but any menu. Also, it doesn't occur on Mac.
Looking into this ...

saintmatthieu · 2024-05-21T10:12:55Z

@dozzzzer just pushed a few commits that alleviate but don't eliminate the problem. What you should now see when closing the menu is a gap, like you do after resizing the graph.
It seems to me that the main thread gets blocked rather easily on Windows. Opening a menu, moving the window, resizing it ... and the data transfer from audio to main thread relies on the main thread being fast, or there is data loss. This explains why the graph isn't deterministic. Maybe there is a more reliable way of transmitting the data, I will have to look into this, but not in this PR anymore.

saintmatthieu · 2024-05-21T10:19:18Z

@saintmatthieu . In the compressor dialog, the panel containing the transfer function doesn't need to be in the tab order. cf Autoduck effect, where in Autoduck.h, the class definition for the panel it uses includes the following lines:
bool AcceptsFocus() const override { return false; }
// So that wxPanel is not included in Tab traversal - see wxWidgets bug 15581
bool AcceptsFocusFromKeyboard() const override { return false; }

@DavidBailes beautiful, thank you!

or when loading his VST we'd have duplicate symbols.

petersampsonaudacity · 2024-05-27T15:30:20Z

Testing on W10 with @saintmatthieu 's latest master alpha build for 3.6.0: audacity-win-3.6.0-alpha-20240527+5f8ee46-x64-msvc2022

The graph now looks continuous:
a) when the "Presets and Settings" menu is opened
b) when parameter sliders are moved and the LMB is later released

saintmatthieu requested a review from vsverchinsky May 7, 2024 08:12

saintmatthieu mentioned this pull request May 7, 2024

Realtime limiter #6306

Closed

11 tasks

saintmatthieu force-pushed the real-time-limiter-ui branch from d6e11c5 to 71d6ceb Compare May 7, 2024 08:17

vsverchinsky reviewed May 8, 2024

View reviewed changes

saintmatthieu force-pushed the real-time-limiter-ui branch 2 times, most recently from dc3fc0f to 85e0f9b Compare May 14, 2024 15:22

saintmatthieu mentioned this pull request May 15, 2024

Help page for new compressor and limiter #6339

Open

vsverchinsky approved these changes May 15, 2024

View reviewed changes

saintmatthieu force-pushed the real-time-limiter-ui branch from cc4aaec to d11db39 Compare May 16, 2024 15:52

saintmatthieu force-pushed the real-time-limiter-ui branch from cc24eaa to 8c2b772 Compare May 17, 2024 10:41

saintmatthieu force-pushed the real-time-limiter-ui branch from 631b01d to fdbac25 Compare May 17, 2024 12:50

saintmatthieu added 8 commits May 22, 2024 15:45

Compressor -> OfflineCompressor

5be2a5e

Copy https://github.com/DanielRudrich/SimpleCompressor @3fb7d34

b0e1919

Place Daniel Rudrich's code in namespace DanielRudrich ...

43a3d22

or when loading his VST we'd have duplicate symbols.

lib-dynamic-range-processor and some tests

200a3a6

Fix compilation warning

f562bdc

Add realtime limiter and compressor to effects

06ebcfb

History creates new segment when detecting packet loss

d3124dc

Show-graph checkbox

11ce44e

saintmatthieu force-pushed the real-time-limiter-ui branch from 4d88ae3 to 11ce44e Compare May 22, 2024 13:48

saintmatthieu merged commit 8f5a226 into audacity:master May 22, 2024
9 of 10 checks passed

Real time limiter UI #6395

Real time limiter UI #6395

Conversation

saintmatthieu commented May 7, 2024 • edited by dozzzzer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dozzzzer commented May 14, 2024

saintmatthieu commented May 14, 2024

petersampsonaudacity commented May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saintmatthieu commented May 16, 2024

petersampsonaudacity commented May 16, 2024 • edited

SteveDaulton commented May 16, 2024

saintmatthieu commented May 16, 2024 • edited

SteveDaulton commented May 16, 2024

saintmatthieu commented May 16, 2024

SteveDaulton commented May 16, 2024

LWinterberg commented May 16, 2024 • edited

SteveDaulton commented May 16, 2024

saintmatthieu commented May 16, 2024

SteveDaulton commented May 16, 2024

saintmatthieu commented May 17, 2024

dozzzzer commented May 17, 2024 • edited

saintmatthieu commented May 17, 2024

dozzzzer commented May 17, 2024

saintmatthieu commented May 17, 2024

saintmatthieu commented May 17, 2024

SteveDaulton commented May 17, 2024

saintmatthieu commented May 17, 2024

petersampsonaudacity commented May 17, 2024

saintmatthieu commented May 17, 2024

petersampsonaudacity commented May 17, 2024

SteveDaulton commented May 17, 2024

dozzzzer commented May 17, 2024

LWinterberg commented May 17, 2024

saintmatthieu commented May 17, 2024 • edited

dozzzzer commented May 20, 2024

DavidBailes commented May 20, 2024

saintmatthieu commented May 21, 2024

saintmatthieu commented May 21, 2024

saintmatthieu commented May 21, 2024

petersampsonaudacity commented May 27, 2024 • edited

saintmatthieu commented May 7, 2024 •

edited by dozzzzer

petersampsonaudacity commented May 15, 2024 •

edited

petersampsonaudacity commented May 16, 2024 •

edited

saintmatthieu commented May 16, 2024 •

edited

LWinterberg commented May 16, 2024 •

edited

dozzzzer commented May 17, 2024 •

edited

saintmatthieu commented May 17, 2024 •

edited

petersampsonaudacity commented May 27, 2024 •

edited