Simple Voice Chat

Simple Voice Chat

31M Downloads

Improve audio continuity with many clients

abernardi597 opened this issue ยท 21 comments

commented

Is your feature request related to a problem? Please describe.
Mod performance on the server seems to drop significantly when many players gather and attempt to speak simultaneously.
This results in choppy, nearly unintelligible audio delivered to all clients.

Describe the solution you'd like
Clear audio, even when there are many clients connected and speaking

Describe alternatives you've considered
The symptoms lead me to believe this is a performance issue where the server can't process all the incoming packets quickly enough.
Looking at the source I found a potential improvement.

In at least two instances, (de.maxhenkel.voicechat.voice.server.Server.packetQueue and de.maxhenkel.voicechat.voice.client.AudioChannel.queue), an ArrayList is being used as a queue to process packets.

This isn't very performant because removing the first element of an ArrayList shifts each successive element to the right (to fill in the empty space), which is linear with the worst case being the removal of the first element (and all other elements are shifted).

Instead, an actual Queue implementation would provide constant-time performance for removing the first element.
I'd recommend LinkedBlockingQueue so spin-locks aren't needed for consuming elements, as .take() will block for you until an element is available.
Upon closer inspection, it looks like a ConcurrentLinkedQueue may be better here because of the intermittent calls when idle.

I could draft a PR for this change, although I don't have an environment to compile/test with at the moment.
I drafted a PR that should compile, but it may require some testing.

Additional context
I can procure a video of the behavior if necessary

commented

Thank you for submitting that PR!
Have you tested your improvements?
How many clients are needed to reproduce this issue?

commented

I haven't had a chance to test it yet.
I saw the issue with ~7 clients on moderately powerful hardware. I may be able to test sometime in the next few days

commented

That would be great! Unfortunately I only have a maximum of 3 people to test it.

commented

I somewhat reproduced the issue with only 5 people on the same hardware. With every client talking, there were multiple reports of choppy audio.

I deployed my PR to some much slower hardware to verify the issue was gone, but none of the clients seemed to be receiving any audio at all.

In the server logs I saw some ConcurrentModificationException stack traces in the server logs:

[03:52:02] [VoiceChatPacketProcessingThread/INFO] [STDERR/]: [de.maxhenkel.voicechat.voice.server.Server$ProcessThread:run:193]: java.util.ConcurrentModificationException
[03:52:02] [VoiceChatPacketProcessingThread/INFO] [STDERR/]: [de.maxhenkel.voicechat.voice.server.Server$ProcessThread:run:193]:        at java.util.HashMap$HashIterator.nextNode(HashMap.java:1445)
[03:52:02] [VoiceChatPacketProcessingThread/INFO] [STDERR/]: [de.maxhenkel.voicechat.voice.server.Server$ProcessThread:run:193]:        at java.util.HashMap$ValueIterator.next(HashMap.java:1474)
[03:52:02] [VoiceChatPacketProcessingThread/INFO] [STDERR/]: [de.maxhenkel.voicechat.voice.server.Server$ProcessThread:run:193]:        at de.maxhenkel.voicechat.voice.server.Server.keepAlive(Server.java:206)
[03:52:02] [VoiceChatPacketProcessingThread/INFO] [STDERR/]: [de.maxhenkel.voicechat.voice.server.Server$ProcessThread:run:193]:        at de.maxhenkel.voicechat.voice.server.Server.access$100(Server.java:19)
[03:52:02] [VoiceChatPacketProcessingThread/INFO] [STDERR/]: [de.maxhenkel.voicechat.voice.server.Server$ProcessThread:run:193]:        at de.maxhenkel.voicechat.voice.server.Server$ProcessThread.run(Server.java:114)

Might be best to make this another issue with another PR, but I'm fairly certain this stems from iterating over connections.values() in a for-each loop that may modify the backing map (when calling disconnectClient due to a client failing to keep-alive).

I drafted a patch (untested) that should fix this:

