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.
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.
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.
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.