Implement look-ahead peak limiter with oar_limiter orchestration layer - #40
Conversation
trsonic
left a comment
There was a problem hiding this comment.
Hi @jingbo-marquis ! Thank you so much for submitting this limiter implementation. We have already been testing this algorithm and we are happy with the outcomes. Below is a review done by my coding agent, it mostly addresses issues with the first processed block, which in our implementation is zero-padded at the beginning, what makes it easier to use in real-time scenarios when the processing buffer size is fixed.
Let me know if you have any questions.
T.
Reviewed and tested on Apple Silicon (macOS, Apple clang), building the branch with CMake and driving it from a fixed-block render loop with the limiter enabled.
The DSP is the right one. I have been running the same libiamf audio_effect_peak_limiter port alongside the existing instant-attack limiter for a while, and the constants here match it exactly: attack 0.001 s, release 0.200 s, look-ahead 0.005 s, threshold -1.0 dBFS, 240 samples of delay at 48 kHz. No disagreement about the algorithm or the defaults.
The problem is the calling contract. oar_limiter_process overwrites output->samples_per_channel, and oar_render validates that same field on entry (src/oar.c:523), so a caller that reuses its oar_audio_block_t across calls gets one good render and then permanent failure. Reproduced on this branch, stereo out, 512-sample blocks, 48 kHz:
limiter delay = 240 samples
frame 0: asked=512 rc=0 -> samples_per_channel=272
frame 1: asked=272 rc=-22 -> samples_per_channel=272
frame 2: asked=272 rc=-22 -> samples_per_channel=272
frame 3: asked=272 rc=-22 -> samples_per_channel=272
Output is never written again after frame 0. Resetting the field before every call works around it, but nothing in the header says a caller must, and oar_render takes the block by pointer.
Underneath that is a design question worth settling before the API lands. Dropping the priming samples changes both the output length and the planar stride on the first block. That is workable for offline file processing and unusable in a fixed-block device callback without every host rewriting its render loop. Emitting the priming samples instead and reporting the delay through oar_get_limiter_delay(), which this PR already adds, gives the same audio with no API break: hosts that want the samples dropped can drop them, hosts that want latency compensation get a number. That is the route I took in my own port for exactly this reason.
Two other things:
The feature has no tests. Nothing in tests/examples/ calls oar_enable_limiter, oar_flush, oar_get_limiter_delay or oar_set_limiter_threshold, lim->enabled starts at 0 from def_mallocz, and oar_limiter_process returns early when disabled without touching the block. All nine green jobs exercise the passthrough path only, which is why the failure above does not show up in CI.
_oar_metadatas_elapse(oar, samples) using the input frame count is correct and worth keeping whatever happens to the rest.
Individual points inline.
| max_req_gain; | ||
| } | ||
| limiter_env[f] = (float)limiter->env; | ||
| output->samples_per_channel = (uint32_t)returned; |
There was a problem hiding this comment.
Blocking. This write breaks the next oar_render call.
oar_render validates the block on entry (src/oar.c:523):
if (output->channels != out_channels ||
output->samples_per_channel != samples)
return ck_oar_error_inval;samples there is oar->config.samples_per_channel. After this line sets the field to returned (272 for a 512-sample block at 48 kHz), a caller that reuses the same oar_audio_block_t fails that check on the next call and every call after it, with the output buffer left untouched. Reproduced on this branch:
frame 0: asked=512 rc=0 -> samples_per_channel=272
frame 1: asked=272 rc=-22 -> samples_per_channel=272
frame 2: asked=272 rc=-22 -> samples_per_channel=272
Reusing the block struct across calls is the natural pattern and is what a device callback does, so this needs to work without the caller restoring the field by hand. Either oar_render should re-derive the field from the config on entry rather than validating the caller's value, or the limiter should not change the block length at all (see the review body).
| return ck_oar_error_inval; | ||
| } else if (returned > 0) { | ||
| for (int c = 0; c < lim->num_channels; c++) { | ||
| memcpy(&output->data[c * returned], &lim->out_buf[c * returned], |
There was a problem hiding this comment.
The re-pack changes the planar stride under the caller. out_buf holds the compacted planes at stride returned, and this copies them into output->data at that same stride, but the caller allocated and reasoned about stride config->samples_per_channel.
Measured on the first block, stereo out, 512 requested:
rc=0 returned samples=272 (asked 512)
plane at stride 272 (what the limiter wrote): ch0 E=121.349 ch1 E=121.349
plane at stride 512 (what the host reads): ch1 starts at out[512] = 0.0
A host that has already committed to stride 512 reads channel 0 as its 272 real samples followed by the first 240 samples of channel 1, and channel 1 as stale pre-limiter mix. Checking the return code does not help; the caller has to re-derive its stride from samples_per_channel on every block, which is a heavier contract than the @note in oar.h conveys.
| ths->peak_pos = wrapped; | ||
| } | ||
| } | ||
| if (ths->peak_pos < 0) ths->peak_pos = wrap_index(idx, ths->delay_size); |
There was a problem hiding this comment.
This fallback defeats the peak cache on silence. It points peak_pos at the current write slot, and update_peak_data then hits its ths->peak_pos == wrapped_idx branch and sets it back to -1, so a silent window rescans all delay_size slots, with a % in wrap_index each, on every sample: 122,880 modulo operations per 512-sample block at a 240-sample delay.
Measured per 512-sample block, stereo out, 48 kHz, 2000 iterations (block period 10.667 ms):
with line 154 line 154 removed
limiter on, silence 0.0800 ms 0.0034 ms
limiter on, tone 0.0035 ms 0.0045 ms
Deleting the line is safe: peak_pos stays -1, and update_peak_data's ths->peak_pos < 0 branch caches the current slot on the next sample, which is the behaviour you want. Neither figure threatens the block deadline, but silence is the idle state, so it is worth not paying 23x for it.
| if (!oar) return ck_oar_error_inval; | ||
| oar->enable_limiter = enable ? 1 : 0; | ||
| return ck_oar_ok; | ||
| return oar_limiter_enable(oar->limiter, enable); |
There was a problem hiding this comment.
oar_limiter_enable(NULL, enable) returns ck_oar_error_inval, so when limiter creation failed this returns -22 where it previously returned ck_oar_ok. That sits oddly with the change just above making creation failure non-fatal: OAR is allowed to come up without a limiter, but then the first thing a caller does with it hard-errors. Either creation failure should stay fatal or this should return ck_oar_ok when there is no limiter to enable.
There was a problem hiding this comment.
If the limiter fails to initialize, returning ck_oar_ok from oar_enable_limiter would mislead the caller into thinking the audio signal is being limited when it actually isn't.
Introducing ck_oar_error_notsup lets the function signal to the caller that the limiting feature is unavailable because the limiter could not be created.
That said, I don't consider the limiter an essential part of OAR — even if limiter creation fails, it shouldn't cause OAR creation itself to fail.
| * @note When limiter is enabled, output->samples_per_channel may be less | ||
| * than config->samples_per_channel (first frames may output 0 | ||
| * samples due to look-ahead delay). Always check | ||
| * output->samples_per_channel. |
There was a problem hiding this comment.
These two notes understate what a caller has to do. Beyond checking output->samples_per_channel, the caller must also re-derive the planar stride from it on every block, and must restore the field to config->samples_per_channel before the next oar_render or that call fails the entry validation at src/oar.c:523. Neither is mentioned.
If the priming samples are emitted rather than dropped, both notes go away and oar_get_limiter_delay() carries the whole contract on its own.
|
|
||
| float peak = find_peak(ths, idx); | ||
| float gain = update_gain_envelope(ths); | ||
| detect_peak(ths, peak); |
There was a problem hiding this comment.
Minor: detect_peak returns an int that process_core discards, and nothing else calls it. Either drop the return or use it.
There was a problem hiding this comment.
Removed the return value of detect_peak.
- Replace simple limiter with look-ahead peak limiter - Encapsulate peak limiter struct and remove hardcoded delay size limit - Fix peak limiter division-by-zero and oar_flush output stride - Introduce oar_limiter orchestration layer and refactor peak limiter - Fix limiter buffer stride and add get_delay API - Fix limiter null-safety and Doxygen annotation issues Signed-off-by: Jingbo Hou <jingbo.hou@samsung.com>
- Replace compact_output (drop-priming) with advance_priming (emit-priming) so limiter always outputs full frame with zero-padded priming samples - Add has_data flag to AudioRendererBase for reliable data tracking - Add null-check guards for oar->limiter in enable/set_threshold functions - Relax oar_render samples_per_channel validation and add any_data check - Update oar_render and oar_flush API documentation for clarity Signed-off-by: Jingbo Hou <jingbo.hou@samsung.com>
- Add test_peak_limiter_oar_api.c with 14 test cases covering emit-priming, threshold control, flush behavior, NULL safety, block reuse, planar stride integrity, and sample conservation - Add generate_sine_ampl() and amplitude_over_threshold() helpers for over-threshold test signal generation - Add is_all_zero() and check_output_limited() output validation helpers - Register test_peak_limiter_oar_api in BUILD.bazel and CMakeLists.txt Signed-off-by: Jingbo Hou <jingbo.hou@samsung.com>
5effd0d to
9f4c85b
Compare
- Move memset and samples_per_channel assignment before any_data check to ensure output is always zeroed even on early cleanup path - Inline delay_size calculation in oar_limiter_create to remove unnecessary intermediate variable Signed-off-by: Jingbo Hou <jingbo.hou@samsung.com>
Problem
The current OAR limiter uses an instant-attack design with no look-ahead. For tonal signals with slowly rising amplitude (e.g., soprano vocal singing forte with vibrato), the instant attack introduces significant distortion. The issue reporter identified that introducing a look-ahead buffer (2 ms is sufficient) would resolve the distortion, but at the cost of introducing latency. The API should also expose this latency so the user's software can compensate for it.
Solution
Replace the simple limiter with a look-ahead peak limiter and introduce an
oar_limiterorchestration layer that wraps the underlying peak limiter and manages buffer allocation, enable/disable state, and flush semantics.Architecture
flowchart TD A["oar.c (public API)"] --> B["oar_limiter.c (orchestration layer)"] B --> C["audio_effect_peak_limiter.c (DSP core)"] classDef api fill:#e65100,color:#ffffff,stroke:#e65100 classDef orch fill:#4a148c,color:#ffffff,stroke:#4a148c classDef dsp fill:#0d47a1,color:#ffffff,stroke:#0d47a1 class A api class B orch class C dspThe solution adopts a three-layer architecture:
DSP Core (
audio_effect_peak_limiter): The look-ahead peak limiter DSP core, sourced from libiamf. It implements attack/release gain envelope smoothing, hard-limit clamping, and a circular delay buffer with peak cache. The struct is opaque to prevent external access.Orchestration Layer (
oar_limiter): A new orchestration layer that wraps the DSP core, managing its lifecycle, intermediate output buffer allocation, enable/disable state, and flush semantics. This layer isolates the upper OAR code from the DSP implementation details.Public API (
oar.c): Updated to use config-based limiter creation, added flush support for end-of-stream, and delegated enable/disable to the orchestration layer. Limiter creation failure is now non-fatal — OAR continues without a limiter (with warning) instead of failing entirely.Key Design Decisions
Behavioral Notes