Voice overhaul #1593
Voice overhaul #1593
Conversation
## Summary
- Use context to manage goroutines and connections
- Eliminate busy wait using sync.Cond
- Use Voice Gateway Version 8
- Change to new encryption mode
- Resume Voice Conenction
- Delete reconnect (excludes resume)
- It is very rare and it should be handled by an application
- Deliver unrecoverable error to the application
- Edit some examples to work with these changes
## Breaking changes
- misc
- Some functions require context.Context in argument
- Session
- Renamed ChannelVoiceJoinManual to VoiceStateUpdate
- Remove ShouldReconnectVoiceOnSessionError
- Because reconnecting feature are deleted
- VoiceConenction
- Removed Ready
- Use Status instead
- Removed Debug
- already deprecated
- Removed ChangeChannel
- Use Session.VoiceStateUpdate
- Removed Close
- Kill is alternative, but why not use Disconnect?
- Packet
- Removed Type
- Separated to Flags and PayloadType
|
Thank you! Discord starts phasing out old encryption and some A/B testing shenanigans are going on right now. As a result certain voice channels do not work with |
| if !v.deaf { | ||
| if v.OpusRecv == nil { | ||
| v.OpusRecv = make(chan *Packet, 2) | ||
| case 2: // READY |
There was a problem hiding this comment.
since you are doing overhaul let's fix few extra things and define these values in case statments as consts
There was a problem hiding this comment.
It would be used only here, so why comment is not enough?
There was a problem hiding this comment.
i'm referring to all cases in this switch statement. For me, it's a preference and will help avoid confusion in the future. case 5: does not have a comment, for example. Additionally, people will change code but will forget to change comments which may cause confusion, if we have consts in place of 2, 3, 4, 5 code will be more self documenting and comments like // READY won't be needed.
Of course these are the rules i like to follow, i'm just suggesting, not requiring.
There was a problem hiding this comment.
I agree, it's better to declare these values as constants.
| if v.sessionID != "" { | ||
| break | ||
| } | ||
| go v.websocket(context.TODO(), *ev.Endpoint, ev.Token) |
There was a problem hiding this comment.
what's the plan with this context.TODO()?
VoiceConnection.websocket is used only here so maybe remove context.Context argument and initialize it inside VoiceConnection.websocket function
There was a problem hiding this comment.
- Normally, top-level context should be generated in the top-level function (= main() of the application), but discordgo(.Session) doesn't accept it. So it is 'TODO'.
- Functions related to Session (ex: Session.Open, Session.listen) may also have context to manage the lifecycle, like this patch.
It may deprecate Session.Close() and replace with cancel of context, and passing that context to here would resolveTODO: Add support for Voice WS/UDP(in the comment of Session.Close)
It is out of the scope of this patch, but websocket() supports being canceled from the caller.
voice.go
Outdated
| go v.reconnect() | ||
| // 4014 indicates a manual disconnection by someone in the guild; | ||
| // we shouldn't reconnect. | ||
| if websocket.IsCloseError(err, 4014) { |
There was a problem hiding this comment.
maybe we could name 4014 and 4015 too?
const manualDisconnect = 4014 and ourCodeIsBad = 4015
|
This fixed the issues for me, good job. |
Co-authored-by: fr-str <73256873+fr-str@users.noreply.github.com>
Co-authored-by: fr-str <73256873+fr-str@users.noreply.github.com>
Co-authored-by: fr-str <73256873+fr-str@users.noreply.github.com>
ozraru
left a comment
ozraru
left a comment
There was a problem hiding this comment.
Thanks for your review!
I wrote response or merged each comment or suggestion.
| if v.sessionID != "" { | ||
| break | ||
| } | ||
| go v.websocket(context.TODO(), *ev.Endpoint, ev.Token) |
There was a problem hiding this comment.
- Normally, top-level context should be generated in the top-level function (= main() of the application), but discordgo(.Session) doesn't accept it. So it is 'TODO'.
- Functions related to Session (ex: Session.Open, Session.listen) may also have context to manage the lifecycle, like this patch.
It may deprecate Session.Close() and replace with cancel of context, and passing that context to here would resolveTODO: Add support for Voice WS/UDP(in the comment of Session.Close)
It is out of the scope of this patch, but websocket() supports being canceled from the caller.
| if !v.deaf { | ||
| if v.OpusRecv == nil { | ||
| v.OpusRecv = make(chan *Packet, 2) | ||
| case 2: // READY |
There was a problem hiding this comment.
It would be used only here, so why comment is not enough?
This branch contains the voice overhaul changes from PR bwmarrin#1593 that fix: - Discord voice connection 'Unknown encryption mode' error (close code 4016) - Modern encryption mode negotiation (aead_aes256_gcm_rtpsize, aead_xchacha20_poly1305_rtpsize) - Improved voice connection stability - Resolves Speaker 2 connection failures in ShotCaller bot Based on: bwmarrin#1593 Applied for: ShotCaller Discord bot voice relay system
This branch contains the voice overhaul changes from PR bwmarrin#1593 that fix: - Discord voice connection 'Unknown encryption mode' error (close code 4016) - Modern encryption mode negotiation (aead_aes256_gcm_rtpsize, aead_xchacha20_poly1305_rtpsize) - Improved voice connection stability - Resolves Speaker 2 connection failures in ShotCaller bot Based on: bwmarrin#1593 Applied for: ShotCaller Discord bot voice relay system
| v.Cond.Broadcast() | ||
| close(v.dead) | ||
| go func() { | ||
| time.Sleep(100 * time.Millisecond) // safe |
|
|
||
| return | ||
| } | ||
| for i := 0; i < 100; i++ { |
There was a problem hiding this comment.
The number of reconnection attempts should be configurable.
| v.Kill() | ||
| v.Disconnect(context.Background()) |
| // Start reconnect goroutine then exit. | ||
| go v.reconnect() | ||
| // reconnect | ||
| break |
There was a problem hiding this comment.
The old code used to have
<-time.After(wait * time.Second)
wait *= 2
Should a wait be added here?
| time.Sleep(100 * time.Millisecond) // safe | ||
| close(v.OpusRecv) | ||
| close(v.OpusSend) |
There was a problem hiding this comment.
This looks like a potential race condition? Are you sure it's always safe?
|
Could you make a proper example of voice receiver? I can't get it to work, and |
|
Hi, |
|
@ozraru Your branch works well, but after I while I get: voice.go:207:failure() voice unrecoverable error, voice websocket closed, websocket: close 4022: Disconnected: Call terminated. and the audio connections is not recovered. |
@galberti This has been fixed in #1629. These changes probably would have to be applied similarly in this branch. Vendoring discordgo and merging PRs with your fork manually is probably the best idea |
|
@RadCraftplay thank you for your answer. It is indeed fixed, but merging the 2 branches together isn't the easiest of the jobs :) |
It's because it's using the release version of discordgo. However, it's still not working in my environment and I haven't been able to find the cause. I confirmed it was working when I wrote this PR, so the cause could be a change by Discord. |
|
I have (partially) merged #1629. The close code problem should be fixed. |
|
From a quick look, Hedede's review looks accurate and I think that I should resolve that. However, I currently don't have enough time to review it in detail. I'll get to it when I have time. |
|
Adding this diff to the voice_receive example doesn't print out anything (though I doubt it did anything without this patch). diff --git a/examples/voice_receive/main.go b/examples/voice_receive/main.go
index fbea9f3..b12317e 100644
--- a/examples/voice_receive/main.go
+++ b/examples/voice_receive/main.go
@@ -90,6 +90,10 @@ func main() {
return
}
+ v.AddHandler(func (vc *discordgo.VoiceConnection, vs *discordgo.VoiceSpeakingUpdate) {
+ fmt.Printf("%+v\n", vs)
+ })
+
go func() {
time.Sleep(10 * time.Second)
v.Disconnect(context.Background())When I debug this, op 5 is hit once and early returns on the lack of handlers. I'd really like to have this working, so I'll keep debugging to try and find the cause. |
I compared packet captures between this pr and discord.js when connecting to a channel. (I can't share the pcaps for security/privacy reasons, unless I spend a lot of effort anonymising it) Some noteworthy differences/messages:
Long story short, it seems like receiving I might give the dave protocol whitepaper a read, and maybe give an attempt at an implementation. |
8 participants
I've rewrited entire code related to Voice.
This is very big pull request.
I'm sorry, I understand that it is difficult to review.
But I believe that this is valuable to check.
I'm not good at English. Wording fixes are welcome.
I'm not sure about the naming. Naming changes are also welcome.
I want to support E2EE in the future but it's not included in this pull request.
Summary
Use context to manage goroutines and connections
Eliminate busy wait using sync.Cond
Use Voice Gateway Version 8
Change to new transport encryption mode
aead_aes256_gcm_rtpsizeandaead_xchacha20_poly1305_rtpsizeResume Voice Conenction
Delete reconnection (excludes resume)
Deliver unrecoverable error to the application
Edit some examples to work with these changes
Breaking changes
note: Currently using transport encryption mode(
xsalsa20_poly1305) and voice gateway version(connecting without a version) is already discontinued according to the document. (but it works currently 🤔 )