Fix Input Boost wrapping around instead of clipping

[mcfnord]

Note

📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.

Input Boost multiplies each sample and narrows the int result back to int16_t with a plain cast, so a boosted sample that passes full scale wraps to the opposite polarity instead of clipping.

// src/client.cpp, CClient::ProcessAudioDataIntern
vecsStereoSndCrd[j + 1] = static_cast<int16_t> ( iInputBoost * vecsStereoSndCrd[j + 1] );
vecsStereoSndCrd[j]     = static_cast<int16_t> ( iInputBoost * vecsStereoSndCrd[j] );

iInputBoost is 2..10 (the "2x".."10x" dropdown), so the product routinely exceeds int16_t. Wraparound is a full-amplitude sign flip, which is considerably more audible than the clipping a user would expect from a gain control.

The invariant this breaks is written down 20 lines below

The pan block in the same function documents exactly when the plain cast is safe:

// note that the gain is always <= 1, therefore a simple cast is
// ok since we never can get an overload

and the mono mixdown block, where sums can overload, says:

// note that we need the Float2Short for stereo pan mode

Input Boost is the only gain > 1 in the send path, and the only one of the three that does not saturate.

Measured

1 kHz sine at −10 dBFS, 4800 samples. "Sign-flipped" means the current cast returned a value of opposite polarity to the true product.

boost	wraps above	samples over full scale	sign-flipped	max error
2x	−6.0 dBFS	0 / 4800	0	0
4x	−12.0 dBFS	2200 / 4800	2200 (100%)	65424 LSB
10x	−20.0 dBFS	3800 / 4800	2200 (the rest wrap twice)	63760 LSB

The threshold is 32767 / iInputBoost. At 10x — the setting a user with a quiet microphone is most likely to reach for — anything above −20 dBFS inverts.

Performance

This is a per-sample loop, so: x86-64, gcc 13.3, -O3, 128-sample frame (2.67 ms), best of 7 runs × 3M reps, loops in a separate translation unit, memcpy baseline subtracted.

variant	ns/frame
current plain cast	108.6
Float2Short (this PR)	1111.7
integer std::min/std::max clamp	1102.3

That is +1000 ns on a 2 670 000 ns budget, or +0.037%, and only on the path where the user has actually enabled the setting (iInputBoost != 1; the default is 1, which skips the loop entirely). The ratio is large because the plain cast vectorizes to pmullw and neither clamped form reaches packssdw on gcc 13 — but the absolute cost is not perceptible.

I measured the integer clamp as the alternative and it comes out the same, so I chose Float2Short for consistency with the rest of the function rather than for speed. Happy to switch if you prefer the integer form.

Caveat: timings are x86-64 only; I have not measured ARM.

Side note, not fixed here

SignalLevelMeter.Update runs after the boost, and CStereoSignalLevelMeter::Update measures negative peaks only (a deliberate speed optimization, every third sample). A positive sample that wraps becomes a large negative and reads as a peak; a negative sample that wraps becomes positive and is missed. So the level meter is not a dependable warning for this, in either direction. That seemed like a separate discussion, so I left it alone.

History

Introduced with the feature in #1222 (2021-04-01, e5bc010) and untouched since except by clang-format. I could not find any existing issue, PR, or discussion about it.

CHANGELOG: Bugfix: Input Boost now clips instead of wrapping around when the boosted signal exceeds full scale.

[ann0see]

Needs a proof that this bug actually existed.

[mcfnord]

Note

📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.

Fair request. The table in the PR body was computed from the expression, which is not the same as showing the bug happen. So here it is end-to-end, through the shipped code path, with no instrumented build and nothing patched except the fix itself.

Rig

A 1 kHz sine goes into a JACK dummy device, into a normal Jamulus client with Input Boost set from its ini, through opus and UDP, into a normal Jamulus server started with -R. The measurement is the server's own per-client WAV recording — so every sample measured came out of the encoder on the send path, not out of a test harness. Two builds: upstream 97799184 unmodified, and the same tree with this PR's two lines applied. Nothing else differs.

Result

0.5 s steady-state window (24 000 samples) from the middle of each take. "Tone vs everything else" is 1 kHz energy against all other energy above 40 Hz. "Steps over 32 768 LSB" counts adjacent samples differing by more than 32 768 LSB. A 1 kHz sine at 48 kHz cannot step by more than about 4 300 LSB even at full amplitude, so any such step is a discontinuity, not signal.

build	boost	input	tone vs everything else	steps over 32 768 LSB	matches
main	1x	−3 dBFS	+61.9 dB	0	control, no overflow
main	10x	−20 dBFS	+57.1 dB	0	control, lands exactly at full scale
main	2x	−3 dBFS	−4.0 dB	2000	wraps
main	4x	−10 dBFS	−6.9 dB	2000	wraps
main	10x	−14 dBFS	−15.4 dB	1952	wraps
this PR	2x	−3 dBFS	+17.3 dB	0	clips
this PR	10x	−14 dBFS	+11.8 dB	0	clips

The row that matters is 2x at −3 dBFS: that is the first entry in the dropdown, on a signal below full scale, and what arrives at the server has the user's tone 4 dB below the noise the wraparound generates. With the patch the same take has the tone 17 dB above, and those steps go to zero.

I also correlated each recording against both candidate models, a hard-clipped boost and an int16_t-narrowed boost, aligned within 200 samples. On main the recordings match the narrowed model at 0.72 to 0.99 and the clipped model at 0.14 to 0.48. With the patch that reverses.

Two things I want to state rather than let them be inferred:

