Skip to content

Conversation

@whatisor
Copy link

@whatisor whatisor commented Nov 6, 2025

Mixing without overflow checking will cause static noise.

@januscla
Copy link

januscla commented Nov 6, 2025

Thanks for your contribution, @whatisor! Please make sure you sign our CLA, as it's a required step before we can merge this.

@lminiero
Copy link
Member

lminiero commented Nov 6, 2025

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.

@tmatth
Copy link
Contributor

tmatth commented Nov 6, 2025

FWIW opus comes with a nice implementation to this end but it's for floating point audio:
https://gitlab.xiph.org/xiph/opus/-/blob/main/include/opus.h?ref_type=heads#L800

@lminiero
Copy link
Member

lminiero commented Nov 6, 2025

FWIW opus comes with a nice implementation to this end but it's for floating point audio:
https://gitlab.xiph.org/xiph/opus/-/blob/main/include/opus.h?ref_type=heads#L800

Turning the samples to float and calling opus_encode_float instead might be an option, but that would be a transformation anyway, so it might be easier to do it ourselves with what we have.

@whatisor
Copy link
Author

whatisor commented Nov 6, 2025

@lminiero You are correct, that is much more efficient because you already use int buffer for sum.
I think it works in my test.

Copy link
Member

@lminiero lminiero left a 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!


/* 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
Copy link
Member

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.

#define SHRT_MIN_TUNED -29491

/* Optimized inline function to clamp 16-bit audio samples */
static inline opus_int16 overflow_check(int sum) {
Copy link
Member

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.

@lminiero
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants