From c748c0726a8dd9a9ad7cb12e9ce6bb177de9b1a5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?kleines=20Filmr=C3=B6llchen?= <malu.bertsch@gmail.com>
Date: Fri, 17 Dec 2021 18:47:31 +0100
Subject: [PATCH] SoundPlayer: Don't enqueue samples depending on the GUI loop

Previously, SoundPlayer would read and enqueue samples in the GUI loop
(through a Timer). Apart from general problems with doing audio on the
GUI thread, this is particularly bad as the audio would lag or drop out
when the GUI lags (e.g. window resizes and moves, changing the
visualizer). As Piano does, now SoundPlayer enqueues more audio once the
audio server signals that a buffer has finished playing. The GUI-
dependent decoding is still kept as a "backup" and to start the entire
cycle, but it's not solely depended on. A queue of buffer IDs is used to
keep track of playing buffers and how many there are. The buffer
overhead, i.e. how many buffers "too many" currently exist, is currently
set to its absolute minimum of 2.
---
 .../Applications/SoundPlayer/PlaybackManager.cpp    | 13 +++++++++++--
 Userland/Applications/SoundPlayer/PlaybackManager.h |  6 +++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Userland/Applications/SoundPlayer/PlaybackManager.cpp b/Userland/Applications/SoundPlayer/PlaybackManager.cpp
index f9253ddf1d..b761509c1c 100644
--- a/Userland/Applications/SoundPlayer/PlaybackManager.cpp
+++ b/Userland/Applications/SoundPlayer/PlaybackManager.cpp
@@ -12,8 +12,16 @@ PlaybackManager::PlaybackManager(NonnullRefPtr<Audio::ClientConnection> connecti
     m_timer = Core::Timer::construct(PlaybackManager::update_rate_ms, [&]() {
         if (!m_loader)
             return;
-        next_buffer();
+        // Make sure that we have some buffers queued up at all times: an audio dropout is the last thing we want.
+        if (m_enqueued_buffers.size() < always_enqueued_buffer_count)
+            next_buffer();
     });
+    m_connection->on_finish_playing_buffer = [this](auto finished_buffer) {
+        auto last_buffer_in_queue = m_enqueued_buffers.dequeue();
+        // A fail here would mean that the server skipped one of our buffers, which is BAD.
+        VERIFY(last_buffer_in_queue == finished_buffer);
+        next_buffer();
+    };
     m_timer->stop();
     m_device_sample_rate = connection->get_sample_rate();
 }
@@ -117,7 +125,7 @@ void PlaybackManager::next_buffer()
         return;
     }
 
-    if (audio_server_remaining_samples < m_device_samples_per_buffer) {
+    if (audio_server_remaining_samples < m_device_samples_per_buffer * always_enqueued_buffer_count) {
         auto maybe_buffer = m_loader->get_more_samples(m_source_buffer_size_bytes);
         if (!maybe_buffer.is_error()) {
             m_current_buffer = maybe_buffer.release_value();
@@ -126,6 +134,7 @@ void PlaybackManager::next_buffer()
             // FIXME: Handle OOM better.
             m_current_buffer = MUST(Audio::resample_buffer(m_resampler.value(), *m_current_buffer));
             m_connection->enqueue(*m_current_buffer);
+            m_enqueued_buffers.enqueue(m_current_buffer->id());
         }
     }
 }
diff --git a/Userland/Applications/SoundPlayer/PlaybackManager.h b/Userland/Applications/SoundPlayer/PlaybackManager.h
index bbbe9f77e6..1a90408c1a 100644
--- a/Userland/Applications/SoundPlayer/PlaybackManager.h
+++ b/Userland/Applications/SoundPlayer/PlaybackManager.h
@@ -6,6 +6,7 @@
 
 #pragma once
 
+#include <AK/Queue.h>
 #include <AK/Vector.h>
 #include <LibAudio/Buffer.h>
 #include <LibAudio/ClientConnection.h>
@@ -39,6 +40,9 @@ public:
     Function<void()> on_finished_playing;
 
 private:
+    // Number of buffers we want to always keep enqueued.
+    static constexpr size_t always_enqueued_buffer_count = 2;
+
     void next_buffer();
     void set_paused(bool);
 
@@ -52,12 +56,12 @@ private:
     RefPtr<Audio::Loader> m_loader { nullptr };
     NonnullRefPtr<Audio::ClientConnection> m_connection;
     RefPtr<Audio::Buffer> m_current_buffer;
+    Queue<i32, always_enqueued_buffer_count + 1> m_enqueued_buffers;
     Optional<Audio::ResampleHelper<double>> m_resampler;
     RefPtr<Core::Timer> m_timer;
 
     // Controls the GUI update rate. A smaller value makes the visualizations nicer.
     static constexpr u32 update_rate_ms = 50;
-
     // Number of milliseconds of audio data contained in each audio buffer
     static constexpr u32 buffer_size_ms = 100;
 };