-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix overflow mixing #3601
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
base: master
Are you sure you want to change the base?
Fix overflow mixing #3601
Conversation
|
I think this may more easily be done in a single place, which is where we define the buffer we'll actually send, rather than in all places where we do a sum: https://github.com/meetecho/janus-gateway/blob/master/src/plugins/janus_audiobridge.c#L8908-L8910 We use an int32 on purpose to do mixing, to allow us to go beyond, and do N-1 mixing effectively. My guess is that N-1 could be broken if the source is touched and the mix clipped. |
|
FWIW opus comes with a nice implementation to this end but it's for floating point audio: |
Turning the samples to float and calling |
|
@lminiero You are correct, that is much more efficient because you already use int buffer for sum. |
6736cae to
d88e257
Compare
lminiero
left a comment
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.
Added a few notes inline, thanks!
src/plugins/janus_audiobridge.c
Outdated
|
|
||
| /* Audio mixing with overflow protection - tuned to 0.9 of MAX/MIN to prevent clipping */ | ||
| #define SHRT_MAX_TUNED 29491 | ||
| #define SHRT_MIN_TUNED -29491 |
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.
To avoid risks of clashes, better to add a unique prefix to those defines, e.g. JANUS_AUDIOBRIDGE_SHRT_MAX_TUNED etc.
src/plugins/janus_audiobridge.c
Outdated
| #define SHRT_MIN_TUNED -29491 | ||
|
|
||
| /* Optimized inline function to clamp 16-bit audio samples */ | ||
| static inline opus_int16 overflow_check(int sum) { |
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.
Same thing for this function here, so something like janus_audiobridge_overflow_check.
As a side note, better to define sum as int32_t, as that's what the original sumBuffer is. A generic int should be fine in general anyway, but considering this could be compiled on different systems, better be explicit about it and avoid surprises.
5bd5b72 to
21ebec0
Compare
|
I think this looks good, in general, but is incomplete. At the moment this only smoothens the mix meant to be sent back to participants, but ignores the places where we do the same sum for the recording (L8839 in current master) and for RTP forwarders (L8969, L8994, L8999), meaning the audio could still be saturated there. |
Mixing without overflow checking will cause static noise.