Ignore comments within playlist files

Includes a general cleanup+restructure of playlist.cpp, and fixing the
tests and benchmarks

2 files changed, 247 insertions, 137 deletions

diff --git a/src/tangara/audio/playlist.cpp b/src/tangara/audio/playlist.cpp
index 944ad143..1436e2d2 100644
--- a/src/tangara/audio/playlist.cpp
+++ b/src/tangara/audio/playlist.cpp
@@ -5,14 +5,16 @@
  */
 #include "playlist.hpp"
 
-#include <string.h>
+#include <string>
 
-#include "audio/playlist.hpp"
-#include "database/database.hpp"
 #include "esp_log.h"
 #include "ff.h"
 
+#include "audio/playlist.hpp"
+#include "database/database.hpp"
+
 namespace audio {
+
 [[maybe_unused]] static constexpr char kTag[] = "playlist";
 
 Playlist::Playlist(const std::string& playlistFilepath)
@@ -20,190 +22,281 @@ Playlist::Playlist(const std::string& playlistFilepath)
       mutex_(),
       total_size_(0),
       pos_(-1),
+      file_open_(false),
+      file_error_(false),
       offset_cache_(&memory::kSpiRamResource),
       sample_size_(50) {}
 
 auto Playlist::open() -> bool {
+  std::unique_lock<std::mutex> lock(mutex_);
+  if (file_open_) {
+    return true;
+  }
+
   FRESULT res =
       f_open(&file_, filepath_.c_str(), FA_READ | FA_WRITE | FA_OPEN_ALWAYS);
   if (res != FR_OK) {
     ESP_LOGE(kTag, "failed to open file! res: %i", res);
     return false;
   }
-  // Count all entries
-  consumeAndCount(-1);
-  // Grab the first one
-  skipTo(0);
-  return true;
+  file_open_ = true;
+  file_error_ = false;
+
+  // Count the playlist size and build our offset cache.
+  countItems();
+  // Advance to the first item.
+  skipToWithoutCache(0);
+
+  return !file_error_;
 }
 
 Playlist::~Playlist() {
-  f_close(&file_);
+  if (file_open_) {
+    f_close(&file_);
+  }
+}
+
+auto Playlist::filepath() const -> std::string {
+  return filepath_;
 }
 
 auto Playlist::currentPosition() const -> size_t {
-  return pos_;
+  std::unique_lock<std::mutex> lock(mutex_);
+  return pos_ < 0 ? 0 : pos_;
 }
 
 auto Playlist::size() const -> size_t {
+  std::unique_lock<std::mutex> lock(mutex_);
   return total_size_;
 }
 
-auto MutablePlaylist::append(Item i) -> void {
+auto Playlist::value() const -> std::string {
   std::unique_lock<std::mutex> lock(mutex_);
-  auto offset = f_tell(&file_);
-  bool first_entry = current_value_.empty();
-  // Seek to end and append
-  auto end = f_size(&file_);
-  auto res = f_lseek(&file_, end);
-  if (res != FR_OK) {
-    ESP_LOGE(kTag, "Seek to end of file failed? Error %d", res);
-    return;
-  }
-  // TODO: Resolve paths for track id, etc
-  std::string path;
-  if (std::holds_alternative<std::string>(i)) {
-    path = std::get<std::string>(i);
-    f_printf(&file_, "%s\n", path.c_str());
-    if (total_size_ % sample_size_ == 0) {
-      offset_cache_.push_back(end);
-    }
-    if (first_entry) {
-      current_value_ = path;
-    }
-    total_size_++;
-  }
-  // Restore position
-  res = f_lseek(&file_, offset);
-    if (res != FR_OK) {
-      ESP_LOGE(kTag, "Failed to restore file position after append?");
-      return;
+  return current_value_;
+}
+
+auto Playlist::atEnd() const -> bool {
+  std::unique_lock<std::mutex> lock(mutex_);
+  return pos_ + 1 >= total_size_;
+}
+
+auto Playlist::next() -> void {
+  std::unique_lock<std::mutex> lock(mutex_);
+  if (pos_ + 1 < total_size_ && !file_error_) {
+    advanceBy(1);
   }
-  res = f_sync(&file_);
-  if (res != FR_OK) {
-    ESP_LOGE(kTag, "Failed to sync playlist file after append");
-    return;
+}
+
+auto Playlist::prev() -> void {
+  std::unique_lock<std::mutex> lock(mutex_);
+  if (!file_error_) {
+    // Naive approach to see how that goes for now
+    skipToLocked(pos_ - 1);
   }
 }
 
 auto Playlist::skipTo(size_t position) -> void {
   std::unique_lock<std::mutex> lock(mutex_);
+  skipToLocked(position);
+}
+
+auto Playlist::skipToLocked(size_t position) -> void {
+  if (!file_open_ || file_error_) {
+    return;
+  }
+
   // Check our cache and go to nearest entry
-  pos_ = position;
   auto remainder = position % sample_size_;
   auto quotient = (position - remainder) / sample_size_;
   if (offset_cache_.size() <= quotient) {
-    // Fall back case
-    ESP_LOGW(kTag, "File offset cache failed, falling back...");
-    f_rewind(&file_);
-    advanceBy(pos_);
+    skipToWithoutCache(position);
     return;
   }
-  auto entry = offset_cache_.at(quotient);
+
   // Go to byte offset
+  auto entry = offset_cache_.at(quotient);
   auto res = f_lseek(&file_, entry);
   if (res != FR_OK) {
-    ESP_LOGW(kTag, "Error going to byte offset %llu for playlist entry index %d", entry, pos_);
+    ESP_LOGW(kTag, "error seeking %u", res);
+    file_error_ = true;
+    return;
   }
-  // Count ahead entries
-  advanceBy(remainder+1);
+
+  // Count ahead entries.
+  advanceBy(remainder + 1);
 }
 
-auto Playlist::next() -> void {
-  if (!atEnd()) {
-    pos_++;
-    skipTo(pos_);
+auto Playlist::skipToWithoutCache(size_t position) -> void {
+  if (position >= pos_) {
+    advanceBy(position - pos_);
+  } else {
+    pos_ = -1;
+    FRESULT res = f_rewind(&file_);
+    if (res != FR_OK) {
+      ESP_LOGW(kTag, "error rewinding %u", res);
+      file_error_ = true;
+      return;
+    }
+    advanceBy(position + 1);
   }
 }
 
-auto Playlist::prev() -> void {
-  // Naive approach to see how that goes for now
-  pos_--;
-  skipTo(pos_);
-}
+auto Playlist::countItems() -> void {
+  TCHAR buff[512];
 
-auto Playlist::value() const -> std::string {
-  return current_value_;
+  for (;;) {
+    auto offset = f_tell(&file_);
+    auto next_item = nextItem(buff);
+    if (!next_item) {
+      break;
+    }
+    if (total_size_ % sample_size_ == 0) {
+      offset_cache_.push_back(offset);
+    }
+    total_size_++;
+  }
+
+  f_rewind(&file_);
 }
 
-MutablePlaylist::MutablePlaylist(const std::string& playlistFilepath) : Playlist(playlistFilepath) {}
+auto Playlist::advanceBy(ssize_t amt) -> bool {
+  TCHAR buff[512];
+  std::optional<std::string_view> item;
 
-auto MutablePlaylist::clear() -> bool {
-  std::unique_lock<std::mutex> lock(mutex_);
-  auto res = f_close(&file_);
-  if (res != FR_OK) {
-    return false;
+  while (amt > 0) {
+    item = nextItem(buff);
+    if (!item) {
+      break;
+    }
+    pos_++;
+    amt--;
   }
-  res =
-      f_open(&file_, filepath_.c_str(), FA_READ | FA_WRITE | FA_CREATE_ALWAYS);
-  if (res != FR_OK) {
-    return false;
+
+  if (item) {
+    current_value_ = *item;
   }
-  total_size_ = 0;
-  current_value_.clear();
-  offset_cache_.clear();
-  pos_ = 0;
-  return true;
-}
 
-auto Playlist::atEnd() -> bool {
-  return pos_ + 1 >= total_size_;
+  return amt == 0;
 }
 
-auto Playlist::filepath() -> std::string {
-  return filepath_;
+auto Playlist::nextItem(std::span<TCHAR> buf)
+    -> std::optional<std::string_view> {
+  while (file_open_ && !file_error_ && !f_eof(&file_)) {
+    // FIXME: f_gets is quite slow (it does several very small reads instead of
+    // grabbing a whole sector at a time), and it doesn't work well for very
+    // long lines. We should do something smarter here.
+    TCHAR* str = f_gets(buf.data(), buf.size(), &file_);
+    if (str == NULL) {
+      ESP_LOGW(kTag, "Error consuming playlist file at offset %llu",
+               f_tell(&file_));
+      file_error_ = true;
+      return {};
+    }
+
+    std::string_view line{str};
+    if (line.starts_with("#")) {
+      continue;
+    }
+    if (line.ends_with('\n')) {
+      line = line.substr(0, line.size() - 1);
+    }
+    return line;
+  }
+
+  // Got to EOF without reading a valid line.
+  return {};
 }
 
-auto Playlist::consumeAndCount(ssize_t upto) -> bool {
+MutablePlaylist::MutablePlaylist(const std::string& playlistFilepath)
+    : Playlist(playlistFilepath) {}
+
+auto MutablePlaylist::clear() -> bool {
   std::unique_lock<std::mutex> lock(mutex_);
-  TCHAR buff[512];
-  size_t count = 0;
-  f_rewind(&file_);
-  while (!f_eof(&file_)) {
-    auto offset = f_tell(&file_);
-    // TODO: Correctly handle lines longer than this
-    // TODO: Also correctly handle the case where the last entry doesn't end in
-    // \n
-    auto res = f_gets(buff, 512, &file_);
-    if (res == NULL) {
-      ESP_LOGW(kTag, "Error consuming playlist file at line %d", count);
+
+  // Try to recover from any IO errors.
+  if (file_error_ && file_open_) {
+    file_error_ = false;
+    file_open_ = false;
+    f_close(&file_);
+  }
+
+  FRESULT res;
+  if (file_open_) {
+    res = f_rewind(&file_);
+    if (res != FR_OK) {
+      ESP_LOGE(kTag, "error rewinding %u", res);
+      file_error_ = true;
       return false;
     }
-    if (count % sample_size_ == 0) {
-      offset_cache_.push_back(offset);
+    res = f_truncate(&file_);
+    if (res != FR_OK) {
+      ESP_LOGE(kTag, "error truncating %u", res);
+      file_error_ = true;
+      return false;
     }
-    count++;
-
-    if (upto >= 0 && count > upto) {
-      size_t len = strlen(buff);
-      current_value_.assign(buff, len - 1);
-      break;
+  } else {
+    res = f_open(&file_, filepath_.c_str(),
+                 FA_READ | FA_WRITE | FA_CREATE_ALWAYS);
+    if (res != FR_OK) {
+      ESP_LOGE(kTag, "error opening file %u", res);
+      file_error_ = true;
+      return false;
     }
+    file_open_ = true;
   }
-  if (upto < 0) {
-    total_size_ = count;
-    f_rewind(&file_);
-  }
+
+  total_size_ = 0;
+  current_value_.clear();
+  offset_cache_.clear();
+  pos_ = -1;
   return true;
 }
 
-auto Playlist::advanceBy(ssize_t amt) -> bool {
-  TCHAR buff[512];
-  size_t count = 0;
-  while (!f_eof(&file_)) {
-    auto res = f_gets(buff, 512, &file_);
-    if (res == NULL) {
-      ESP_LOGW(kTag, "Error consuming playlist file at line %d", count);
-      return false;
+auto MutablePlaylist::append(Item i) -> void {
+  std::unique_lock<std::mutex> lock(mutex_);
+  if (!file_open_ || file_error_) {
+    return;
+  }
+
+  auto offset = f_tell(&file_);
+  bool first_entry = current_value_.empty();
+
+  // Seek to end and append
+  auto end = f_size(&file_);
+  auto res = f_lseek(&file_, end);
+  if (res != FR_OK) {
+    ESP_LOGE(kTag, "Seek to end of file failed? Error %d", res);
+    file_error_ = true;
+    return;
+  }
+
+  // TODO: Resolve paths for track id, etc
+  std::string path;
+  if (std::holds_alternative<std::string>(i)) {
+    path = std::get<std::string>(i);
+    f_printf(&file_, "%s\n", path.c_str());
+    if (total_size_ % sample_size_ == 0) {
+      offset_cache_.push_back(end);
     }
-    count++;
-    if (count >= amt) {
-      size_t len = strlen(buff);
-      current_value_.assign(buff, len - 1);
-      break;
+    if (first_entry) {
+      current_value_ = path;
     }
+    total_size_++;
+  }
+
+  // Restore position
+  res = f_lseek(&file_, offset);
+  if (res != FR_OK) {
+    ESP_LOGE(kTag, "Failed to restore file position after append?");
+    file_error_ = true;
+    return;
+  }
+  res = f_sync(&file_);
+  if (res != FR_OK) {
+    ESP_LOGE(kTag, "Failed to sync playlist file after append");
+    file_error_ = true;
+    return;
   }
-  return true;
 }
 
-}  // namespace audio
\ No newline at end of file
+}  // namespace audio
diff --git a/src/tangara/audio/playlist.hpp b/src/tangara/audio/playlist.hpp
index b248ac77..ac62c82e 100644
--- a/src/tangara/audio/playlist.hpp
+++ b/src/tangara/audio/playlist.hpp
@@ -6,11 +6,14 @@
  */
 
 #pragma once
+
 #include <string>
 #include <variant>
+
+#include "ff.h"
+
 #include "database/database.hpp"
 #include "database/track.hpp"
-#include "ff.h"
 
 namespace audio {
 
@@ -31,40 +34,54 @@ class Playlist {
   using Item =
       std::variant<database::TrackId, database::TrackIterator, std::string>;
   auto open() -> bool;
+
+  auto filepath() const -> std::string;
   auto currentPosition() const -> size_t;
   auto size() const -> size_t;
-  auto skipTo(size_t position) -> void;
+  auto value() const -> std::string;
+  auto atEnd() const -> bool;
+
   auto next() -> void;
   auto prev() -> void;
-  auto value() const -> std::string;
-  auto atEnd() -> bool;
-  auto filepath() -> std::string;
+  auto skipTo(size_t position) -> void;
 
  protected:
-  std::string filepath_;
-  std::mutex mutex_;
+  const std::string filepath_;
+
+  mutable std::mutex mutex_;
   size_t total_size_;
-  size_t pos_;
+  ssize_t pos_;
+
   FIL file_;
+  bool file_open_;
+  bool file_error_;
+
   std::string current_value_;
 
+  /* List of offsets determined by sample size */
+  std::pmr::vector<FSIZE_t> offset_cache_;
 
-  std::pmr::vector<FSIZE_t> offset_cache_; // List of offsets determined by sample size;
   /*
-  * How many tracks per offset saved (ie, a value of 100 means every 100 tracks the file offset is saved)
-  * This speeds up searches, especially in the case of shuffling a lot of tracks.
-  */
-  const uint32_t sample_size_; 
+   * How many tracks per offset saved (ie, a value of 100 means every 100 tracks
+   * the file offset is saved) This speeds up searches, especially in the case
+   * of shuffling a lot of tracks.
+   */
+  const uint32_t sample_size_;
 
-  auto consumeAndCount(ssize_t upto) -> bool;
+ private:
+  auto skipToLocked(size_t position) -> void;
+  auto countItems() -> void;
   auto advanceBy(ssize_t amt) -> bool;
+  auto nextItem(std::span<TCHAR>) -> std::optional<std::string_view>;
+  auto skipToWithoutCache(size_t position) -> void;
 };
 
 class MutablePlaylist : public Playlist {
-public:
+ public:
   MutablePlaylist(const std::string& playlistFilepath);
+
   auto clear() -> bool;
   auto append(Item i) -> void;
 };
 
-}  // namespace audio
\ No newline at end of file
+}  // namespace audio