From f09ba5ffd53bf7d28e0dc516c00a8f69ca7efae9 Mon Sep 17 00:00:00 2001 From: jacqueline Date: Thu, 28 Sep 2023 08:29:55 +1000 Subject: Use bindey for databinding instead of hand rolling ui updates --- src/database/database.cpp | 104 +++++++++++++++++++----------------- src/database/include/database.hpp | 24 +++++---- src/database/include/records.hpp | 2 +- src/database/include/tag_parser.hpp | 18 +++---- src/database/include/track.hpp | 43 ++++++++------- src/database/records.cpp | 4 +- src/database/tag_parser.cpp | 50 ++++++++--------- src/database/track.cpp | 6 --- 8 files changed, 129 insertions(+), 122 deletions(-) (limited to 'src/database') diff --git a/src/database/database.cpp b/src/database/database.cpp index fd0e50c1..1ecd72e0 100644 --- a/src/database/database.cpp +++ b/src/database/database.cpp @@ -144,7 +144,7 @@ auto Database::Update() -> std::future { OwningSlice prefix = EncodeDataPrefix(); it->Seek(prefix.slice); while (it->Valid() && it->key().starts_with(prefix.slice)) { - std::optional track = ParseDataValue(it->value()); + std::shared_ptr track = ParseDataValue(it->value()); if (!track) { // The value was malformed. Drop this record. ESP_LOGW(kTag, "dropping malformed metadata"); @@ -159,9 +159,9 @@ auto Database::Update() -> std::future { continue; } - TrackTags tags{}; - if (!tag_parser_.ReadAndParseTags(track->filepath(), &tags) || - tags.encoding() == Container::kUnsupported) { + std::shared_ptr tags = + tag_parser_.ReadAndParseTags(track->filepath()); + if (!tags || tags->encoding() == Container::kUnsupported) { // We couldn't read the tags for this track. Either they were // malformed, or perhaps the file is missing. Either way, tombstone // this record. @@ -174,7 +174,7 @@ auto Database::Update() -> std::future { // At this point, we know that the track still exists in its original // location. All that's left to do is update any metadata about it. - uint64_t new_hash = tags.Hash(); + uint64_t new_hash = tags->Hash(); if (new_hash != track->tags_hash()) { // This track's tags have changed. Since the filepath is exactly the // same, we assume this is a legitimate correction. Update the @@ -185,7 +185,9 @@ auto Database::Update() -> std::future { dbPutHash(new_hash, track->id()); } - dbCreateIndexesForTrack({*track, tags}); + Track t{track, tags}; + + dbCreateIndexesForTrack(t); it->Next(); } @@ -197,15 +199,14 @@ auto Database::Update() -> std::future { .stage = event::UpdateProgress::Stage::kScanningForNewTracks, }); file_gatherer_.FindFiles("", [&](const std::pmr::string& path) { - TrackTags tags; - if (!tag_parser_.ReadAndParseTags(path, &tags) || - tags.encoding() == Container::kUnsupported) { + std::shared_ptr tags = tag_parser_.ReadAndParseTags(path); + if (!tags || tags->encoding() == Container::kUnsupported) { // No parseable tags; skip this fiile. return; } // Check for any existing record with the same hash. - uint64_t hash = tags.Hash(); + uint64_t hash = tags->Hash(); OwningSlice key = EncodeHashKey(hash); std::optional existing_hash; std::string raw_entry; @@ -219,33 +220,36 @@ auto Database::Update() -> std::future { TrackId id = dbMintNewTrackId(); ESP_LOGI(kTag, "recording new 0x%lx", id); - TrackData data(id, path, hash); - dbPutTrackData(data); + auto data = std::make_shared(id, path, hash); + dbPutTrackData(*data); dbPutHash(hash, id); - dbCreateIndexesForTrack({data, tags}); + auto t = std::make_shared(data, tags); + dbCreateIndexesForTrack(*t); return; } - std::optional existing_data = dbGetTrackData(*existing_hash); + std::shared_ptr existing_data = dbGetTrackData(*existing_hash); if (!existing_data) { // We found a hash that matches, but there's no data record? Weird. - TrackData new_data(*existing_hash, path, hash); - dbPutTrackData(new_data); - dbCreateIndexesForTrack({*existing_data, tags}); + auto new_data = std::make_shared(*existing_hash, path, hash); + dbPutTrackData(*new_data); + auto t = std::make_shared(new_data, tags); + dbCreateIndexesForTrack(*t); return; } if (existing_data->is_tombstoned()) { ESP_LOGI(kTag, "exhuming track %lu", existing_data->id()); dbPutTrackData(existing_data->Exhume(path)); - dbCreateIndexesForTrack({*existing_data, tags}); + auto t = std::make_shared(existing_data, tags); + dbCreateIndexesForTrack(*t); } else if (existing_data->filepath() != path) { ESP_LOGW(kTag, "tag hash collision for %s and %s", existing_data->filepath().c_str(), path.c_str()); ESP_LOGI(kTag, "hash components: %s, %s, %s", - tags.at(Tag::kTitle).value_or("no title").c_str(), - tags.at(Tag::kArtist).value_or("no artist").c_str(), - tags.at(Tag::kAlbum).value_or("no album").c_str()); + tags->at(Tag::kTitle).value_or("no title").c_str(), + tags->at(Tag::kArtist).value_or("no artist").c_str(), + tags->at(Tag::kAlbum).value_or("no album").c_str()); } }); events::Ui().Dispatch(event::UpdateFinished{}); @@ -264,26 +268,27 @@ auto Database::GetTrackPath(TrackId id) }); } -auto Database::GetTrack(TrackId id) -> std::future> { - return worker_task_->Dispatch>( - [=, this]() -> std::optional { - std::optional data = dbGetTrackData(id); +auto Database::GetTrack(TrackId id) -> std::future> { + return worker_task_->Dispatch>( + [=, this]() -> std::shared_ptr { + std::shared_ptr data = dbGetTrackData(id); if (!data || data->is_tombstoned()) { return {}; } - TrackTags tags; - if (!tag_parser_.ReadAndParseTags(data->filepath(), &tags)) { + std::shared_ptr tags = + tag_parser_.ReadAndParseTags(data->filepath()); + if (!tags) { return {}; } - return Track(*data, tags); + return std::make_shared(data, tags); }); } auto Database::GetBulkTracks(std::vector ids) - -> std::future>> { - return worker_task_->Dispatch>>( - [=, this]() -> std::vector> { - std::map id_to_track{}; + -> std::future>> { + return worker_task_->Dispatch>>( + [=, this]() -> std::vector> { + std::map> id_to_track{}; // Sort the list of ids so that we can retrieve them all in a single // iteration through the database, without re-seeking. @@ -299,16 +304,16 @@ auto Database::GetBulkTracks(std::vector ids) // This id wasn't found at all. Skip it. continue; } - std::optional track = + std::shared_ptr track = ParseRecord(it->key(), it->value()); if (track) { - id_to_track.insert({id, *track}); + id_to_track.insert({id, track}); } } // We've fetched all of the ids in the request, so now just put them // back into the order they were asked for in. - std::vector> results; + std::vector> results; for (const TrackId& id : ids) { if (id_to_track.contains(id)) { results.push_back(id_to_track.at(id)); @@ -426,7 +431,7 @@ auto Database::dbPutTrackData(const TrackData& s) -> void { } } -auto Database::dbGetTrackData(TrackId id) -> std::optional { +auto Database::dbGetTrackData(TrackId id) -> std::shared_ptr { OwningSlice key = EncodeDataKey(id); std::string raw_val; if (!db_->Get(leveldb::ReadOptions(), key.slice, &raw_val).ok()) { @@ -454,7 +459,7 @@ auto Database::dbGetHash(const uint64_t& hash) -> std::optional { return ParseHashValue(raw_val); } -auto Database::dbCreateIndexesForTrack(Track track) -> void { +auto Database::dbCreateIndexesForTrack(const Track& track) -> void { for (const IndexInfo& index : GetIndexes()) { leveldb::WriteBatch writes; if (Index(index, track, &writes)) { @@ -481,7 +486,7 @@ auto Database::dbGetPage(const Continuation& c) -> Result* { // Grab results. std::optional first_key; - std::vector records; + std::vector> records; while (records.size() < c.page_size && it->Valid()) { if (!it->key().starts_with({c.prefix.data(), c.prefix.size()})) { break; @@ -489,9 +494,9 @@ auto Database::dbGetPage(const Continuation& c) -> Result* { if (!first_key) { first_key = it->key().ToString(); } - std::optional parsed = ParseRecord(it->key(), it->value()); + std::shared_ptr parsed = ParseRecord(it->key(), it->value()); if (parsed) { - records.push_back(*parsed); + records.push_back(parsed); } if (c.forward) { it->Next(); @@ -577,7 +582,7 @@ template auto Database::dbGetPage( template <> auto Database::ParseRecord(const leveldb::Slice& key, const leveldb::Slice& val) - -> std::optional { + -> std::shared_ptr { std::optional data = ParseIndexKey(key); if (!data) { return {}; @@ -588,28 +593,29 @@ auto Database::ParseRecord(const leveldb::Slice& key, title = val.ToString(); } - return IndexRecord(*data, title, data->track); + return std::make_shared(*data, title, data->track); } template <> auto Database::ParseRecord(const leveldb::Slice& key, const leveldb::Slice& val) - -> std::optional { - std::optional data = ParseDataValue(val); + -> std::shared_ptr { + std::shared_ptr data = ParseDataValue(val); if (!data || data->is_tombstoned()) { return {}; } - TrackTags tags; - if (!tag_parser_.ReadAndParseTags(data->filepath(), &tags)) { + std::shared_ptr tags = + tag_parser_.ReadAndParseTags(data->filepath()); + if (!tags) { return {}; } - return Track(*data, tags); + return std::make_shared(data, tags); } template <> auto Database::ParseRecord(const leveldb::Slice& key, const leveldb::Slice& val) - -> std::optional { + -> std::shared_ptr { std::ostringstream stream; stream << "key: "; if (key.size() < 3 || key.data()[1] != '\0') { @@ -634,7 +640,7 @@ auto Database::ParseRecord(const leveldb::Slice& key, } } std::pmr::string res{stream.str(), &memory::kSpiRamResource}; - return res; + return std::make_shared(res); } IndexRecord::IndexRecord(const IndexKey& key, diff --git a/src/database/include/database.hpp b/src/database/include/database.hpp index 98540f41..6ad8d318 100644 --- a/src/database/include/database.hpp +++ b/src/database/include/database.hpp @@ -48,12 +48,14 @@ struct Continuation { template class Result { public: - auto values() const -> const std::vector& { return values_; } + auto values() const -> const std::vector>& { + return values_; + } auto next_page() -> std::optional>& { return next_page_; } auto prev_page() -> std::optional>& { return prev_page_; } - Result(const std::vector&& values, + Result(const std::vector>&& values, std::optional> next, std::optional> prev) : values_(values), next_page_(next), prev_page_(prev) {} @@ -62,7 +64,7 @@ class Result { Result& operator=(const Result&) = delete; private: - std::vector values_; + std::vector> values_; std::optional> next_page_; std::optional> prev_page_; }; @@ -102,14 +104,14 @@ class Database { auto GetTrackPath(TrackId id) -> std::future>; - auto GetTrack(TrackId id) -> std::future>; + auto GetTrack(TrackId id) -> std::future>; /* * Fetches data for multiple tracks more efficiently than multiple calls to * GetTrack. */ auto GetBulkTracks(std::vector id) - -> std::future>>; + -> std::future>>; auto GetIndexes() -> std::vector; auto GetTracksByIndex(const IndexInfo& index, std::size_t page_size) @@ -145,30 +147,30 @@ class Database { auto dbEntomb(TrackId track, uint64_t hash) -> void; auto dbPutTrackData(const TrackData& s) -> void; - auto dbGetTrackData(TrackId id) -> std::optional; + auto dbGetTrackData(TrackId id) -> std::shared_ptr; auto dbPutHash(const uint64_t& hash, TrackId i) -> void; auto dbGetHash(const uint64_t& hash) -> std::optional; - auto dbCreateIndexesForTrack(Track track) -> void; + auto dbCreateIndexesForTrack(const Track& track) -> void; template auto dbGetPage(const Continuation& c) -> Result*; template auto ParseRecord(const leveldb::Slice& key, const leveldb::Slice& val) - -> std::optional; + -> std::shared_ptr; }; template <> auto Database::ParseRecord(const leveldb::Slice& key, const leveldb::Slice& val) - -> std::optional; + -> std::shared_ptr; template <> auto Database::ParseRecord(const leveldb::Slice& key, const leveldb::Slice& val) - -> std::optional; + -> std::shared_ptr; template <> auto Database::ParseRecord(const leveldb::Slice& key, const leveldb::Slice& val) - -> std::optional; + -> std::shared_ptr; } // namespace database diff --git a/src/database/include/records.hpp b/src/database/include/records.hpp index b144dece..e7d7738c 100644 --- a/src/database/include/records.hpp +++ b/src/database/include/records.hpp @@ -53,7 +53,7 @@ auto EncodeDataValue(const TrackData& track) -> OwningSlice; * Parses bytes previously encoded via EncodeDataValue back into a TrackData. * May return nullopt if parsing fails. */ -auto ParseDataValue(const leveldb::Slice& slice) -> std::optional; +auto ParseDataValue(const leveldb::Slice& slice) -> std::shared_ptr; /* Encodes a hash key for the specified hash. */ auto EncodeHashKey(const uint64_t& hash) -> OwningSlice; diff --git a/src/database/include/tag_parser.hpp b/src/database/include/tag_parser.hpp index d77967d8..04817c59 100644 --- a/src/database/include/tag_parser.hpp +++ b/src/database/include/tag_parser.hpp @@ -16,21 +16,21 @@ namespace database { class ITagParser { public: virtual ~ITagParser() {} - virtual auto ReadAndParseTags(const std::pmr::string& path, TrackTags* out) - -> bool = 0; + virtual auto ReadAndParseTags(const std::pmr::string& path) + -> std::shared_ptr = 0; }; class GenericTagParser : public ITagParser { public: - auto ReadAndParseTags(const std::pmr::string& path, TrackTags* out) - -> bool override; + auto ReadAndParseTags(const std::pmr::string& path) + -> std::shared_ptr override; }; class TagParserImpl : public ITagParser { public: TagParserImpl(); - auto ReadAndParseTags(const std::pmr::string& path, TrackTags* out) - -> bool override; + auto ReadAndParseTags(const std::pmr::string& path) + -> std::shared_ptr override; private: std::map> extension_to_parser_; @@ -41,7 +41,7 @@ class TagParserImpl : public ITagParser { * cache should be slightly larger than any page sizes in the UI. */ std::mutex cache_mutex_; - util::LruCache<16, std::pmr::string, TrackTags> cache_; + util::LruCache<16, std::pmr::string, std::shared_ptr> cache_; // We could also consider keeping caches of artist name -> std::pmr::string // and similar. This hasn't been done yet, as this isn't a common workload in @@ -50,8 +50,8 @@ class TagParserImpl : public ITagParser { class OpusTagParser : public ITagParser { public: - auto ReadAndParseTags(const std::pmr::string& path, TrackTags* out) - -> bool override; + auto ReadAndParseTags(const std::pmr::string& path) + -> std::shared_ptr override; }; } // namespace database diff --git a/src/database/include/track.hpp b/src/database/include/track.hpp index 1c11ddea..3c7b20fa 100644 --- a/src/database/include/track.hpp +++ b/src/database/include/track.hpp @@ -61,12 +61,17 @@ enum class Tag { */ class TrackTags { public: - auto encoding() const -> Container { return encoding_; }; - auto encoding(Container e) -> void { encoding_ = e; }; - TrackTags() : encoding_(Container::kUnsupported), tags_(&memory::kSpiRamResource) {} + TrackTags(const TrackTags& other) = delete; + TrackTags& operator=(TrackTags& other) = delete; + + bool operator==(const TrackTags&) const = default; + + auto encoding() const -> Container { return encoding_; }; + auto encoding(Container e) -> void { encoding_ = e; }; + std::optional channels; std::optional sample_rate; std::optional bits_per_sample; @@ -85,10 +90,6 @@ class TrackTags { */ auto Hash() const -> uint64_t; - bool operator==(const TrackTags&) const = default; - TrackTags& operator=(const TrackTags&) = default; - TrackTags(const TrackTags&) = default; - private: Container encoding_; std::pmr::unordered_map tags_; @@ -139,6 +140,11 @@ class TrackData { play_count_(play_count), is_tombstoned_(is_tombstoned) {} + TrackData(TrackData&& other) = delete; + TrackData& operator=(TrackData& other) = delete; + + bool operator==(const TrackData&) const = default; + auto id() const -> TrackId { return id_; } auto filepath() const -> std::pmr::string { return filepath_; } auto play_count() const -> uint32_t { return play_count_; } @@ -158,8 +164,6 @@ class TrackData { * new location. */ auto Exhume(const std::pmr::string& new_path) const -> TrackData; - - bool operator==(const TrackData&) const = default; }; /* @@ -172,23 +176,22 @@ class TrackData { */ class Track { public: - Track(const TrackData& data, const TrackTags& tags) + Track(std::shared_ptr& data, std::shared_ptr tags) : data_(data), tags_(tags) {} - Track(const Track& other) = default; - auto data() const -> const TrackData& { return data_; } - auto tags() const -> const TrackTags& { return tags_; } - - auto TitleOrFilename() const -> std::pmr::string; + Track(Track& other) = delete; + Track& operator=(Track& other) = delete; bool operator==(const Track&) const = default; - Track operator=(const Track& other) const { return Track(other); } + + auto data() const -> const TrackData& { return *data_; } + auto tags() const -> const TrackTags& { return *tags_; } + + auto TitleOrFilename() const -> std::pmr::string; private: - const TrackData data_; - const TrackTags tags_; + std::shared_ptr data_; + std::shared_ptr tags_; }; -void swap(Track& first, Track& second); - } // namespace database diff --git a/src/database/records.cpp b/src/database/records.cpp index f493500c..103b3547 100644 --- a/src/database/records.cpp +++ b/src/database/records.cpp @@ -149,7 +149,7 @@ auto EncodeDataValue(const TrackData& track) -> OwningSlice { return OwningSlice(as_str); } -auto ParseDataValue(const leveldb::Slice& slice) -> std::optional { +auto ParseDataValue(const leveldb::Slice& slice) -> std::shared_ptr { CborParser parser; CborValue container; CborError err; @@ -211,7 +211,7 @@ auto ParseDataValue(const leveldb::Slice& slice) -> std::optional { return {}; } - return TrackData(id, path, hash, play_count, is_tombstoned); + return std::make_shared(id, path, hash, play_count, is_tombstoned); } /* 'H/ 0xBEEF' */ diff --git a/src/database/tag_parser.cpp b/src/database/tag_parser.cpp index 8912690b..fe71089d 100644 --- a/src/database/tag_parser.cpp +++ b/src/database/tag_parser.cpp @@ -130,14 +130,13 @@ TagParserImpl::TagParserImpl() { extension_to_parser_["opus"] = std::make_unique(); } -auto TagParserImpl::ReadAndParseTags(const std::pmr::string& path, - TrackTags* out) -> bool { +auto TagParserImpl::ReadAndParseTags(const std::pmr::string& path) + -> std::shared_ptr { { std::lock_guard lock{cache_mutex_}; - std::optional cached = cache_.Get(path); + std::optional> cached = cache_.Get(path); if (cached) { - *out = *cached; - return true; + return *cached; } } @@ -152,41 +151,43 @@ auto TagParserImpl::ReadAndParseTags(const std::pmr::string& path, } } - if (!parser->ReadAndParseTags(path, out)) { - return false; + std::shared_ptr tags = parser->ReadAndParseTags(path); + if (!tags) { + return {}; } // There wasn't a track number found in the track's tags. Try to synthesize // one from the filename, which will sometimes have a track number at the // start. - if (!out->at(Tag::kAlbumTrack)) { + if (!tags->at(Tag::kAlbumTrack)) { auto slash_pos = path.find_last_of("/"); if (slash_pos != std::pmr::string::npos && path.size() - slash_pos > 1) { - out->set(Tag::kAlbumTrack, path.substr(slash_pos + 1)); + tags->set(Tag::kAlbumTrack, path.substr(slash_pos + 1)); } } // Normalise track numbers; they're usually treated as strings, but we would // like to sort them lexicographically. - out->set(Tag::kAlbumTrack, - convert_track_number(out->at(Tag::kAlbumTrack).value_or("0"))); + tags->set(Tag::kAlbumTrack, + convert_track_number(tags->at(Tag::kAlbumTrack).value_or("0"))); { std::lock_guard lock{cache_mutex_}; - cache_.Put(path, *out); + cache_.Put(path, tags); } - return true; + return tags; } -auto GenericTagParser::ReadAndParseTags(const std::pmr::string& path, - TrackTags* out) -> bool { +auto GenericTagParser::ReadAndParseTags(const std::pmr::string& path) + -> std::shared_ptr { libtags::Aux aux; - aux.tags = out; + auto out = std::make_shared(); + aux.tags = out.get(); if (f_stat(path.c_str(), &aux.info) != FR_OK || f_open(&aux.file, path.c_str(), FA_READ) != FR_OK) { ESP_LOGW(kTag, "failed to open file %s", path.c_str()); - return false; + return {}; } // Fine to have this on the stack; this is only called on tasks with large // stacks anyway, due to all the string handling. @@ -205,7 +206,7 @@ auto GenericTagParser::ReadAndParseTags(const std::pmr::string& path, if (res != 0) { // Parsing failed. ESP_LOGE(kTag, "tag parsing for %s failed, reason %d", path.c_str(), res); - return false; + return {}; } switch (ctx.format) { @@ -240,25 +241,26 @@ auto GenericTagParser::ReadAndParseTags(const std::pmr::string& path, if (ctx.duration > 0) { out->duration = ctx.duration; } - return true; + return out; } -auto OpusTagParser::ReadAndParseTags(const std::pmr::string& path, - TrackTags* out) -> bool { +auto OpusTagParser::ReadAndParseTags(const std::pmr::string& path) + -> std::shared_ptr { std::pmr::string vfs_path = "/sdcard" + path; int err; OggOpusFile* f = op_test_file(vfs_path.c_str(), &err); if (f == NULL) { ESP_LOGE(kTag, "opusfile tag parsing failed: %d", err); - return false; + return {}; } const OpusTags* tags = op_tags(f, -1); if (tags == NULL) { ESP_LOGE(kTag, "no tags in opusfile"); op_free(f); - return false; + return {}; } + auto out = std::make_shared(); out->encoding(Container::kOpus); for (const auto& pair : kVorbisIdToTag) { const char* tag = opus_tags_query(tags, pair.first, 0); @@ -268,7 +270,7 @@ auto OpusTagParser::ReadAndParseTags(const std::pmr::string& path, } op_free(f); - return true; + return out; } } // namespace database diff --git a/src/database/track.cpp b/src/database/track.cpp index a3c7dc99..f48bb8ed 100644 --- a/src/database/track.cpp +++ b/src/database/track.cpp @@ -64,12 +64,6 @@ auto TrackData::Exhume(const std::pmr::string& new_path) const -> TrackData { return TrackData(id_, new_path, tags_hash_, play_count_, false); } -void swap(Track& first, Track& second) { - Track temp = first; - first = second; - second = temp; -} - auto Track::TitleOrFilename() const -> std::pmr::string { auto title = tags().at(Tag::kTitle); if (title) { -- cgit v1.2.3