From bc6ff0227bc21f7a08f20100ce0b85b510d24f7c Mon Sep 17 00:00:00 2001
From: Mynacol <Mynacol@users.noreply.github.com>
Date: Mon, 1 Jul 2024 14:39:18 +0200
Subject: [PATCH] Migrate from synchronous to async read

To improve the performance. The current implementation severely
underperforms, reaching at maximum 60 Mbit/s on a local 1 Gbit/s link
(where the cmd tool `scp` performs at over 800 Mbit/s). With video bit rates
reaching 60 Mbit/s, videos can no longer be streamed without occasional
interruptions.
This problem severely worsens if the network latency is higher. One
situation with about 40 ms RTT latency only reached about 10 Mbit/s,
even though the network could handle more throughput.
See https://github.com/xbmc/vfs.sftp/issues/22 for more discussion.

This work always initiates the next read at the end of each previous one.
This however requires to specify a length, which might not match with
the length Kodi requests next (e.g. at the end of file). To avoid
memory leaks, we have to call `sftp_async_read` with a valid buffer of
the correct size. This means a temporary buffer is needed.
If too much was actually read, this work simply rewinds the current file
offset to the Kodi-expected place.

This might also mess with other/threaded usage, as the async work can
run concurrently with normally mutexed areas, and even is expected to
change the file position during retrieval, leading to inconsistent
results when calling `GetPosition` at the same time.
---
 src/SFTPSession.cpp | 32 +++++++++++++++++++++++++++++++-
 src/SFTPSession.h   |  2 ++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/src/SFTPSession.cpp b/src/SFTPSession.cpp
index 1731aaf..f96d84e 100644
--- a/src/SFTPSession.cpp
+++ b/src/SFTPSession.cpp
@@ -91,6 +91,7 @@ CSFTPSession::CSFTPSession(const kodi::addon::VFSUrl& url)
     Disconnect();
 
   m_LastActive = std::chrono::high_resolution_clock::now();
+  read_request_identifier = 0;
 }
 
 CSFTPSession::~CSFTPSession()
@@ -296,7 +297,36 @@ int CSFTPSession::Read(sftp_file handle, void* buffer, size_t length)
 {
   std::unique_lock<std::recursive_mutex> lock(m_lock);
   m_LastActive = std::chrono::high_resolution_clock::now();
-  int result = sftp_read(handle, buffer, length);
+
+  // Start read request if no other read was already queued
+  if (!read_request_identifier) {
+    read_request_length = length;
+    read_request_identifier = sftp_async_read_begin(handle, read_request_length);
+  }
+
+  // Retrieve read from libssh while handling size differences of Kodi-requested and already sftp-requested buffers
+  size_t result = -1;
+  if (length >= read_request_length) {
+    result = sftp_async_read(handle, buffer, length, read_request_identifier);
+  } else {
+    void* temp_buf = malloc(read_request_length);
+    if (!temp_buf) {
+      kodi::Log(ADDON_LOG_ERROR, "SFTPSession::Read - Failed to allocate a temporary buffer with size %d", read_request_length);
+      return -1;
+    }
+    result = sftp_async_read(handle, temp_buf, read_request_length, read_request_identifier);
+    if (result > length) {
+      Seek(handle, GetPosition(handle) - (result - length));
+      kodi::Log(ADDON_LOG_INFO, "SFTPSession::Read - this read requested less bytes than before, requiring inefficient copying of buffers. Last read: %d, this read: %d", result, length);
+    }
+    result = std::min(result, length);
+    memcpy(buffer, temp_buf, result);
+    free(temp_buf);
+  }
+
+  // Already request the next chunk, assume the length will be the same
+  read_request_length = std::min(length, read_request_length);
+  read_request_identifier = sftp_async_read_begin(handle, read_request_length);
   return result;
 }
 
diff --git a/src/SFTPSession.h b/src/SFTPSession.h
index 1e47a80..a8e73de 100644
--- a/src/SFTPSession.h
+++ b/src/SFTPSession.h
@@ -51,6 +51,8 @@ class CSFTPSession
   bool m_connected;
   ssh_session m_session;
   sftp_session m_sftp_session;
+  int read_request_identifier;
+  size_t read_request_length;
   std::chrono::high_resolution_clock::time_point m_LastActive;
 };