• The two control rows are deliberate. −20 dBFS times 10 lands on exactly full scale and does not wrap, so it shows the rig producing a clean result when no overflow occurs. If the rig were manufacturing distortion, those rows would not read +57 and +62 dB.
• The patched rows sit below the controls (+17 and +12 rather than +57). That is correct and expected: hard clipping genuinely adds harmonics. The claim is not that the fix is transparent, it is that a gain control which runs out of headroom should distort by clipping rather than by inverting.

Measured on x86-64 Linux, opus at default quality. I have not run this on ARM or macOS.

Reproducing it

Two traps cost me a run each, so they are worth writing down:

1. Feedback detection has to be off. clientdlg.cpp:1100 mutes the channel and opens a modal box whenever an input meter pegs, which is exactly what an overflowing boost does. Set enablefeedbackdetection to 0 in the ini or you record silence.
2. A headless client ignores Input Boost entirely — see below. The client has to be the GUI one.

The rig is four small files — a JACK sine generator, the run script, the analyzer, and a README with the full result table and the traps: https://gist.github.com/mcfnord/85390a43901f96391d0b41ce467ac008

The whole method, if you would rather not fetch anything:

client.ini:

<?xml version="1.0" encoding="UTF-8"?>
<client>
 <inputboost>2</inputboost>
 <enablefeedbackdetection>0</enablefeedbackdetection>
</client>

jackd -R -d dummy -r 48000 -p 128 &
./Jamulus -s -n -p 22124 -R rec/ &
./gen 1000 -3 Jamulus &                            # 1 kHz sine at -3 dBFS
./Jamulus -i client.ini -j -c 127.0.0.1:22124      # GUI client; xvfb-run works
# stop after about 15 s; the WAV is under rec/Jam-*/

Open the WAV and look at it, or measure the 1 kHz band against the rest. At 2x the wraparound is clearly visible in the waveform: the sine flips polarity near every peak.

One separate bug found while building this

A headless client silently ignores inputboost. CClientSettings::ReadSettingsFromXML parses the ini value at settings.cpp:419, but the only call to CClient::SetInputBoost outside the settings dialog is clientdlg.cpp:268, which only runs when the GUI is constructed. Measured on the same rig: identical ini, client started with -n, recorded peak 6542 LSB where the boosted path gives 32 767 (unboosted peak at −14 dBFS is 6528).

That is unrelated to this PR and I have not opened anything for it. It may not be the only setting in that position, so if it is worth reporting I would rather first check whether other ini keys are applied only from the dialog and report them together. Happy to do that if you want it.

Why this may never have been reported

Speculation, offered because it is testable rather than as part of the argument: the bug hides behind the feedback detector. Turn Input Boost up far enough to wrap and the input meter pegs, so clientdlg.cpp:1100 mutes your channel and tells you that audio feedback or a loud signal was detected. That is a plausible-sounding message which sends the user looking for a feedback loop, not for a gain control that inverts. It is also why the level meter is not a usable warning here, as noted in the PR body.

[pljones]

A headless client silently ignores inputboost

Ugh, more "business logic" in the GUI.

Should be somewhere like CSound with CClient (or CSettings) owning the state, with the GUI (and JSONRPC) controlling the setting.

Separate issue for this...

[ann0see]

Ok. Nothing in the PR where this was initially introduced we had a comment by someone that the input boost is wrong from theoretical standpoint point. Likely it should be dB based. I think it's worth re investigating this. #1222 (comment)

[ann0see]

The fix does look sensible to me definitely. I think we do clip on the server with basically this code also.

[pljones]

This is:

static_cast<float> ( <int> ) * <short>

(e.g. 1.0 * 1) Is that going to work or is * <short> going to coerce <float> to <short> anyway?

Then it says:

<short> = Float2Short ( <short> )

if I'm right on the first bit.

I'm assuming -- given the naming convention vecs.... is a std::vector<short> (or QVector).

[mcfnord]

MY LLM WROTE:

The promotion goes the other way. float * short never coerces the float down; C++'s usual arithmetic conversions take the wider type, so the short is integer-promoted to int and then converted to float. The expression's static type is float, and Float2Short() receives a real float, not an already-narrowed short.

Your assumption about the vector is right, by the way: ProcessAudioDataIntern ( CVector<short>& vecsStereoSndCrd ) (client.h:322), and CVector<short> derives from std::vector<short>, so vecsStereoSndCrd[j] is a short lvalue.

Confirmed rather than asserted:

static_assert ( std::is_same< decltype ( static_cast<float> ( iInputBoost ) * vecsStereoSndCrd[0] ),
                              float >::value, "product is not float" );

compiles, and the whole loop is clean under -Wall -Wconversion.

The values, boost = 2:

input sample	old: static_cast<int16_t> ( int * short )	new: Float2Short ( float * short )	exact product
30000	−5536	32767	60000
−30000	5536	−32768	−60000
20000	−25536	32767	40000
32767	−2	32767	65534

So the old form is where the narrowing happened — the int product was truncated to 16 bits, which is the wraparound this PR fixes. The new form keeps full range until Float2Short() saturates it.

On precision: the largest magnitude reachable is 10 × 32768 = 327 680, far inside float's exactly-representable integer range (2^24), so nothing is lost to rounding before the clamp.

For what it's worth, this is the same idiom the pan path a few lines below already uses, unchanged from before this PR:

// client.cpp:1477
vecsStereoSndCrd[i] = Float2Short ( fGainL * vecsStereoSndCrd[j] + fGainR * vecsStereoSndCrd[j + 1] );

One thing you might legitimately want changed: the static_cast<float> is not load-bearing. iInputBoost * vecsStereoSndCrd[j] is int * int → int, which cannot overflow at the maximum boost of 10, and it would convert to float at the call to Float2Short() anyway. I kept the cast because it makes "this multiply is done in float" visible at the point it happens rather than at the callee's signature. Happy to drop it if you prefer the shorter line.