search

serenity-rs / songbird

An async Rust library for the Discord voice API
ISC License
384 stars 110 forks source link

Driver: Split receive into its own feature #141

Closed FelixMcFelix closed 2 years ago

FelixMcFelix commented 2 years ago

Adds the "receive" feature, which is disabled by default. When this is disabled, the UDP receive task is not compiled and not run, and as an optimisation the UDP receive buffer size is set to 0. All related events are also removed.

Closes #131.

GnomedDev commented 2 years ago

I know it would probably be a bit of a hassle, but performance could be further improved by getting rid of the udp_tx task entirely when receive is disabled, eg: https://github.com/GnomedDev/songbird/commit/d4b5d87c02eedab537fe68a2fb284ab467db0843

FelixMcFelix commented 2 years ago

I originally had a slight ideological issue with "putting a UDP send in sync code", but thinking on the packet cycle structure this is probably fine. If you step into the cycle a bit, you get send -> mix -> wait for (20 - x) ms -> .... Blocking on one send needs to overrun around 19.7ms to actually hurt the next packet, which shouldn't happen unless you're sending more traffic than the OS can serve (and the next frame will try to catch up).

The current UDP Tx design has some benefits (never blocks sender, unbounded), but costs a Vec alloc and ~300B memcpy every cycle. I put some thought into alternatives without a Vec like a shared buffer with ~1500B slots, but with a single slot you'll block on the same circumstances, and with m slots the sender still needs to keep its own sleep loop to prevent having m-1 20ms frames of output delay (i.e., by filling up the slots to send on). For the extra complexity I'm not sure it's worth it. This is a longwinded way of saying that returning to this UDP send method is probably a valid tradeoff -- low send overhead, can block but low risk, no added delay, no writing sync<->async shared buffers.

The only thing about the patch you link is that it also drops sending UDP keepalives -- I suppose you can get away without them if you know you're running on dedicated infra, but they're generally there to make sure that NAT entries don't vanish during silent periods for folks running { songbird, Discord itself } from a home machine. I could just stick them after packet sends to keep the sleep logic simple.

FelixMcFelix commented 2 years ago

Quick test on "receive" suggests that a non-blocking std::net::UdpSocket for Tx should be okay, so try_clone etc. is a valid way to remove the udp_tx task in all cases, I'll tidy up shortly, have integrated keepalives too.