From 7c2d493bb525450367bca09aa860478cfbeadafb Mon Sep 17 00:00:00 2001
From: Chris Spencer <spencercw@gmail.com>
Date: Tue, 22 Feb 2022 09:21:36 +0000
Subject: [PATCH] [client] audio: add latency tuning parameter

This adds a new `audio:bufferLatency` option which allows the user to
adjust the amount of buffering LG does over the absolute bare minimum. By
default, this is set large enough to absorb typical timing jitter from
Spice. Users may reduce this if they care more about latency than audio
quality.
---
 client/src/audio.c  | 28 +++++++++++++++++++---------
 client/src/config.c |  8 ++++++++
 client/src/main.h   |  1 +
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/client/src/audio.c b/client/src/audio.c
index a9363be0..5bea1052 100644
--- a/client/src/audio.c
+++ b/client/src/audio.c
@@ -517,16 +517,26 @@ void audio_playbackData(uint8_t * data, size_t size)
     spiceData->devNextPosition = deviceTick.nextPosition;
   }
 
-  // Determine the target latency. Ideally, this would be precisely equal to
-  // the maximum device period size. However, we need to allow for some timing
-  // jitter to avoid underruns. Packets from Spice in particular can sometimes
-  // be delayed by an entire period or more, so include a fixed amount of
-  // latency to absorb these gaps. For device jitter use a multiplier, so timing
-  // requirements get progressively stricter as the period size is reduced
-  int spiceJitterMs = 13;
+  // Determine the target latency. This is made up of three components:
+  // 1. Half the Spice period. This is necessary due to the way qemu handles
+  //    audio. Data is not sent as soon as it is produced by the virtual sound
+  //    card; instead, qemu polls for new data every ~10ms. This results in a
+  //    sawtooth pattern in the packet timing as it drifts in and out of phase
+  //    with the virtual device. LG measures the average progression of the
+  //    Spice clock, so sees the packet timing error drift by half a period
+  //    above and below the measured clock. We need to account for this in the
+  //    target latency to avoid underrunning.
+  // 2. The maximum audio device period, plus a little extra to absorb timing
+  //    jitter.
+  // 3. A configurable additional buffer period. The default value is set high
+  //    enough to absorb typical timing jitter from Spice, which can be quite
+  //    significant. Users may reduce this if they care more about latency than
+  //    audio quality.
+  int configLatencyMs = max(g_params.audioBufferLatency, 0);
   double targetLatencyFrames =
-    spiceJitterMs * audio.playback.sampleRate / 1000.0 +
-    audio.playback.deviceMaxPeriodFrames * 1.1;
+    spiceData->periodFrames / 2.0 +
+    audio.playback.deviceMaxPeriodFrames * 1.1 +
+    configLatencyMs * audio.playback.sampleRate / 1000.0;
 
   // If the device is currently at a lower period size than its maximum (which
   // can happen, for example, if another application has requested a lower
diff --git a/client/src/config.c b/client/src/config.c
index 6d9a4c48..7772f3ec 100644
--- a/client/src/config.c
+++ b/client/src/config.c
@@ -473,6 +473,13 @@ static struct Option options[] =
     .type           = OPTION_TYPE_INT,
     .value.x_int    = 2048
   },
+  {
+    .module         = "audio",
+    .name           = "bufferLatency",
+    .description    = "Additional buffer latency in milliseconds",
+    .type           = OPTION_TYPE_INT,
+    .value.x_int    = 8
+  },
   {0}
 };
 
@@ -646,6 +653,7 @@ bool config_load(int argc, char * argv[])
   }
 
   g_params.audioPeriodSize = option_get_int("audio", "periodSize");
+  g_params.audioBufferLatency = option_get_int("audio", "bufferLatency");
 
   return true;
 }
diff --git a/client/src/main.h b/client/src/main.h
index fe48639d..2bc8f219 100644
--- a/client/src/main.h
+++ b/client/src/main.h
@@ -201,6 +201,7 @@ struct AppParams
   bool              showCursorDot;
 
   int               audioPeriodSize;
+  int               audioBufferLatency;
 };
 
 struct CBRequest