Skip to content

Fix Input Boost wrapping around instead of clipping #3834

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.

Sign up for GitHub

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

Open
mcfnord wants to merge 1 commit into
base: main
Choose a base branch
from
+2 −2
Open

Fix Input Boost wrapping around instead of clipping #3834

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1429,8 +1429,8 @@ void CClient::ProcessAudioDataIntern ( CVector<int16_t>& vecsStereoSndCrd )
// apply a general gain boost to all audio input:
for ( i = 0, j = 0; i < iMonoBlockSizeSam; i++, j += 2 )
{
vecsStereoSndCrd[j + 1] = static_cast<int16_t> ( iInputBoost * vecsStereoSndCrd[j + 1] );
vecsStereoSndCrd[j] = static_cast<int16_t> ( iInputBoost * vecsStereoSndCrd[j] );
vecsStereoSndCrd[j + 1] = Float2Short ( static_cast<float> ( iInputBoost ) * vecsStereoSndCrd[j + 1] );

@pljones pljones Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mcfnord Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

vecsStereoSndCrd[j] = Float2Short ( static_cast<float> ( iInputBoost ) * vecsStereoSndCrd[j] );

@ann0see ann0see Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

Expand Down
Loading