-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Real time limiter UI #6395
Real time limiter UI #6395
Conversation
d6e11c5
to
71d6ceb
Compare
@@ -0,0 +1,143 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be lib-src
is better place for this library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been considering, yes, but I'll probably be modifying the code in some follow-up work. Is that a good reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
std::make_shared<ParameterWrapper>( | ||
releaseMs), | ||
} } } | ||
, mSettings { std::move(settings) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::move
is redundant here, better pass settings
parameter by reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the case with POD-types I guess. It will simply create an extra copy.
src/effects/CompressorEditor.cpp
Outdated
services, | ||
access, | ||
{ { | ||
mSettings.inCompressionThreshDb, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spot, thanks.
wxWindow* parent, const DynamicRangeProcessorHistoryUPtr& history, | ||
DynamicRangeProcessorOutputs& outputs, EffectSettingsAccess& access) | ||
{ | ||
const CompressorSettings* settings = access.Get().cast<CompressorSettings>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 {...};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, didn't think of it. EffectWithSettings<>::MakeSettings
is virtual and can be overriden, will that help?
dc3fc0f
to
85e0f9b
Compare
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 |
@petersampsonaudacity was reporting this here for a chirp and with other parameters, too. |
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: This is technically a regression on 3.5.1 behavior with the old, replaced, effects. |
[this](AudioIOEvent evt) { | ||
if (evt.on) | ||
ClearHistory(); | ||
, mInitializeProcessingSettingsSubscription { instance.Subscribe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instance
will outlive editor, so that should be safe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if it didn't, would there be harm in a dangling subscription ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it stores pointer in weak_ptr
, should be safe anyway
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possibly depend on ring buffer size (which is usually 0.25ms)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The size of the low-latency playback thread buffer you mean? Yes.
prev = it; | ||
} | ||
if (std::unique_ptr<wxGraphicsContext> gc { wxGraphicsContext::Create(dc) }) | ||
DrawHistory<wxPoint2DDouble>(*gc, GetSize(), history, *mSync); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wxPoint2DDouble
makes graph move smoother?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's more for anti-aliasing. Will comment.
// Process trivially | ||
for (size_t ii = 0; ii < chans; ++ii) | ||
memcpy(outbuf[ii], inbuf[ii], numSamples * sizeof(float)); | ||
if (pInstance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
class CompressorInstance final : | ||
public PerTrackEffect::Instance, | ||
public EffectInstanceWithBlockSize, | ||
public Observer::Publisher<std::optional<InitializeProcessingSettings>> | ||
public InitializeProcessingSettingsPublisher, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@petersampsonaudacity thanks for your report! |
@saintmatthieu testing on W11 with your two latest branch builds; I confirm that So looks like this works properly now.
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 |
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. |
That's the correct version. I'm listening :) |
cc4aaec
to
d11db39
Compare
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. 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: |
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 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. |
Also tested the same AppImage in a clean install of Debian in VirtualBox and I get the same result. |
Right, it was with a square wave. Now with a sine: |
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). |
Thanks for your early report, @SteveDaulton , @dozzzzer has confirmed this. Looking into this. |
(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: |
I was using |
When resizing the window during playback, the graph drawing stops until the LMB is released: 2024-05-17_11-38-35.mp4 |
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. |
cc24eaa
to
8c2b772
Compare
@dozzzzer actually that was an easy fix. Please re-test when latest build is ready. |
Testing with Commit Id: d40c0e I am still seeing the same bug. |
I beg your pardon: I messed up force-pushing from different machines. 0d0f371 is the fix commit. |
@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. |
631b01d
to
fdbac25
Compare
Yes, I think it's better with the blank rather than the line.
I'm pretty sure folk can live with that, resizing the dialog is not something most users will be doing a lot of. |
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. |
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.mp4I 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? |
Oof. Yeah that's pretty bad. |
Thanks @dozzzzer, it is a bug you caught indeed. |
@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? |
@saintmatthieu . In the compressor dialog, the panel containing the transfer function doesn't need to be in the tab order.
|
@dozzzzer thanks. It appears that it's not only when opening that particular menu, but any menu. Also, it doesn't occur on Mac. |
@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. |
@DavidBailes beautiful, thank you! |
or when loading his VST we'd have duplicate symbols.
4d88ae3
to
11ce44e
Compare
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: |
Resolves: #6305, #6366
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
andVolume 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.
Recommended:
QA:
Channel routing tests: