Add support for receiving voice and opening up the voice api #386

lost-illusi0n · 2021-09-06T22:28:02Z

All incoming packets are abstracted away into the Streams class where it provides a few different flows in different states of processing.

Attempted to open up the api.

lukellmann

In addition to my comments, I was wondering whether you can disable receiving voice if you don't need it.

lukellmann · 2021-09-07T15:53:05Z

voice/src/main/kotlin/Streams.kt

+        SupervisorJob() + dispatcher + CoroutineName("Voice Connection Incoming Streams")
+
+    // this will be set before it is used as the key is received before the udp connection is even established
+    internal lateinit var key: ByteArray


This is not thread safe, there is no happens-before relationship between setting and using key.
-> @Volatile or atomic?

lukellmann · 2021-09-07T15:53:27Z

voice/src/main/kotlin/Streams.kt

+    private val _ssrcToUser: MutableMap<UInt, Snowflake> = mutableMapOf()
+
+    /**
+     * A mapping of ssrc to user id.
+     * This cache is built over time through the [dev.kord.voice.gateway.VoiceGateway].
+     */
+    val ssrcToUser: Map<UInt, Snowflake> get() = _ssrcToUser


Not thread safe -> ConcurrentHashMap?

not something we can do easily if we want to go multiplatform. next point should be enough

Multiplatform could use some sort of expect and actual with ConcurrentHashMap on JVM

Still the same problem, we'd have to find different implementations for each platform. The actual mutablemap is private, and no other unsafe operations are being done.

Sure, the same would be true for other caches. Problem is that this map could be edited and accessed from any thread, so there is no guarantee for changes to become visible.

The multiplatform solution is to make the variable atomic and interactions with the map sequential. You can achieve the latter by consuming the flow directly without launching.

lukellmann · 2021-09-07T15:53:34Z

voice/src/main/kotlin/Streams.kt

+            if (ssrcToUser[ssrc] == null) {
+                _ssrcToUser[ssrc] = userId


computeIfAbsent (especially with ConcurrentHashMap)

lukellmann · 2021-09-07T15:55:47Z

voice/src/main/kotlin/gateway/VoiceGateway.kt

+ * Convenience method that will invoke the [consumer] on every event [T] created by [VoiceGateway.events].
+ *
+ * The events are buffered in an [unlimited][Channel.UNLIMITED] [buffer][Flow.buffer] and
+ * [launched][CoroutineScope.launch] in the supplied [scope], which is [Gateway] by default.


should be [VoiceGateway] instead of [Gateway] in the documentation comment

lukellmann · 2021-09-07T15:56:41Z

voice/src/main/kotlin/handlers/UdpLifeCycleHandler.kt

 ) : EventHandler<VoiceEvent>(flow, "UdpInterceptor") {
-    private var framePollerConfigurationBuilder = AudioFramePollerConfigurationBuilder()
-    private var udp: DiscordUdpConnection? = null
+    private var ssrc by Delegates.notNull<UInt>()


This is not thread safe, there is no happens-before relationship between setting and using ssrc.
-> @Volatile or atomic?

lukellmann · 2021-09-07T15:57:49Z

voice/src/main/kotlin/udp/AudioFrameSender.kt

+        } catch (e: Exception) {
+            /* we're done polling, nothing to worry about */
+        }


Where does the Exception come from?

anything that could cause the flow to terminate.
kord follows this same procedure in DefaultGatway and DefaultVoiceGateway

Alright, just me beeing curious

voice/src/main/kotlin/udp/AudioPacket.kt

    @OptIn(ExperimentalIoApi::class)
-    private val header = BytePacketBuilder().also {
+    val header = BytePacketBuilder().also {


lukellmann · 2021-09-07T15:58:11Z

voice/src/main/kotlin/udp/AudioPacket.kt

+            header.copyInto(nonce, 0, 0, RTP_HEADER_LENGTH)
+            val decrypted = XSalsa20Poly1305Codec.decrypt(data, key, nonce) ?: error("couldn't decrypt audio data")
+            return DecryptedPacket(sequence, timestamp, ssrc, decrypted)
+        }
    }

    fun asByteReadPacket() = BytePacketBuilder().also {


apply instead of also?

lukellmann · 2021-09-07T15:58:28Z

voice/src/main/kotlin/udp/AudioPacket.kt

+        sequence: UShort,
+        timestamp: UInt,
+        ssrc: UInt,
+        encryptedData: ByteArray


should be decryptedData instead of encryptedData

lukellmann · 2021-09-07T15:58:58Z

voice/src/main/kotlin/udp/DefaultVoiceUdpConnection.kt

+    private lateinit var socket: ConnectedDatagramSocket
+    private lateinit var configuration: VoiceUdpConnectionConfiguration


This is not thread safe, there is no happens-before relationship between setting and using socket and configuration (between start, discoverIp and send).
-> @Volatile or atomic?

The lateinit socket is something done in Gateway and VoiceGateway, I think it would be best to rework the concurrency structure of each gateway/connection in its own pr, which I think is being done.

i don't mind making configuration atomic, however.

lost-illusi0n · 2021-09-07T19:01:55Z

do you have an idea in mind how disabling/enabling receiving voice would look like?

lukellmann · 2021-09-07T19:05:50Z

do you have an idea in mind how disabling/enabling receiving voice would look like?

Maybe a Boolean setting in VoiceConnectionBuilder, disabled by default and some Noop Streams implementation?

lukellmann · 2021-09-08T00:10:20Z

voice/src/main/kotlin/Streams.kt

+    private val _incomingUserAudioFrames: MutableSharedFlow<Pair<Snowflake, AudioFrame>> = MutableSharedFlow()
+
+    /**
+     * A flow of incoming [AudioFrame]s mapped to its corresponding user id. Streams for each user are built over time,
+     * or whenever the [dev.kord.voice.gateway.VoiceGateway] receives a [Speaking] event.
+     */
+    val incomingUserStreams: SharedFlow<Pair<Snowflake, AudioFrame>> = _incomingUserAudioFrames


incomingUserStreams could also use _incomingUserAudioFrames.asSharedFlow() to forbid casting

This pattern is used a few other places throughout Kord, is there any other benefit to use asSharedFlow?

Nothing I know about, this is not a must (but I thought I could mention it).

lukellmann · 2021-09-08T00:13:23Z

voice/src/main/kotlin/gateway/VoiceGateway.kt

+inline fun <reified T : VoiceEvent> VoiceGateway.on(
+    scope: CoroutineScope = this,
+    crossinline consumer: suspend T.() -> Unit
+): Job {
+    return this.events.buffer(Channel.UNLIMITED).filterIsInstance<T>().onEach {
+        launch { it.runCatching { it.consumer() }.onFailure(voiceGatewayOnLogger::error) }
+    }.launchIn(scope)
+}


This should not launch inside onEach to make sure that changes to the MutableMap in Streams are sequential (see @BartArys comment). Other solution would be to not use VoiceGateway.on() in Streams.

This function should stay the same, as it would be confusing that the VoiceGateway.on functions differently from Gateway.on
Though I now understand what you meant by not using launch, I did not realize it was used here :p

connection.voiceGateway.events.buffer(Channel.UNLIMITED).filterIsInstance<Speaking>().onEach { speaking -> // ... }.launchIn(this)

in init for Streams should do the trick then

lukellmann · 2021-09-08T00:17:58Z

voice/src/main/kotlin/udp/DefaultVoiceUdpConnection.kt

+    private val _incoming: MutableSharedFlow<ByteReadPacket> = MutableSharedFlow()
+    override val incoming: SharedFlow<ByteReadPacket> = _incoming


incoming could also use _incoming.asSharedFlow() to forbid casting

lukellmann · 2021-09-08T19:03:59Z

voice/src/main/kotlin/handlers/StreamsHandler.kt

+package dev.kord.voice.handlers
+
+import dev.kord.common.annotation.KordVoice
+import dev.kord.voice.Streams


wrong package, won't compile (should import dev.kord.voice.streams.Streams)

intellij cannot refactor packages apparently, or properly at least :-(

lukellmann · 2021-09-08T19:04:03Z

voice/src/main/kotlin/streams/Streams.kt

+@KordVoice
+abstract class Streams {
+    /**
+     * An encryption key used for decryption of Discord packets.
+     */
+    internal abstract var key: ByteArray?


abstract class with internal abstract member cannot be inherited outside of voice module

was a bit too easy i guess :p

lost-illusi0n · 2021-09-11T17:23:34Z

it seems like discord uses much more of the rtp spec than implemented before, and it differs from voice server to voice server. time to implement that

lost-illusi0n · 2021-09-13T05:58:26Z

there may or may not be a memory leak that may or may not have to be fixed.

HopeBaron · 2021-09-14T09:54:09Z

for what reason?

lost-illusi0n · 2021-09-15T05:07:33Z

not completely sure yet but something is definitely there. here is a graph from jprofiler

this is not apparent when running a test bot without a voice connection.

lost-illusi0n · 2021-09-15T07:05:29Z

Seems like there are actually two memory leaks, one major, and one much, much more minor.
I believe I fixed the major one, now only the minor one left. It seems to be in the default audio frame sender.

Signed-off-by: Lost <[email protected]>

Declaring the version catalog in libs.versions.toml instead of settings.gradle.kts has the advantage that it can be reused in buildSrc. It also introduces a single place to define dependencies and their version. General cleanup of gradle files was also done, including: * using default output for dokka: relying on defaults means less config and maintenance burden * removing DocsTask because it was unused, last usage of this custom task was removed in #367 * getting rid of source set workaround introduced in #386 by moving TweetNaclFast from voice/src/main/kotlin to voice/src/main/java

Declaring the version catalog in libs.versions.toml instead of settings.gradle.kts has the advantage that it can be reused in buildSrc. It also introduces a single place to define dependencies and their version. General cleanup of gradle files was also done, including: * using default output for dokka: relying on defaults means less config and maintenance burden * removing DocsTask because it was unused, last usage of this custom task was removed in kordlib#367 * getting rid of the source set workaround introduced in kordlib#386 by moving TweetNaclFast from voice/src/main/kotlin to voice/src/main/java

Declaring the version catalog in libs.versions.toml instead of settings.gradle.kts has the advantage that it can be reused in buildSrc. It also introduces a single place to define dependencies and their version. General cleanup of gradle files was also done, including: * using default output for dokka: relying on defaults means less config and maintenance burden * removing DocsTask because it was unused, last usage of this custom task was removed in #367 * getting rid of the source set workaround introduced in #386 by moving TweetNaclFast from voice/src/main/kotlin to voice/src/main/java * removing io.codearte.nexus-staging plugin introduced in #154 since the tasks of the plugin were never used (Nexus GUI was used manually instead)

lukellmann requested changes Sep 7, 2021

View reviewed changes

lost-illusi0n force-pushed the features/0.8.x/receiving branch from 41c4300 to c3537d5 Compare September 7, 2021 23:18

lukellmann requested changes Sep 8, 2021

View reviewed changes

lukellmann reviewed Sep 8, 2021

View reviewed changes

lost-illusi0n force-pushed the features/0.8.x/receiving branch from 025e481 to 42e0ee4 Compare September 8, 2021 20:28

lukellmann approved these changes Sep 9, 2021

View reviewed changes

lost-illusi0n marked this pull request as draft September 11, 2021 17:22

lost-illusi0n marked this pull request as ready for review September 12, 2021 02:59

lost-illusi0n force-pushed the features/0.8.x/receiving branch from b6aee63 to e3a879d Compare September 12, 2021 03:01

lost-illusi0n marked this pull request as draft September 13, 2021 05:58

lost-illusi0n added 13 commits October 11, 2021 00:26

rework udp connection

238ba42

rework audio poller

ea77a2e

minor gateway changes

ce04273

add decryption to xsalsa codec

9090891

add streams and minor changes to voice connection

b377c16

concurrency is a thing, among other minor changes

089edea

better atomics

585546d

add missing KordVoice

d0f3678

move default audioframesender impl into separate file

f46a423

sequential processing for user streams

0f223b2

intellij messed up packages

d9a74f5

allow for different implementations of streams, nop by default

b99e865

better handling of discord-spec rtp packets

1c5d273

Signed-off-by: Lost <[email protected]>

lost-illusi0n added 8 commits October 11, 2021 00:27

kord io

2a493e6

replace encryption implementation

8fa2d73

move connect to behavior

8f8d29d

replace bounded udp channels with actual udp sockets

1452ab8

remove AudioPackets

4e04407

update streams

4703fea

connection builder fix

82760f4

invert logic in voice update handler

47ebd69

lost-illusi0n force-pushed the features/0.8.x/receiving branch from c0e5c0b to 47ebd69 Compare October 11, 2021 04:30

lost-illusi0n marked this pull request as ready for review October 11, 2021 04:31

lost-illusi0n added 4 commits October 11, 2021 01:16

core explicit api

6c903c7

fix intellij's dislike of RTPPacket

cc1b498

global socket shouldn't stop

869f961

external port may actually change in the global udp socket

8877a4c

lost-illusi0n mentioned this pull request Oct 15, 2021

Voice Concurrency #423

Merged

HopeBaron merged commit c1e1bf5 into kordlib:0.8.x Oct 18, 2021

lukellmann mentioned this pull request Nov 12, 2022

Use libs.versions.toml and clean gradle files #719

Merged

		private lateinit var socket: ConnectedDatagramSocket
		private lateinit var configuration: VoiceUdpConnectionConfiguration

		private val _incoming: MutableSharedFlow<ByteReadPacket> = MutableSharedFlow()
		override val incoming: SharedFlow<ByteReadPacket> = _incoming

Add support for receiving voice and opening up the voice api #386

Add support for receiving voice and opening up the voice api #386

Conversation

lost-illusi0n commented Sep 6, 2021

lukellmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lost-illusi0n commented Sep 7, 2021 • edited Loading

lukellmann commented Sep 7, 2021

Choose a reason for hiding this comment

lost-illusi0n Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukellmann Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lost-illusi0n commented Sep 11, 2021

lost-illusi0n commented Sep 13, 2021

HopeBaron commented Sep 14, 2021

lost-illusi0n commented Sep 15, 2021

lost-illusi0n commented Sep 15, 2021

lost-illusi0n commented Sep 7, 2021 •

edited

Loading

lost-illusi0n Sep 8, 2021 •

edited

Loading

lukellmann Sep 8, 2021 •

edited

Loading