From 8e38aedd16ebd2e57cdd5f467b577d8bd011d84e Mon Sep 17 00:00:00 2001 From: casey langen Date: Sat, 8 Dec 2018 23:11:50 -0800 Subject: [PATCH] Added ITagStore::ContainsThumbnail(), and added optimizations around not re-processing thumbnails at the track level if artwork already exists at the album-level. --- src/core/library/Indexer.cpp | 4 +- src/core/library/track/IndexerTrack.cpp | 122 ++++++++++++------ src/core/library/track/IndexerTrack.h | 12 +- src/core/library/track/LibraryTrack.cpp | 9 ++ src/core/library/track/LibraryTrack.h | 1 + src/core/library/track/Track.cpp | 5 + src/core/library/track/Track.h | 2 + src/core/sdk/ITagStore.h | 1 + src/core/sdk/constants.h | 2 +- .../taglib_plugin/TaglibMetadataReader.cpp | 26 ++-- 10 files changed, 130 insertions(+), 54 deletions(-) diff --git a/src/core/library/Indexer.cpp b/src/core/library/Indexer.cpp index 01be6764b..06ed6f3db 100644 --- a/src/core/library/Indexer.cpp +++ b/src/core/library/Indexer.cpp @@ -220,6 +220,8 @@ void Indexer::RemovePath(const std::string& path) { } void Indexer::Synchronize(const SyncContext& context, boost::asio::io_service* io) { + IndexerTrack::OnIndexerStarted(this->dbConnection); + this->ProcessAddRemoveQueue(); this->tracksScanned = 0; @@ -348,7 +350,7 @@ void Indexer::FinalizeSync(const SyncContext& context) { /* run analyzers. */ this->RunAnalyzers(); - IndexerTrack::ResetIdCache(); + IndexerTrack::OnIndexerFinished(this->dbConnection); } void Indexer::ReadMetadataFromFile( diff --git a/src/core/library/track/IndexerTrack.cpp b/src/core/library/track/IndexerTrack.cpp index cc4aa6ad9..bffa83398 100644 --- a/src/core/library/track/IndexerTrack.cpp +++ b/src/core/library/track/IndexerTrack.cpp @@ -60,14 +60,44 @@ using namespace musik::core::sdk; std::mutex IndexerTrack::sharedWriteMutex; static std::unordered_map metadataIdCache; +static std::unordered_map thumbnailIdCache; /* albumId:thumbnailId */ -void IndexerTrack::ResetIdCache() { - metadataIdCache.clear(); +/* http://stackoverflow.com/a/2351171 */ +static size_t hash32(const char* str) { + unsigned int h; + unsigned char *p; + h = 0; + for (p = (unsigned char*)str; *p != '\0'; p++) { + h = 37 * h + *p; + } + h += (h >> 5); + return h; } -IndexerTrack::IndexerTrack(int64_t id) +void IndexerTrack::OnIndexerStarted(db::Connection &dbConnection) { + /* unused, for now */ +} + +void IndexerTrack::OnIndexerFinished(db::Connection &dbConnection) { + metadataIdCache.clear(); + + /* if we got some new album art, make sure all of the tracks for the + album get the updated ID! */ + std::string query = "UPDATE tracks SET thumbnail_id=? WHERE album_id=?)"; + db::ScopedTransaction transaction(dbConnection); + for (auto it : thumbnailIdCache) { + db::Statement stmt(query.c_str(), dbConnection); + stmt.BindInt64(0, it.second); + stmt.BindInt64(1, it.first); + stmt.Step(); + } + + thumbnailIdCache.clear(); +} + +IndexerTrack::IndexerTrack(int64_t trackId) : internalMetadata(new IndexerTrack::InternalMetadata()) -, id(id) +, trackId(trackId) { } @@ -150,6 +180,26 @@ void IndexerTrack::SetThumbnail(const char *data, long size) { memcpy(this->internalMetadata->thumbnailData, data, size); } +int64_t IndexerTrack::GetThumbnailId() { + std::string key = this->GetString("album") + "-" + this->GetString("album_artist"); + size_t id = hash32(key.c_str()); + auto it = thumbnailIdCache.find(id); + if (it != thumbnailIdCache.end()) { + return it->second; + } + return 0; +} + +bool IndexerTrack::ContainsThumbnail() { + if (this->internalMetadata->thumbnailData && + this->internalMetadata->thumbnailSize) + { + return true; + } + std::unique_lock lock(sharedWriteMutex); + return this->GetThumbnailId() != 0; +} + void IndexerTrack::SetReplayGain(const ReplayGain& replayGain) { this->internalMetadata->replayGain.reset(); this->internalMetadata->replayGain = std::make_shared(); @@ -187,7 +237,7 @@ Track::MetadataIteratorRange IndexerTrack::GetAllValues() { } int64_t IndexerTrack::GetId() { - return this->id; + return this->trackId; } bool IndexerTrack::NeedsToBeIndexed( @@ -219,7 +269,7 @@ bool IndexerTrack::NeedsToBeIndexed( bool fileDifferent = true; if (stmt.Step() == db::Row) { - this->id = stmt.ColumnInt64(0); + this->trackId = stmt.ColumnInt64(0); int dbFileSize = stmt.ColumnInt32(2); int dbFileTime = stmt.ColumnInt32(3); @@ -353,7 +403,7 @@ void IndexerTrack::SaveReplayGain(db::Connection& dbConnection) if (replayGain) { { db::Statement removeOld("DELETE FROM replay_gain WHERE track_id=?", dbConnection); - removeOld.BindInt64(0, this->id); + removeOld.BindInt64(0, this->trackId); removeOld.Step(); } @@ -367,7 +417,7 @@ void IndexerTrack::SaveReplayGain(db::Connection& dbConnection) "VALUES (?, ?, ?, ?, ?);", dbConnection); - insert.BindInt64(0, this->id); + insert.BindInt64(0, this->trackId); insert.BindFloat(1, replayGain->albumGain); insert.BindFloat(2, replayGain->albumPeak); insert.BindFloat(3, replayGain->trackGain); @@ -526,7 +576,7 @@ void IndexerTrack::ProcessNonStandardMetadata(db::Connection& connection) { if (process) { insertTrackMeta.Reset(); - insertTrackMeta.BindInt64(0, this->id); + insertTrackMeta.BindInt64(0, this->trackId); insertTrackMeta.BindInt64(1, valueId); insertTrackMeta.Step(); } @@ -534,18 +584,6 @@ void IndexerTrack::ProcessNonStandardMetadata(db::Connection& connection) { } } -/* http://stackoverflow.com/a/2351171 */ -static size_t hash32(const char* str) { - unsigned int h; - unsigned char *p; - h = 0; - for (p = (unsigned char*)str; *p != '\0'; p++) { - h = 37 * h + *p; - } - h += (h >> 5); - return h; -} - static std::string createTrackExternalId(IndexerTrack& track) { size_t hash1 = (size_t) hash32(track.GetString("filename").c_str()); @@ -565,8 +603,10 @@ int64_t IndexerTrack::SaveAlbum(db::Connection& dbConnection, int64_t thumbnailI std::string value = album + "-" + this->GetString("album_artist"); /* ideally we'd use std::hash<>, but on some platforms this returns a 64-bit - unsigned number, which cannot be easily used with sqlite3. */ - size_t id = hash32(value.c_str()); + unsigned number, which cannot be easily used with sqlite3. TODO: this seems + really strange, why don't we just cast to a signed int and be done with it? + something to do with negative values? i can't remember now. */ + size_t albumId = hash32(value.c_str()); std::string cacheKey = "album-" + value; if (metadataIdCache.find(cacheKey) != metadataIdCache.end()) { @@ -575,11 +615,11 @@ int64_t IndexerTrack::SaveAlbum(db::Connection& dbConnection, int64_t thumbnailI else { std::string insertStatement = "INSERT INTO albums (id, name) VALUES (?, ?)"; db::Statement insertValue(insertStatement.c_str(), dbConnection); - insertValue.BindInt64(0, id); + insertValue.BindInt64(0, albumId); insertValue.BindText(1, album); if (insertValue.Step() == db::Done) { - metadataIdCache[cacheKey] = id; + metadataIdCache[cacheKey] = albumId; } } @@ -588,11 +628,13 @@ int64_t IndexerTrack::SaveAlbum(db::Connection& dbConnection, int64_t thumbnailI "UPDATE albums SET thumbnail_id=? WHERE id=?", dbConnection); updateStatement.BindInt64(0, thumbnailId); - updateStatement.BindInt64(1, id); + updateStatement.BindInt64(1, albumId); updateStatement.Step(); + + thumbnailIdCache[albumId] = thumbnailId; } - return id; + return albumId; } int64_t IndexerTrack::SaveSingleValueField( @@ -726,7 +768,7 @@ void IndexerTrack::SaveDirectory(db::Connection& db, const std::string& filename if (dirId != -1) { db::Statement update("UPDATE tracks SET directory_id=? WHERE id=?", db); update.BindInt64(0, dirId); - update.BindInt64(1, this->id); + update.BindInt64(1, this->trackId); update.Step(); } } @@ -751,21 +793,27 @@ bool IndexerTrack::Save(db::Connection &dbConnection, std::string libraryDirecto /* remove existing relations -- we're going to update them with fresh data */ - if (this->id != 0) { - removeRelation(dbConnection, "track_genres", this->id); - removeRelation(dbConnection, "track_artists", this->id); - removeRelation(dbConnection, "track_meta", this->id); + if (this->trackId != 0) { + removeRelation(dbConnection, "track_genres", this->trackId); + removeRelation(dbConnection, "track_artists", this->trackId); + removeRelation(dbConnection, "track_meta", this->trackId); } /* write generic info to the tracks table */ - this->id = writeToTracksTable(dbConnection, *this); + this->trackId = writeToTracksTable(dbConnection, *this); - if (!this->id) { + if (!this->trackId) { return false; } + /* see if the metadata reader plugin extracted a thumbnail. if not, we call + this->GetThumbnailId() to see if one already exists for the album */ int64_t thumbnailId = this->SaveThumbnail(dbConnection, libraryDirectory); + if (thumbnailId == 0) { + thumbnailId = this->GetThumbnailId(); + } + int64_t albumId = this->SaveAlbum(dbConnection, thumbnailId); int64_t genreId = this->SaveGenre(dbConnection); int64_t artistId = this->SaveArtist(dbConnection); @@ -798,7 +846,7 @@ bool IndexerTrack::Save(db::Connection &dbConnection, std::string libraryDirecto stmt.BindInt64(3, albumArtistId); stmt.BindInt64(4, thumbnailId); stmt.BindInt64(5, sourceId); - stmt.BindInt64(6, this->id); + stmt.BindInt64(6, this->trackId); stmt.Step(); } @@ -861,7 +909,7 @@ int64_t IndexerTrack::SaveNormalizedFieldValue( % relationJunctionTableName % relationJunctionTableColumn); db::Statement stmt(query.c_str(), dbConnection); - stmt.BindInt64(0, this->id); + stmt.BindInt64(0, this->trackId); stmt.BindInt64(1, fieldId); stmt.Step(); } @@ -870,7 +918,7 @@ int64_t IndexerTrack::SaveNormalizedFieldValue( } TrackPtr IndexerTrack::Copy() { - return TrackPtr(new IndexerTrack(this->id)); + return TrackPtr(new IndexerTrack(this->trackId)); } IndexerTrack::InternalMetadata::InternalMetadata() diff --git a/src/core/library/track/IndexerTrack.h b/src/core/library/track/IndexerTrack.h index 5a642775f..84be137ee 100644 --- a/src/core/library/track/IndexerTrack.h +++ b/src/core/library/track/IndexerTrack.h @@ -42,7 +42,7 @@ namespace musik { namespace core { class IndexerTrack : public Track { public: - IndexerTrack(int64_t id); + IndexerTrack(int64_t trackId); virtual ~IndexerTrack(void); /* ITagStore */ @@ -50,6 +50,7 @@ namespace musik { namespace core { virtual void ClearValue(const char* metakey); virtual bool Contains(const char* metakey); virtual void SetThumbnail(const char *data, long size); + virtual bool ContainsThumbnail(); virtual void SetReplayGain(const musik::core::sdk::ReplayGain& replayGain); /* ITrack */ @@ -67,7 +68,7 @@ namespace musik { namespace core { virtual TrackPtr Copy(); virtual int64_t GetId(); - virtual void SetId(int64_t id) { this->id = id; } + virtual void SetId(int64_t trackId) { this->trackId = trackId; } bool NeedsToBeIndexed( const boost::filesystem::path &file, @@ -77,14 +78,15 @@ namespace musik { namespace core { db::Connection &dbConnection, std::string libraryDirectory); - static void ResetIdCache(); + static void OnIndexerStarted(db::Connection &dbConnection); + static void OnIndexerFinished(db::Connection &dbConnection); protected: friend class Indexer; static std::mutex sharedWriteMutex; private: - int64_t id; + int64_t trackId; private: class InternalMetadata { @@ -104,6 +106,8 @@ namespace musik { namespace core { db::Connection& connection, const std::string& libraryDirectory); + int64_t GetThumbnailId(); + int64_t SaveGenre(db::Connection& connection); int64_t SaveArtist(db::Connection& connection); diff --git a/src/core/library/track/LibraryTrack.cpp b/src/core/library/track/LibraryTrack.cpp index 964282c78..a6f0be105 100644 --- a/src/core/library/track/LibraryTrack.cpp +++ b/src/core/library/track/LibraryTrack.cpp @@ -127,6 +127,15 @@ void LibraryTrack::SetThumbnail(const char *data, long size) { IndexerTrack. */ } +bool LibraryTrack::ContainsThumbnail() { + std::unique_lock lock(this->mutex); + auto it = this->metadata.find("thumbnail_id"); + if (it != this->metadata.end()) { + return it->second.size() > 0; + } + return false; +} + void LibraryTrack::SetReplayGain(const musik::core::sdk::ReplayGain& replayGain) { /* do nothing, we don't use this value and this method should never be called. it's used by the IndexerTrack implementation, and also by the diff --git a/src/core/library/track/LibraryTrack.h b/src/core/library/track/LibraryTrack.h index 7296b164b..ebcbb0311 100644 --- a/src/core/library/track/LibraryTrack.h +++ b/src/core/library/track/LibraryTrack.h @@ -62,6 +62,7 @@ namespace musik { namespace core { virtual void ClearValue(const char* metakey); virtual bool Contains(const char* metakey); virtual void SetThumbnail(const char *data, long size); + virtual bool ContainsThumbnail(); virtual void SetReplayGain(const musik::core::sdk::ReplayGain& replayGain); /* ITrack */ diff --git a/src/core/library/track/Track.cpp b/src/core/library/track/Track.cpp index 2abd45851..daab87a8e 100644 --- a/src/core/library/track/Track.cpp +++ b/src/core/library/track/Track.cpp @@ -105,6 +105,7 @@ class SdkWrapper : public Track { virtual void ClearValue(const char* key) override { NO_IMPL } virtual void SetThumbnail(const char *data, long size) override { NO_IMPL } virtual bool Contains(const char* key) override { NO_IMPL } + virtual bool ContainsThumbnail() override { NO_IMPL } virtual void SetReplayGain(const ReplayGain& replayGain) override { NO_IMPL } virtual void SetId(int64_t id) override { NO_IMPL } virtual std::string GetString(const char* metakey) override { NO_IMPL } @@ -191,6 +192,10 @@ bool TagStore::Contains(const char* key) { return this->track->Contains(key); } +bool TagStore::ContainsThumbnail() { + return this->track->ContainsThumbnail(); +} + void TagStore::SetThumbnail(const char *data, long size) { this->track->SetThumbnail(data, size); } diff --git a/src/core/library/track/Track.h b/src/core/library/track/Track.h index 78e3f6eaa..f83f1dce0 100644 --- a/src/core/library/track/Track.h +++ b/src/core/library/track/Track.h @@ -66,6 +66,7 @@ namespace musik { namespace core { virtual void ClearValue(const char* key) = 0; virtual void SetThumbnail(const char *data, long size) = 0; virtual bool Contains(const char* key) = 0; + virtual bool ContainsThumbnail() = 0; virtual void SetReplayGain(const musik::core::sdk::ReplayGain& replayGain) = 0; /* IResource */ @@ -113,6 +114,7 @@ namespace musik { namespace core { virtual void SetValue(const char* key, const char* value) override; virtual void ClearValue(const char* key) override; virtual bool Contains(const char* key) override; + virtual bool ContainsThumbnail() override; virtual void SetThumbnail(const char *data, long size) override; virtual void SetReplayGain(const musik::core::sdk::ReplayGain& replayGain) override; diff --git a/src/core/sdk/ITagStore.h b/src/core/sdk/ITagStore.h index ebfa7dad2..3c4ba5e05 100644 --- a/src/core/sdk/ITagStore.h +++ b/src/core/sdk/ITagStore.h @@ -51,6 +51,7 @@ namespace musik { namespace core { namespace sdk { virtual void ClearValue(const char* key) = 0; virtual bool Contains(const char* key) = 0; virtual void SetThumbnail(const char *data, long size) = 0; + virtual bool ContainsThumbnail() = 0; virtual void SetReplayGain(const ReplayGain& replayGain) = 0; }; diff --git a/src/core/sdk/constants.h b/src/core/sdk/constants.h index 636348d1d..b5db73dea 100644 --- a/src/core/sdk/constants.h +++ b/src/core/sdk/constants.h @@ -147,5 +147,5 @@ namespace musik { static const char* ExternalId = "external_id"; } - static const int SdkVersion = 14; + static const int SdkVersion = 15; } } } diff --git a/src/plugins/taglib_plugin/TaglibMetadataReader.cpp b/src/plugins/taglib_plugin/TaglibMetadataReader.cpp index db53c7dea..1c71d83c1 100644 --- a/src/plugins/taglib_plugin/TaglibMetadataReader.cpp +++ b/src/plugins/taglib_plugin/TaglibMetadataReader.cpp @@ -549,21 +549,25 @@ bool TaglibMetadataReader::ReadID3V2(const char* uri, ITagStore *track) { } } - /* thumbnail */ + /* thumbnail -- should come last, otherwise ::ContainsThumbnail() may + not be reliable; the thumbnails are computed and stored at the album level + so the album and album artist names need to have already been parsed. */ - TagLib::ID3v2::FrameList pictures = allTags["APIC"]; - if(!pictures.isEmpty()) { - /* there can be multiple pictures, apparently. let's just use - the first one. */ + if (!track->ContainsThumbnail()) { + TagLib::ID3v2::FrameList pictures = allTags["APIC"]; + if (!pictures.isEmpty()) { + /* there can be multiple pictures, apparently. let's just use + the first one. */ - TagLib::ID3v2::AttachedPictureFrame *picture = - static_cast(pictures.front()); + TagLib::ID3v2::AttachedPictureFrame *picture = + static_cast(pictures.front()); - TagLib::ByteVector pictureData = picture->picture(); - long long size = pictureData.size(); + TagLib::ByteVector pictureData = picture->picture(); + long long size = pictureData.size(); - if(size > 32) { /* noticed that some id3tags have like a 4-8 byte size with no thumbnail */ - track->SetThumbnail(pictureData.data(), size); + if(size > 32) { /* noticed that some id3tags have like a 4-8 byte size with no thumbnail */ + track->SetThumbnail(pictureData.data(), size); + } } }