-
Notifications
You must be signed in to change notification settings - Fork 49
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
Opus DTX support #492
Opus DTX support #492
Conversation
src/format.rs
Outdated
// If neither value is specified both sides should assume DTX is not used as this is | ||
// the default. | ||
let either_dtx_specified = c0.format.use_dtx.is_some() || c1.format.use_dtx.is_some(); | ||
if either_dtx_specified && c0.format.use_dtx != c1.format.use_dtx { | ||
score = score.saturating_sub(1); | ||
} |
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.
I wasn't sure how much to lower the score for a DTX mismatch. I notice minptime
lowers it by 1 and useinbandfec
lowers it by 2. Do they need to be distinct penalties?
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.
I think I might have aimed for making it deterministic what the sorting order is.
I.e. minptime
and use_dtx
shouldn't result in the same score.
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.
Hmm, if I had to put an ordering, then, I think DTX differences should be the biggest penalty, due to both the large difference in packet volume (and resulting bandwidth) between having it on vs. off, and the claimed small ding in audio quality.
If we want a deterministic sorting order, then, it sounds like we need to decrease the score by 4 in that case, since 3 would overlap with a simultaneous difference in minptime
and useinbandfec
. I'll make that change now.
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.
Apart from the sort order penalty, I think this is ready to go.
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.
Let's ship it!
Thanks! |
This adds support for an Opus format param
usedtx
, which signals a receiver's preference for DTX. DTX stands for Discontinuous Transmission, and it's a feature in Opus that lowers the packet rate, not just the bitrate, during periods of silence. For instance, in my WebRTC environment, Chrome and Firefox will reduce the packet rate from 50 packets per second to 4 during silence. This makes it ideal for SFU-type applications, where any particular person spends most of the time not talking, and so we can save the CPU time spent processing such packets.As mentioned in the above link, it does have slight effects on the resulting audio, and browsers don't currently negotiate it without SDP munging, so here I've set it off by default.
Known caveat: at a reduced rate of 4 packets per second, if your
StreamRx.pause_threshold
is less than 250 ms, str0m will emit stream pause/unpause events between each packet. That said, this could be considered expected behavior.