diff --git a/src/main/java/de/maxhenkel/voicechat/voice/server/Server.java b/src/main/java/de/maxhenkel/voicechat/voice/server/Server.java
index 67e9340..181337f 100644
--- a/src/main/java/de/maxhenkel/voicechat/voice/server/Server.java
+++ b/src/main/java/de/maxhenkel/voicechat/voice/server/Server.java
@@ -203,22 +203,26 @@ public class Server extends Thread {
     private void keepAlive() throws IOException {
         long timestamp = System.currentTimeMillis();
         KeepAlivePacket keepAlive = new KeepAlivePacket();
+        List<UUID> connectionsToDrop = new ArrayList<>(connections.size());
         for (ClientConnection connection : connections.values()) {
             if (timestamp - connection.getLastKeepAliveResponse() >= Main.SERVER_CONFIG.keepAlive.get() * 10L) {
-                disconnectClient(connection.getPlayerUUID());
-                Main.LOGGER.info("Player {} timed out", connection.getPlayerUUID());
-                ServerPlayerEntity player = server.getPlayerList().getPlayerByUUID(connection.getPlayerUUID());
-                if (player != null) {
-                    Main.LOGGER.info("Reconnecting player {}", player.getDisplayName().getString());
-                    Main.SERVER_VOICE_EVENTS.initializePlayerConnection(player);
-                } else {
-                    Main.LOGGER.warn("Reconnecting player {} failed (Could not find player)", player.getDisplayName().getString());
-                }
+                connectionsToDrop.add(connection.getPlayerUUID());
             } else if (timestamp - connection.getLastKeepAlive() >= Main.SERVER_CONFIG.keepAlive.get()) {
                 connection.setLastKeepAlive(timestamp);
                 sendPacket(keepAlive, connection);
             }
         }
+        for (UUID uuid : connectionsToDrop) {
+            disconnectClient(uuid);
+            Main.LOGGER.info("Player {} timed out", uuid);
+            ServerPlayerEntity player = server.getPlayerList().getPlayerByUUID(uuid);
+            if (player != null) {
+                Main.LOGGER.info("Reconnecting player {}", player.getDisplayName().getString());
+                Main.SERVER_VOICE_EVENTS.initializePlayerConnection(player);
+            } else {
+                Main.LOGGER.warn("Reconnecting player {} failed (Could not find player)", player.getDisplayName().getString());
+            }
+        }
     }

     private boolean isPacketAuthorized(NetworkMessage message, UUID sender) {

For now I am unsure if this error cropped up because of the proposed changes or because I was overloading the slow hardware I was running the server on. I will try to do some more tests with the patch above to see what happens.

commented

Just add it to your PR.
Let me know if this fixes the issue.

commented

Great thanks!

I haven't been able to wrangle together enough people to load test again, but I can confirm the mod works with these changes with only one other client.

I think I want to make a tweak to the change and try to use a LinkedBlockingQueue and it's poll(Long, TimeUnit) with a timeout of 10ms instead of having the processing thread sleep for 10ms when there is nothing on the queue. This way, an incoming packet will be processed as soon as it arrives instead of waiting in the queue until the processing thread is done sleeping. I will try to get that updated today, and may be able to load test as soon as tonight (though tomorrow may be more likely).

commented

Great idea! Let me know if there are any updates.

commented

I added your proposed patch for the ConcurrentModificationException to the latest 1.16.5 Forge version, just to let you know.

commented

So I'm looking at the processing loop in AudioChannel that receives the packets and plays them.

I don't really understand why this bit is necessary though:

// Filling the speaker with silence for one packet size
// to build a small buffer to compensate for network latency
if (speaker.getBufferSize() - speaker.available() <= 0) {
    byte[] data = new byte[Math.min(AudioChannelConfig.getDataLength() * 2 * Main.CLIENT_CONFIG.outputBufferSize.get(), speaker.getBufferSize() - AudioChannelConfig.getDataLength())];
    speaker.write(data, 0, data.length);
}

To me it doesn't make sense to insert silence when we have perfectly good sound data in the queue, but I may be misunderstanding here.
Can you explain in more depth what this does and why it needs to be done?

commented

It basically fills the speaker with silence when it starts playing audio. This prevents voice crackling if voice packets take a little longer than the packet before to arrive. It is basically a little buffer, to mitigate packet loss or jitter.
I hope this was understandable :D

commented

Hey I wanted to ask if this fix is also coming for forge. Because Saturday we are planing to start a Rollplay Forge Project. Would be nice thanks i love youre work

commented

Sure!

commented

I update the PR with the LinkedBlockingQueue changes.

One noteworthy difference is that I changed the processing loop in AudioChannel to process only a single packet at a time (instead of gathering all the packets in the queue at once to write to the data line).
This shouldn't incur too much additional overhead (at least compared to the rate at which the buffer is consumed) but I wanted to point it out.

I will test out that it works tonight, and will possibly be able to load test as well.

commented

Yeah the gatherPacketData was a fix attempt for a problem that wasn't even there :D
Your changes look promising. Let me know if there are any performance improvements.

commented

Did a test with 6 clients on a relatively underpowered server and the results were much more clear/continuous than before!

There was still a little bit of skipping, but I think that may be due to a short TTL or UDP packets getting lost/dropped.

commented

Sounds great! Should I merge the PR or do you want to change something?

commented

I think it's good to go!

commented

Great! I'm gonna test it a little bit too and then merge it.

Do you want to create a PR for the fabric version or should I do it?

commented

I've never worked with Fabric before so it might be better for you to do it

commented

Alright, no problem.

Also, thank you very much for your contribution.

commented

Of course, I'm glad I could help out!