From be45012443015f9c5ca4a30f94126565d70cf919 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 28 Sep 2019 15:46:26 +0200 Subject: [PATCH 01/17] Import and merge missing track metadata from file tags --- src/library/dao/trackdao.cpp | 27 -- src/sources/soundsourceproxy.cpp | 159 ++++--- src/test/trackupdate_test.cpp | 16 +- src/track/albuminfo.cpp | 12 - src/track/albuminfo.h | 3 - src/track/bpm.h | 6 + src/track/track.cpp | 156 +++--- src/track/track.h | 14 +- src/track/trackinfo.cpp | 22 - src/track/trackinfo.h | 3 - src/track/trackmetadata.cpp | 15 + src/track/trackmetadata.h | 24 +- src/track/trackmetadatataglib.cpp | 767 +++++++++++------------------- src/track/trackrecord.cpp | 81 ++++ src/track/trackrecord.h | 6 + 15 files changed, 571 insertions(+), 740 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index d7101900547..923dd3da279 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1344,33 +1344,6 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const { SoundSourceProxy(pTrack).updateTrackFromSource(); } - // Data migration: Reload track total from file tags if not initialized - // yet. The added column "tracktotal" has been initialized with the - // default value "//". - // See also: Schema revision 26 in schema.xml - if (pTrack->getTrackTotal() == "//") { - // Reload track total from file tags if the special track - // total migration value "//" indicates that the track total - // is missing and needs to be reloaded. - qDebug() << "Reloading value for 'tracktotal' once-only from file" - " to replace the default value introduced with a previous" - " schema upgrade"; - mixxx::TrackMetadata trackMetadata; - if (SoundSourceProxy(pTrack).importTrackMetadata(&trackMetadata) == mixxx::MetadataSource::ImportResult::Succeeded) { - // Copy the track total from the temporary track object - pTrack->setTrackTotal(trackMetadata.getTrackInfo().getTrackTotal()); - // Also set the track number if it is still empty due - // to insufficient parsing capabilities of Mixxx in - // previous versions. - if (!trackMetadata.getTrackInfo().getTrackNumber().isEmpty() && pTrack->getTrackNumber().isEmpty()) { - pTrack->setTrackNumber(trackMetadata.getTrackInfo().getTrackNumber()); - } - } else { - qWarning() << "Failed to reload value for 'tracktotal' from file tags:" - << trackLocation; - } - } - // Listen to dirty and changed signals connect(pTrack.get(), &Track::dirty, diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index 893996f1719..09a64e3e737 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -323,19 +323,16 @@ void SoundSourceProxy::updateTrackFromSource( // at all and this information would get lost entirely otherwise! mixxx::TrackMetadata trackMetadata; bool metadataSynchronized = false; - m_pTrack->getTrackMetadata(&trackMetadata, &metadataSynchronized); + m_pTrack->readTrackMetadata(&trackMetadata, &metadataSynchronized); // If the file tags have already been parsed at least once, the // existing track metadata should not be updated implicitly, i.e. // if the user did not explicitly choose to (re-)import metadata // explicitly from this file. + bool mergeImportedMetadata = false; if (metadataSynchronized && (importTrackMetadataMode == ImportTrackMetadataMode::Once)) { - if (kLogger.debugEnabled()) { - kLogger.debug() - << "Skip importing of track metadata and embedded cover art from file" - << getUrl().toString(); - } - return; // abort + // No (re-)import needed or desired, only merge missing properties + mergeImportedMetadata = true; } // Embedded cover art is imported together with the track's metadata. @@ -343,24 +340,21 @@ void SoundSourceProxy::updateTrackFromSource( // track! QImage coverImg; DEBUG_ASSERT(coverImg.isNull()); - QImage* pCoverImg; // pointer is also used as a flag - const CoverInfoRelative coverInfoOld = m_pTrack->getCoverInfo(); - // Only re-import cover art if it is save to update, e.g. never - // discard a users's custom choice! We are using a whitelisting - // filter here that explicitly checks all valid preconditions - // when it is permissible to update/replace existing cover art. - if (((coverInfoOld.source == CoverInfo::UNKNOWN) || (coverInfoOld.source == CoverInfo::GUESSED)) && - ((coverInfoOld.type == CoverInfo::NONE) || (coverInfoOld.type == CoverInfo::METADATA))) { - // Should import and update embedded cover art - pCoverImg = &coverImg; - } else { - if (kLogger.debugEnabled()) { - kLogger.debug() - << "Skip importing of embedded cover art from file" - << getUrl().toString(); + QImage* pCoverImg = nullptr; // pointer also serves as a flag + if (!mergeImportedMetadata) { + const CoverInfoRelative coverInfoOld = m_pTrack->getCoverInfo(); + if (coverInfoOld.source == CoverInfo::USER_SELECTED && + coverInfoOld.type == CoverInfo::FILE) { + // Ignore embedded cover art + if (kLogger.debugEnabled()) { + kLogger.debug() + << "Skip importing of embedded cover art from file" + << getUrl().toString(); + } + } else { + // (Re-)import embedded cover art + pCoverImg = &coverImg; } - // Skip import of embedded cover art - pCoverImg = nullptr; } // Parse the tags stored in the audio file @@ -373,68 +367,75 @@ void SoundSourceProxy::updateTrackFromSource( << (pCoverImg ? "and embedded cover art" : "") << "from file" << getUrl().toString(); - // Continue for now, but the abort may follow shortly if the - // track already has metadata (see below) } - if (metadataSynchronized) { - // Metadata has been synchronized successfully at least - // once in the past. Only overwrite this information if - // new data has actually been imported, otherwise abort - // and preserve the existing data! - if (metadataImported.first != mixxx::MetadataSource::ImportResult::Succeeded) { - return; // abort - } - if (kLogger.debugEnabled()) { - kLogger.debug() - << "Updating track metadata" - << (pCoverImg ? "and embedded cover art" : "") - << "from file" - << getUrl().toString(); - } - } else { - DEBUG_ASSERT(pCoverImg); - if (kLogger.debugEnabled()) { - kLogger.debug() - << "Initializing track metadata and embedded cover art from file" - << getUrl().toString(); + + if (metadataImported.first != mixxx::MetadataSource::ImportResult::Succeeded) { + if (mergeImportedMetadata) { + // Nothing to do if no metadata imported + return; + } else if (trackMetadata.getTrackInfo().getTitle().trimmed().isEmpty()) { + // Only parse artist and title if both fields are empty to avoid + // inconsistencies. Otherwise the file name (without extension) + // is used as the title and the artist is unmodified. + // + // TODO(XXX): Disable splitting of artist/title in settings, i.e. + // optionally don't split even if both title and artist are empty? + // Some users might want to import the whole file name of untagged + // files as the title without splitting the artist: + // https://www.mixxx.org/forums/viewtopic.php?f=3&t=12838 + // NOTE(uklotzde, 2019-09-26): Whoever needs this should simply set + // splitArtistTitle = false here and compile their custom version! + // It is not worth extending the settings and injecting them into + // SoundSourceProxy for just a few people. + const bool splitArtistTitle = + trackMetadata.getTrackInfo().getArtist().trimmed().isEmpty(); + const auto trackFile = m_pTrack->getFileInfo(); + kLogger.info() + << "Parsing missing" + << (splitArtistTitle ? "artist/title" : "title") + << "from file name:" + << trackFile; + if (trackMetadata.refTrackInfo().parseArtistTitleFromFileName(trackFile.fileName(), splitArtistTitle) && + metadataImported.second.isNull()) { + // Since this is also some kind of metadata import, we mark the + // track's metadata as synchronized with the time stamp of the file. + metadataImported.second = trackFile.fileLastModified(); + } } } - // Fallback: If the title field is empty then try to populate title - // (and optionally artist) from the file name. This might happen if - // tags are unavailable, unreadable, or partially/completely missing. - if (trackMetadata.getTrackInfo().getTitle().trimmed().isEmpty()) { - // Only parse artist and title if both fields are empty to avoid - // inconsistencies. Otherwise the file name (without extension) - // is used as the title and the artist is unmodified. - // - // TODO(XXX): Disable splitting of artist/title in settings, i.e. - // optionally don't split even if both title and artist are empty? - // Some users might want to import the whole file name of untagged - // files as the title without splitting the artist: - // https://www.mixxx.org/forums/viewtopic.php?f=3&t=12838 - // NOTE(uklotzde, 2019-09-26): Whoever needs this should simply set - // splitArtistTitle = false here and compile their custom version! - // It is not worth extending the settings and injecting them into - // SoundSourceProxy for just a few people. - const bool splitArtistTitle = - trackMetadata.getTrackInfo().getArtist().trimmed().isEmpty(); - const auto trackFile = m_pTrack->getFileInfo(); - kLogger.info() - << "Parsing missing" - << (splitArtistTitle ? "artist/title" : "title") - << "from file name:" - << trackFile; - if (trackMetadata.refTrackInfo().parseArtistTitleFromFileName(trackFile.fileName(), splitArtistTitle) && - metadataImported.second.isNull()) { - // Since this is also some kind of metadata import, we mark the - // track's metadata as synchronized with the time stamp of the file. - metadataImported.second = trackFile.fileLastModified(); + if (mergeImportedMetadata) { + // Partial import of properties that are not (yet) stored + // in the database + m_pTrack->mergeImportedMetadata(trackMetadata); + } else { + // Full import + if (metadataSynchronized) { + // Metadata has been synchronized successfully at least + // once in the past. Only overwrite this information if + // new data has actually been imported, otherwise abort + // and preserve the existing data! + if (metadataImported.first != mixxx::MetadataSource::ImportResult::Succeeded) { + return; // abort + } + if (kLogger.debugEnabled()) { + kLogger.debug() + << "Updating track metadata" + << (pCoverImg ? "and embedded cover art" : "") + << "from file" + << getUrl().toString(); + } + } else { + DEBUG_ASSERT(pCoverImg); + if (kLogger.debugEnabled()) { + kLogger.debug() + << "Initializing track metadata and embedded cover art from file" + << getUrl().toString(); + } } + m_pTrack->importMetadata(trackMetadata, metadataImported.second); } - m_pTrack->setTrackMetadata(trackMetadata, metadataImported.second); - if (pCoverImg) { // If the pointer is not null then the cover art should be guessed // from the embedded metadata diff --git a/src/test/trackupdate_test.cpp b/src/test/trackupdate_test.cpp index 0e26d2fbcee..cdc5e049084 100644 --- a/src/test/trackupdate_test.cpp +++ b/src/test/trackupdate_test.cpp @@ -57,14 +57,14 @@ TEST_F(TrackUpdateTest, parseModifiedCleanOnce) { pTrack->markClean(); mixxx::TrackMetadata trackMetadataBefore; - pTrack->getTrackMetadata(&trackMetadataBefore); + pTrack->readTrackMetadata(&trackMetadataBefore); auto coverInfoBefore = pTrack->getCoverInfo(); SoundSourceProxy(pTrack).updateTrackFromSource( SoundSourceProxy::ImportTrackMetadataMode::Once); mixxx::TrackMetadata trackMetadataAfter; - pTrack->getTrackMetadata(&trackMetadataAfter); + pTrack->readTrackMetadata(&trackMetadataAfter); auto coverInfoAfter = pTrack->getCoverInfo(); // Not updated @@ -79,14 +79,14 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainSkipCover) { pTrack->markClean(); mixxx::TrackMetadata trackMetadataBefore; - pTrack->getTrackMetadata(&trackMetadataBefore); + pTrack->readTrackMetadata(&trackMetadataBefore); auto coverInfoBefore = pTrack->getCoverInfo(); SoundSourceProxy(pTrack).updateTrackFromSource( SoundSourceProxy::ImportTrackMetadataMode::Again); mixxx::TrackMetadata trackMetadataAfter; - pTrack->getTrackMetadata(&trackMetadataAfter); + pTrack->readTrackMetadata(&trackMetadataAfter); auto coverInfoAfter = pTrack->getCoverInfo(); // Updated @@ -105,14 +105,14 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainUpdateCover) { pTrack->markClean(); mixxx::TrackMetadata trackMetadataBefore; - pTrack->getTrackMetadata(&trackMetadataBefore); + pTrack->readTrackMetadata(&trackMetadataBefore); auto coverInfoBefore = pTrack->getCoverInfo(); SoundSourceProxy(pTrack).updateTrackFromSource( SoundSourceProxy::ImportTrackMetadataMode::Again); mixxx::TrackMetadata trackMetadataAfter; - pTrack->getTrackMetadata(&trackMetadataAfter); + pTrack->readTrackMetadata(&trackMetadataAfter); auto coverInfoAfter = pTrack->getCoverInfo(); // Updated @@ -126,14 +126,14 @@ TEST_F(TrackUpdateTest, parseModifiedDirtyAgain) { auto pTrack = newTestTrackParsedModified(); mixxx::TrackMetadata trackMetadataBefore; - pTrack->getTrackMetadata(&trackMetadataBefore); + pTrack->readTrackMetadata(&trackMetadataBefore); auto coverInfoBefore = pTrack->getCoverInfo(); SoundSourceProxy(pTrack).updateTrackFromSource( SoundSourceProxy::ImportTrackMetadataMode::Again); mixxx::TrackMetadata trackMetadataAfter; - pTrack->getTrackMetadata(&trackMetadataAfter); + pTrack->readTrackMetadata(&trackMetadataAfter); auto coverInfoAfter = pTrack->getCoverInfo(); // Updated diff --git a/src/track/albuminfo.cpp b/src/track/albuminfo.cpp index f6551e0c267..232de95ea4e 100644 --- a/src/track/albuminfo.cpp +++ b/src/track/albuminfo.cpp @@ -3,18 +3,6 @@ namespace mixxx { -void AlbumInfo::resetUnsupportedValues() { -#if defined(__EXTRA_METADATA__) - setCopyright(QString()); - setLicense(QString()); - setMusicBrainzArtistId(QString()); - setMusicBrainzReleaseId(QString()); - setMusicBrainzReleaseGroupId(QString()); - setRecordLabel(QString()); - setReplayGain(ReplayGain()); -#endif // __EXTRA_METADATA__ -} - bool operator==(const AlbumInfo& lhs, const AlbumInfo& rhs) { return (lhs.getArtist() == rhs.getArtist()) && #if defined(__EXTRA_METADATA__) diff --git a/src/track/albuminfo.h b/src/track/albuminfo.h index 1640a7d9df1..f4b45125aa0 100644 --- a/src/track/albuminfo.h +++ b/src/track/albuminfo.h @@ -33,9 +33,6 @@ class AlbumInfo final { AlbumInfo& operator=(AlbumInfo&&) = default; AlbumInfo& operator=(const AlbumInfo&) = default; - // TODO(XXX): Remove after all new fields have been added to the library - void resetUnsupportedValues(); - // Adjusts floating-point properties to match their string representation // in file tags to account for rounding errors. void normalizeBeforeExport() { diff --git a/src/track/bpm.h b/src/track/bpm.h index 88677fa858f..ab7ffdc2abf 100644 --- a/src/track/bpm.h +++ b/src/track/bpm.h @@ -25,6 +25,12 @@ class Bpm final { // Adjusts floating-point values to match their string representation // in file tags to account for rounding errors. + // NOTE(2019-02-19, uklotzde): The pre-normalization cannot prevent + // repeated export of metadata for files with ID3 tags that are only + // able to store the BPM value with integer precision! In case of a + // fractional value the ID3 metadata is always detected as modified + // and will be exported regardless if it has actually been modified + // or not. void normalizeBeforeExport() { m_value = normalizeValue(m_value); } diff --git a/src/track/track.cpp b/src/track/track.cpp index 540dea26f4b..33fbd8c73db 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -121,13 +121,13 @@ void Track::relocate( // the updated location from the database. } -void Track::setTrackMetadata( - mixxx::TrackMetadata trackMetadata, +void Track::importMetadata( + mixxx::TrackMetadata importedMetadata, QDateTime metadataSynchronized) { // Safe some values that are needed after move assignment and unlocking(see below) - const auto newBpm = trackMetadata.getTrackInfo().getBpm(); - const auto newKey = trackMetadata.getTrackInfo().getKey(); - const auto newReplayGain = trackMetadata.getTrackInfo().getReplayGain(); + const auto newBpm = importedMetadata.getTrackInfo().getBpm(); + const auto newKey = importedMetadata.getTrackInfo().getKey(); + const auto newReplayGain = importedMetadata.getTrackInfo().getReplayGain(); { // enter locking scope @@ -137,11 +137,11 @@ void Track::setTrackMetadata( &m_record.refMetadataSynchronized(), !metadataSynchronized.isNull()); bool modifiedReplayGain = false; - if (m_record.getMetadata() != trackMetadata) { + if (m_record.getMetadata() != importedMetadata) { modifiedReplayGain = (m_record.getMetadata().getTrackInfo().getReplayGain() != newReplayGain); - m_record.setMetadata(std::move(trackMetadata)); - // Don't use trackMetadata after move assignment!! + m_record.setMetadata(std::move(importedMetadata)); + // Don't use importedMetadata after move assignment!! modified = true; } if (modified) { @@ -167,7 +167,13 @@ void Track::setTrackMetadata( } } -void Track::getTrackMetadata( +void Track::mergeImportedMetadata( + const mixxx::TrackMetadata& importedMetadata) { + QMutexLocker lock(&m_qMutex); + m_record.mergeImportedMetadata(importedMetadata); +} + +void Track::readTrackMetadata( mixxx::TrackMetadata* pTrackMetadata, bool* pMetadataSynchronized) const { DEBUG_ASSERT(pTrackMetadata); @@ -178,7 +184,7 @@ void Track::getTrackMetadata( } } -void Track::getTrackRecord( +void Track::readTrackRecord( mixxx::TrackRecord* pTrackRecord, bool* pDirty) const { DEBUG_ASSERT(pTrackRecord); @@ -935,93 +941,81 @@ ExportTrackMetadataResult Track::exportMetadata( // be called after all references to the object have been dropped. // But it doesn't hurt much, so let's play it safe ;) QMutexLocker lock(&m_qMutex); - // Discard the values of all currently unsupported fields that are - // not stored in the library, yet. Those fields are already imported - // from file tags, but the database schema needs to be extended for - // storing them. Currently those fields will be empty/null when read - // from the database and must be ignored until the schema has been - // updated. - m_record.refMetadata().resetUnsupportedValues(); + // TODO(XXX): m_record.getMetadataSynchronized() currently is a + // boolean flag, but it should become a time stamp in the future. + // We could take this time stamp and the file's last modification + // time stamp into account and might decide to skip importing + // the metadata again. + if (!m_bMarkedForMetadataExport && !m_record.getMetadataSynchronized()) { + // If the metadata has never been imported from file tags it + // must be exported explicitly once. This ensures that we don't + // overwrite existing file tags with completely different + // information. + kLogger.info() + << "Skip exporting of unsynchronized track metadata:" + << getLocation(); + // abort + return ExportTrackMetadataResult::Skipped; + } // Normalize metadata before exporting to adjust the precision of // floating values, ... Otherwise the following comparisons may // repeatedly indicate that values have changed only due to // rounding errors. m_record.refMetadata().normalizeBeforeExport(); - if (!m_bMarkedForMetadataExport) { - // Perform some consistency checks if metadata is exported - // implicitly after a track has been modified and NOT explicitly - // requested by a user as indicated by this flag. - if (!m_record.getMetadataSynchronized()) { - // If the metadata has never been imported from file tags it - // must be exported explicitly once. This ensures that we don't - // overwrite existing file tags with completely different - // information. - kLogger.info() - << "Skip exporting of unsynchronized track metadata:" - << getLocation(); + // Check if the metadata has actually been modified. Otherwise + // we don't need to write it back. Exporting unmodified metadata + // would needlessly update the file's time stamp and should be + // avoided. Since we don't know in which state the file's metadata + // is we import it again into a temporary variable. + mixxx::TrackMetadata importedFromFile; + if ((pMetadataSource->importTrackMetadataAndCoverImage(&importedFromFile, nullptr).first == + mixxx::MetadataSource::ImportResult::Succeeded)) { + // Prevent overwriting any file tags that are not yet stored in the + // library database! + m_record.mergeImportedMetadata(importedFromFile); + // Finally the track's current metadata and the imported/adjusted metadata + // can be compared for differences to decide whether the tags in the file + // would change if we perform the write operation. This function will also + // copy all extra properties that are not (yet) stored in the library before + // checking for differences! If an export has been requested explicitly then + // we will continue even if no differences are detected. + if (!m_bMarkedForMetadataExport && + !m_record.getMetadata().anyFileTagsModified(importedFromFile)) { + // The file tags are in-sync with the track's metadata and don't need + // to be updated. + if (kLogger.debugEnabled()) { + kLogger.debug() + << "Skip exporting of unmodified track metadata into file:" + << getLocation(); + } + // abort return ExportTrackMetadataResult::Skipped; } - // Check if the metadata has actually been modified. Otherwise - // we don't need to write it back. Exporting unmodified metadata - // would needlessly update the file's time stamp and should be - // avoided. Since we don't know in which state the file's metadata - // is we import it again into a temporary variable. - // TODO(XXX): m_record.getMetadataSynchronized() currently is a - // boolean flag, but it should become a time stamp in the future. - // We could take this time stamp and the file's last modification - // time stamp into // account and might decide to skip importing - // the metadata again. - mixxx::TrackMetadata importedFromFile; - if ((pMetadataSource->importTrackMetadataAndCoverImage(&importedFromFile, nullptr).first == - mixxx::MetadataSource::ImportResult::Succeeded)) { - // Discard the values of all currently unsupported fields that are - // not stored in the library, yet. We have done the same with the track's - // current metadata to make the tags comparable (see above). - importedFromFile.resetUnsupportedValues(); - // Account for floating-point rounding errors before exporting - // the BPM value. The imported value must be compared to the - // pre-normalized value for valid results. - // NOTE(2019-02-19, uklotzde): The pre-normalization cannot prevent - // repeated export of metadata for files with ID3 tags that are only - // able to store the BPM value with integer precision! In case of a - // fractional value the ID3 metadata is always detected as modified - // and will be exported regardless if it has actually been modified - // or not. - auto currentBpm = m_record.getMetadata().getTrackInfo().getBpm(); - currentBpm.normalizeBeforeExport(); - m_record.refMetadata().refTrackInfo().setBpm(currentBpm); - // Finally the track's current metadata and the imported/adjusted metadata - // can be compared for differences to decide whether the tags in the file - // would change if we perform the write operation. We are using a special - // comparison function that excludes all read-only audio properties which - // are stored in file tags, but may not be accurate. They can't be written - // anyway, so we must not take them into account here. - if (!m_record.getMetadata().hasBeenModifiedAfterImport(importedFromFile)) { - // The file tags are in-sync with the track's metadata and don't need - // to be updated. - if (kLogger.debugEnabled()) { - kLogger.debug() - << "Skip exporting of unmodified track metadata into file:" - << getLocation(); - } - return ExportTrackMetadataResult::Skipped; - } + } else { + // The file doesn't contain any tags yet or it might be missing, unreadable, + // or corrupt. + if (m_bMarkedForMetadataExport) { + kLogger.info() + << "Adding or overwriting tags after failure to import tags from file:" + << getLocation(); + // ...and continue } else { - // Something must be wrong with the file or it doesn't - // contain any file tags. We don't want to risk a failure - // during export and abort the operation for safety here. - // The user may decide to explicitly export the metadata. kLogger.warning() - << "Skip exporting of track metadata after import failed." - << "Export of metadata must be triggered explicitly for this file:" + << "Skip exporting of track metadata after failure to import tags from file:" << getLocation(); + // abort return ExportTrackMetadataResult::Skipped; } - // ...by continuing the file tags will be updated } // The track's metadata will be exported instantly. The export should // only be tried once so we reset the marker flag. m_bMarkedForMetadataExport = false; + kLogger.debug() + << "Old metadata (imported)" + << importedFromFile; + kLogger.debug() + << "New metadata (modified)" + << m_record.getMetadata(); const auto trackMetadataExported = pMetadataSource->exportTrackMetadata(m_record.getMetadata()); if (trackMetadataExported.first == mixxx::MetadataSource::ExportResult::Succeeded) { diff --git a/src/track/track.h b/src/track/track.h index 4e724c2b5fa..87a9635fc6c 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -281,14 +281,18 @@ class Track : public QObject { quint16 getCoverHash() const; // Set/get track metadata and cover art (optional) all at once. - void setTrackMetadata( - mixxx::TrackMetadata trackMetadata, + void importMetadata( + mixxx::TrackMetadata importedMetadata, QDateTime metadataSynchronized); - void getTrackMetadata( + // Merge additional metadata that is not (yet) stored in the database + // and only available from file tags. + void mergeImportedMetadata( + const mixxx::TrackMetadata& importedMetadata); + + void readTrackMetadata( mixxx::TrackMetadata* pTrackMetadata, bool* pMetadataSynchronized = nullptr) const; - - void getTrackRecord( + void readTrackRecord( mixxx::TrackRecord* pTrackRecord, bool* pDirty = nullptr) const; diff --git a/src/track/trackinfo.cpp b/src/track/trackinfo.cpp index ca95b487591..93baf8867ac 100644 --- a/src/track/trackinfo.cpp +++ b/src/track/trackinfo.cpp @@ -3,28 +3,6 @@ namespace mixxx { -void TrackInfo::resetUnsupportedValues() { -#if defined(__EXTRA_METADATA__) - setConductor(QString()); - setDiscNumber(QString()); - setDiscTotal(QString()); - setEncoder(QString()); - setEncoderSettings(QString()); - setISRC(QString()); - setLanguage(QString()); - setLyricist(QString()); - setMood(QString()); - setMovement(QString()); - setMusicBrainzArtistId(QString()); - setMusicBrainzRecordingId(QString()); - setMusicBrainzReleaseId(QString()); - setMusicBrainzWorkId(QString()); - setRemixer(QString()); - setSubtitle(QString()); - setWork(QString()); -#endif // __EXTRA_METADATA__ -} - namespace { const QString kArtistTitleSeparatorWithSpaces = " - "; diff --git a/src/track/trackinfo.h b/src/track/trackinfo.h index 4f9907bd003..a46b89958da 100644 --- a/src/track/trackinfo.h +++ b/src/track/trackinfo.h @@ -72,9 +72,6 @@ class TrackInfo final { QString fileName, bool splitArtistTitle); - // TODO(XXX): Remove after all new fields have been added to the library - void resetUnsupportedValues(); - // Adjusts floating-point properties to match their string representation // in file tags to account for rounding errors. void normalizeBeforeExport() { diff --git a/src/track/trackmetadata.cpp b/src/track/trackmetadata.cpp index e3fc9a9202b..6bfd6f58b3d 100644 --- a/src/track/trackmetadata.cpp +++ b/src/track/trackmetadata.cpp @@ -66,6 +66,21 @@ QString TrackMetadata::reformatYear(QString year) { return year.simplified(); } +void TrackMetadata::normalizeBeforeExport() { + refAlbumInfo().normalizeBeforeExport(); + refTrackInfo().normalizeBeforeExport(); +} + +bool TrackMetadata::anyFileTagsModified( + const TrackMetadata& importedFromFile) const { + // NOTE(uklotzde): The read-only audio properties that are stored + // directly as members of this class might differ after they have + // been updated while decoding audio data. They are read-only and + // must not be considered when exporting metadata! + return getAlbumInfo() != importedFromFile.getAlbumInfo() || + getTrackInfo() != importedFromFile.getTrackInfo(); +} + bool operator==(const TrackMetadata& lhs, const TrackMetadata& rhs) { return (lhs.getAlbumInfo() == rhs.getAlbumInfo()) && (lhs.getTrackInfo() == rhs.getTrackInfo()) && diff --git a/src/track/trackmetadata.h b/src/track/trackmetadata.h index 956b6c7107f..9d4888841a2 100644 --- a/src/track/trackmetadata.h +++ b/src/track/trackmetadata.h @@ -33,28 +33,14 @@ class TrackMetadata final { TrackMetadata& operator=(TrackMetadata&&) = default; TrackMetadata& operator=(const TrackMetadata&) = default; - // TODO(XXX): Remove after all new fields have been added to the library - void resetUnsupportedValues() { - refAlbumInfo().resetUnsupportedValues(); - refTrackInfo().resetUnsupportedValues(); - } - // Adjusts floating-point values to match their string representation // in file tags to account for rounding errors. - void normalizeBeforeExport() { - refAlbumInfo().normalizeBeforeExport(); - refTrackInfo().normalizeBeforeExport(); - } + void normalizeBeforeExport(); - // Compares the contents with metadata that has been freshly imported - // from a file. - bool hasBeenModifiedAfterImport(const TrackMetadata& importedFromFile) const { - // NOTE(uklotzde): The read-only audio properties might differ after - // they have been updated while decoding audio data. They are read-only - // and must not be considered when exporting metadata. - return (getAlbumInfo() != importedFromFile.getAlbumInfo()) || - (getTrackInfo() != importedFromFile.getTrackInfo()); - } + // Returns true if the current metadata differs from the imported metadata + // and needs to be exported. A result of false indicates that no export + // is needed. + bool anyFileTagsModified(const TrackMetadata& importedFromFile) const; // Parse an format date/time values according to ISO 8601 static QDate parseDate(QString str) { diff --git a/src/track/trackmetadatataglib.cpp b/src/track/trackmetadatataglib.cpp index 259e7d2d543..37f2f6c632f 100644 --- a/src/track/trackmetadatataglib.cpp +++ b/src/track/trackmetadatataglib.cpp @@ -66,6 +66,13 @@ bool checkID3v2HeaderVersionSupported(const TagLib::ID3v2::Header& header) { } } +// Owners of ID3v2 UFID frames. +// NOTE(uklotzde, 2019-09-28): This is the owner string for MusicBrainz +// as written by MusicBrainz Picard 2.1.3 although the mapping table +// doesn't mention any "http://" prefix. +// See also: https://picard.musicbrainz.org/docs/mappings +const QString kMusicBrainzOwner = "http://musicbrainz.org"; + } // anonymous namespace namespace taglib { @@ -352,11 +359,6 @@ QString formatReplayGainPeak(const ReplayGain& replayGain) { return ReplayGain::peakToString(replayGain.getPeak()); } -inline -bool hasTrackPeak(const TrackMetadata& trackMetadata) { - return trackMetadata.getTrackInfo().getReplayGain().hasPeak(); -} - inline QString formatTrackPeak(const TrackMetadata& trackMetadata) { return formatReplayGainPeak(trackMetadata.getTrackInfo().getReplayGain()); @@ -389,11 +391,6 @@ bool parseTrackPeak( } #if defined(__EXTRA_METADATA__) -inline -bool hasAlbumGain(const TrackMetadata& trackMetadata) { - return trackMetadata.getAlbumInfo().getReplayGain().hasRatio(); -} - inline QString formatAlbumGain(const TrackMetadata& trackMetadata) { return formatReplayGainGain(trackMetadata.getAlbumInfo().getReplayGain()); @@ -412,11 +409,6 @@ bool parseAlbumGain( return isRatioValid; } -inline -bool hasAlbumPeak(const TrackMetadata& trackMetadata) { - return trackMetadata.getAlbumInfo().getReplayGain().hasPeak(); -} - inline QString formatAlbumPeak(const TrackMetadata& trackMetadata) { return formatReplayGainPeak(trackMetadata.getAlbumInfo().getReplayGain()); @@ -633,47 +625,6 @@ QString readFirstUserTextIdentificationFrame( } } -// Deletes all TXXX frame with the given description (case-insensitive). -int removeUserTextIdentificationFrames( - TagLib::ID3v2::Tag* pTag, - const QString& description) { - DEBUG_ASSERT(pTag); - DEBUG_ASSERT(!description.isEmpty()); - int count = 0; - bool repeat; - do { - repeat = false; - // Bind the const-ref result to avoid a local copy - const TagLib::ID3v2::FrameList& textFrames = - pTag->frameListMap()["TXXX"]; - for (TagLib::ID3v2::FrameList::ConstIterator it(textFrames.begin()); - it != textFrames.end(); ++it) { - auto pFrame = - dynamic_cast(*it); - if (pFrame) { - const QString frameDescription( - toQString(pFrame->description())); - if (0 == frameDescription.compare( - description, Qt::CaseInsensitive)) { - if (kLogger.debugEnabled()) { - kLogger.debug() - << "Removing ID3v2 TXXX frame:" - << toQString(pFrame->description()); - } - // After removing a frame the result of frameListMap() - // is no longer valid!! - pTag->removeFrame(pFrame, false); // remove an unowned frame - ++count; - // Exit and restart loop - repeat = true; - break; - } - } - } - } while (repeat); - return count; -} - #if defined(__EXTRA_METADATA__) // Finds the first UFID frame that with a matching owner (case-insensitive). // If multiple UFID frames with matching descriptions exist prefer the first @@ -712,7 +663,6 @@ TagLib::ID3v2::UniqueFileIdentifierFrame* findFirstUniqueFileIdentifierFrame( return pFirstFrame; } -inline QByteArray readFirstUniqueFileIdentifierFrame( const TagLib::ID3v2::Tag& tag, const QString& owner) { @@ -800,13 +750,13 @@ void writeID3v2TextIdentificationFrame( } } -void writeID3v2CommentsFrame( +void writeID3v2UserTextIdentificationFrame( TagLib::ID3v2::Tag* pTag, - const QString& text, const QString& description, + const QString& text, bool isNumericOrURL = false) { - TagLib::ID3v2::CommentsFrame* pFrame = - findFirstCommentsFrame(*pTag, description, true); + TagLib::ID3v2::UserTextIdentificationFrame* pFrame = + findFirstUserTextIdentificationFrame(*pTag, description); if (pFrame) { // Modify existing frame if (text.isEmpty()) { @@ -822,7 +772,7 @@ void writeID3v2CommentsFrame( const TagLib::String::Type stringType = getID3v2StringType(*pTag, isNumericOrURL); auto pFrame = - std::make_unique(stringType); + std::make_unique(stringType); pFrame->setDescription(toTagLibString(description)); pFrame->setText(toTagLibString(text)); pTag->addFrame(pFrame.get()); @@ -831,31 +781,56 @@ void writeID3v2CommentsFrame( pFrame.release(); } } - // Cleanup: Remove non-standard comment frames to avoid redundant and - // inconsistent tags. - // See also: Compatibility workaround when reading ID3v2 comment tags. - int numberOfRemovedCommentFrames = - removeUserTextIdentificationFrames(pTag, "COMMENT"); - if (numberOfRemovedCommentFrames > 0) { - kLogger.warning() << "Removed" << numberOfRemovedCommentFrames - << "non-standard ID3v2 TXXX comment frames"; - } } -void writeID3v2CommentsFrameWithoutDescription( +// Deletes all TXXX frame with the given description (case-insensitive). +int removeUserTextIdentificationFrames( TagLib::ID3v2::Tag* pTag, - const QString& text, - bool isNumericOrURL = false) { - writeID3v2CommentsFrame(pTag, text, QString(), isNumericOrURL); + const QString& description) { + DEBUG_ASSERT(pTag); + DEBUG_ASSERT(!description.isEmpty()); + int count = 0; + bool repeat; + do { + repeat = false; + // Bind the const-ref result to avoid a local copy + const TagLib::ID3v2::FrameList& textFrames = + pTag->frameListMap()["TXXX"]; + for (TagLib::ID3v2::FrameList::ConstIterator it(textFrames.begin()); + it != textFrames.end(); ++it) { + auto pFrame = + dynamic_cast(*it); + if (pFrame) { + const QString frameDescription( + toQString(pFrame->description())); + if (0 == frameDescription.compare( + description, Qt::CaseInsensitive)) { + if (kLogger.debugEnabled()) { + kLogger.debug() + << "Removing ID3v2 TXXX frame:" + << toQString(pFrame->description()); + } + // After removing a frame the result of frameListMap() + // is no longer valid!! + pTag->removeFrame(pFrame, false); // remove an unowned frame + ++count; + // Exit and restart loop + repeat = true; + break; + } + } + } + } while (repeat); + return count; } -void writeID3v2UserTextIdentificationFrame( +void writeID3v2CommentsFrame( TagLib::ID3v2::Tag* pTag, - const QString& description, const QString& text, + const QString& description, bool isNumericOrURL = false) { - TagLib::ID3v2::UserTextIdentificationFrame* pFrame = - findFirstUserTextIdentificationFrame(*pTag, description); + TagLib::ID3v2::CommentsFrame* pFrame = + findFirstCommentsFrame(*pTag, description, true); if (pFrame) { // Modify existing frame if (text.isEmpty()) { @@ -871,7 +846,7 @@ void writeID3v2UserTextIdentificationFrame( const TagLib::String::Type stringType = getID3v2StringType(*pTag, isNumericOrURL); auto pFrame = - std::make_unique(stringType); + std::make_unique(stringType); pFrame->setDescription(toTagLibString(description)); pFrame->setText(toTagLibString(text)); pTag->addFrame(pFrame.get()); @@ -880,21 +855,25 @@ void writeID3v2UserTextIdentificationFrame( pFrame.release(); } } + // Cleanup: Remove non-standard comment frames to avoid redundant and + // inconsistent tags. + // See also: Compatibility workaround when reading ID3v2 comment tags. + int numberOfRemovedCommentFrames = + removeUserTextIdentificationFrames(pTag, "COMMENT"); + if (numberOfRemovedCommentFrames > 0) { + kLogger.warning() << "Removed" << numberOfRemovedCommentFrames + << "non-standard ID3v2 TXXX comment frames"; + } } -#if defined(__EXTRA_METADATA__) -bool writeID3v2TextIdentificationFrameStringIfNotNull( +void writeID3v2CommentsFrameWithoutDescription( TagLib::ID3v2::Tag* pTag, - const TagLib::ByteVector &id, - const QString& text) { - if (text.isNull()) { - return false; - } else { - writeID3v2TextIdentificationFrame(pTag, id, text); - return true; - } + const QString& text, + bool isNumericOrURL = false) { + writeID3v2CommentsFrame(pTag, text, QString(), isNumericOrURL); } +#if defined(__EXTRA_METADATA__) void writeID3v2UniqueFileIdentifierFrame( TagLib::ID3v2::Tag* pTag, const QString& owner, @@ -1585,7 +1564,7 @@ void importTrackMetadataFromID3v2Tag( pTrackMetadata->refTrackInfo().setMusicBrainzArtistId(QUuid(trackArtistId)); } QByteArray trackRecordingId = - readFirstUniqueFileIdentifierFrame(tag, "http://musicbrainz.org"); + readFirstUniqueFileIdentifierFrame(tag, kMusicBrainzOwner); if (!trackRecordingId.isEmpty()) { pTrackMetadata->refTrackInfo().setMusicBrainzRecordingId(QUuid(trackRecordingId)); } @@ -2381,11 +2360,11 @@ bool exportTrackMetadataIntoID3v2Tag(TagLib::ID3v2::Tag* pTag, "GRP1", trackMetadata.getTrackInfo().getGrouping()); #if defined(__EXTRA_METADATA__) - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TIT1", trackMetadata.getTrackInfo().getWork()); - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "MVNM", trackMetadata.getTrackInfo().getMovement()); @@ -2412,142 +2391,120 @@ bool exportTrackMetadataIntoID3v2Tag(TagLib::ID3v2::Tag* pTag, "REPLAYGAIN_TRACK_GAIN", formatTrackGain(trackMetadata), true); - // NOTE(uklotzde, 2018-04-22): The analyzers currently doesn't - // calculate a peak value, so leave it untouched in the file if - // the value is invalid/absent. Otherwise the ID3 frame would - // be deleted. - if (hasTrackPeak(trackMetadata)) { - writeID3v2UserTextIdentificationFrame( - pTag, - "REPLAYGAIN_TRACK_PEAK", - formatTrackPeak(trackMetadata), - true); - } + writeID3v2UserTextIdentificationFrame( + pTag, + "REPLAYGAIN_TRACK_PEAK", + formatTrackPeak(trackMetadata), + true); - // TODO(XXX): The following tags have been added later and are currently - // not stored in the Mixxx library. Only write fields that have non-null - // values to preserve any existing file tags instead of removing them! #if defined(__EXTRA_METADATA__) - if (hasAlbumGain(trackMetadata)) { - writeID3v2UserTextIdentificationFrame( - pTag, - "REPLAYGAIN_ALBUM_GAIN", - formatAlbumGain(trackMetadata), - true); - } - if (hasAlbumPeak(trackMetadata)) { - writeID3v2UserTextIdentificationFrame( - pTag, - "REPLAYGAIN_ALBUM_PEAK", - formatAlbumPeak(trackMetadata), - true); - } + writeID3v2TextIdentificationFrame(pTag, "TPOS", TrackNumbers::joinStrings( + trackMetadata.getTrackInfo().getDiscNumber(), + trackMetadata.getTrackInfo().getDiscTotal())); - if (!trackMetadata.getTrackInfo().getMusicBrainzArtistId().isNull()) { - writeID3v2UserTextIdentificationFrame( - pTag, - "MusicBrainz Artist Id", - trackMetadata.getTrackInfo().getMusicBrainzArtistId().toString(), - false); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzRecordingId().isNull()) { + writeID3v2UserTextIdentificationFrame( + pTag, + "REPLAYGAIN_ALBUM_GAIN", + formatAlbumGain(trackMetadata), + true); + writeID3v2UserTextIdentificationFrame( + pTag, + "REPLAYGAIN_ALBUM_PEAK", + formatAlbumPeak(trackMetadata), + true); + + writeID3v2UserTextIdentificationFrame( + pTag, + "MusicBrainz Artist Id", + formatUuid(trackMetadata.getTrackInfo().getMusicBrainzArtistId()), + false); + { QByteArray identifier = trackMetadata.getTrackInfo().getMusicBrainzRecordingId().toByteArray(); if (identifier.size() == 38) { // Strip leading/trailing curly braces + DEBUG_ASSERT(identifier.startsWith('{')); + DEBUG_ASSERT(identifier.endsWith('}')); identifier = identifier.mid(1, 36); } DEBUG_ASSERT(identifier.size() == 36); writeID3v2UniqueFileIdentifierFrame( pTag, - "http://musicbrainz.org", + kMusicBrainzOwner, identifier); } - if (!trackMetadata.getTrackInfo().getMusicBrainzReleaseId().isNull()) { - writeID3v2UserTextIdentificationFrame( - pTag, - "MusicBrainz Release Track Id", - trackMetadata.getTrackInfo().getMusicBrainzReleaseId().toString(), - false); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzWorkId().isNull()) { - writeID3v2UserTextIdentificationFrame( - pTag, - "MusicBrainz Work Id", - trackMetadata.getTrackInfo().getMusicBrainzWorkId().toString(), - false); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzArtistId().isNull()) { - writeID3v2UserTextIdentificationFrame( - pTag, - "MusicBrainz Album Artist Id", - trackMetadata.getAlbumInfo().getMusicBrainzArtistId().toString(), - false); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzReleaseId().isNull()) { - writeID3v2UserTextIdentificationFrame( - pTag, - "MusicBrainz Album Id", - trackMetadata.getAlbumInfo().getMusicBrainzReleaseId().toString(), - false); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId().isNull()) { - writeID3v2UserTextIdentificationFrame( - pTag, - "MusicBrainz Release Group Id", - trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId().toString(), - false); - } + writeID3v2UserTextIdentificationFrame( + pTag, + "MusicBrainz Release Track Id", + formatUuid(trackMetadata.getTrackInfo().getMusicBrainzReleaseId()), + false); + writeID3v2UserTextIdentificationFrame( + pTag, + "MusicBrainz Work Id", + formatUuid(trackMetadata.getTrackInfo().getMusicBrainzWorkId()), + false); + writeID3v2UserTextIdentificationFrame( + pTag, + "MusicBrainz Album Artist Id", + formatUuid(trackMetadata.getAlbumInfo().getMusicBrainzArtistId()), + false); + writeID3v2UserTextIdentificationFrame( + pTag, + "MusicBrainz Album Id", + formatUuid(trackMetadata.getAlbumInfo().getMusicBrainzReleaseId()), + false); + writeID3v2UserTextIdentificationFrame( + pTag, + "MusicBrainz Release Group Id", + formatUuid(trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId()), + false); - writeID3v2TextIdentificationFrameStringIfNotNull(pTag, "TPOS", TrackNumbers::joinStrings( - trackMetadata.getTrackInfo().getDiscNumber(), - trackMetadata.getTrackInfo().getDiscTotal())); - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TPE3", trackMetadata.getTrackInfo().getConductor()); - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TSRC", trackMetadata.getTrackInfo().getISRC()); - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TLAN", trackMetadata.getTrackInfo().getLanguage()); - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TEXT", trackMetadata.getTrackInfo().getLyricist()); if (pHeader->majorVersion() >= 4) { - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TMOO", trackMetadata.getTrackInfo().getMood()); } - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TCOP", trackMetadata.getAlbumInfo().getCopyright()); - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "WCOP", trackMetadata.getAlbumInfo().getLicense()); - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TPUB", trackMetadata.getAlbumInfo().getRecordLabel()); - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TPE4", trackMetadata.getTrackInfo().getRemixer()); - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TIT3", trackMetadata.getTrackInfo().getSubtitle()); - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TENC", trackMetadata.getTrackInfo().getEncoder()); - writeID3v2TextIdentificationFrameStringIfNotNull( + writeID3v2TextIdentificationFrame( pTag, "TSSE", trackMetadata.getTrackInfo().getEncoderSettings()); @@ -2590,112 +2547,59 @@ bool exportTrackMetadataIntoAPETag(TagLib::APE::Tag* pTag, const TrackMetadata& writeAPEItem(pTag, "REPLAYGAIN_TRACK_GAIN", toTagLibString(formatTrackGain(trackMetadata))); - // NOTE(uklotzde, 2018-04-22): The analyzers currently doesn't - // calculate a peak value, so leave it untouched in the file if - // the value is invalid/absent. Otherwise the APE item would be - // deleted. - if (hasTrackPeak(trackMetadata)) { - writeAPEItem(pTag, "REPLAYGAIN_TRACK_PEAK", - toTagLibString(formatTrackPeak(trackMetadata))); - } + writeAPEItem(pTag, "REPLAYGAIN_TRACK_PEAK", + toTagLibString(formatTrackPeak(trackMetadata))); - // TODO(XXX): The following tags have been added later and are currently - // not stored in the Mixxx library. Only write fields that have non-null - // values to preserve any existing file tags instead of removing them! #if defined(__EXTRA_METADATA__) auto discNumbers = TrackNumbers::joinStrings( trackMetadata.getTrackInfo().getDiscNumber(), trackMetadata.getTrackInfo().getDiscTotal()); - if (!discNumbers.isNull()) { - writeAPEItem(pTag, "Disc", toTagLibString(discNumbers)); - } - - if (hasAlbumGain(trackMetadata)) { - writeAPEItem(pTag, "REPLAYGAIN_ALBUM_GAIN", - toTagLibString(formatAlbumGain(trackMetadata))); - } - if (hasAlbumPeak(trackMetadata)) { - writeAPEItem(pTag, "REPLAYGAIN_ALBUM_PEAK", - toTagLibString(formatAlbumPeak(trackMetadata))); - } - - if (!trackMetadata.getTrackInfo().getMusicBrainzArtistId().isNull()) { - writeAPEItem(pTag, "MUSICBRAINZ_ARTISTID", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzArtistId().toString())); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzRecordingId().isNull()) { - writeAPEItem(pTag, "MUSICBRAINZ_TRACKID", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzRecordingId().toString())); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzReleaseId().isNull()) { - writeAPEItem(pTag, "MUSICBRAINZ_RELEASETRACKID", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzReleaseId().toString())); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzWorkId().isNull()) { - writeAPEItem(pTag, "MUSICBRAINZ_WORKID", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzWorkId().toString())); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzArtistId().isNull()) { - writeAPEItem(pTag, "MUSICBRAINZ_ALBUMARTISTID", - toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzArtistId().toString())); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzReleaseId().isNull()) { - writeAPEItem(pTag, "MUSICBRAINZ_ALBUMID", - toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseId().toString())); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId().isNull()) { - writeAPEItem(pTag, "MUSICBRAINZ_RELEASEGROUPID", - toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId().toString())); - } - - if (!trackMetadata.getTrackInfo().getConductor().isNull()) { - writeAPEItem(pTag, "Conductor", - toTagLibString(trackMetadata.getTrackInfo().getConductor())); - } - if (!trackMetadata.getTrackInfo().getISRC().isNull()) { - writeAPEItem(pTag, "ISRC", - toTagLibString(trackMetadata.getTrackInfo().getISRC())); - } - if (!trackMetadata.getTrackInfo().getLanguage().isNull()) { - writeAPEItem(pTag, "Language", - toTagLibString(trackMetadata.getTrackInfo().getLanguage())); - } - if (!trackMetadata.getTrackInfo().getLyricist().isNull()) { - writeAPEItem(pTag, "Lyricist", - toTagLibString(trackMetadata.getTrackInfo().getLyricist())); - } - if (!trackMetadata.getTrackInfo().getMood().isNull()) { - writeAPEItem(pTag, "Mood", - toTagLibString(trackMetadata.getTrackInfo().getMood())); - } - if (!trackMetadata.getAlbumInfo().getCopyright().isNull()) { - writeAPEItem(pTag, "Copyright", - toTagLibString(trackMetadata.getAlbumInfo().getCopyright())); - } - if (!trackMetadata.getAlbumInfo().getLicense().isNull()) { - writeAPEItem(pTag, "LICENSE", - toTagLibString(trackMetadata.getAlbumInfo().getLicense())); - } - if (!trackMetadata.getAlbumInfo().getRecordLabel().isNull()) { - writeAPEItem(pTag, "Label", - toTagLibString(trackMetadata.getAlbumInfo().getRecordLabel())); - } - if (!trackMetadata.getTrackInfo().getRemixer().isNull()) { - writeAPEItem(pTag, "MixArtist", - toTagLibString(trackMetadata.getTrackInfo().getRemixer())); - } - if (!trackMetadata.getTrackInfo().getSubtitle().isNull()) { - writeAPEItem(pTag, "Subtitle", - toTagLibString(trackMetadata.getTrackInfo().getSubtitle())); - } - if (!trackMetadata.getTrackInfo().getEncoder().isNull()) { - writeAPEItem(pTag, "EncodedBy", - toTagLibString(trackMetadata.getTrackInfo().getEncoder())); - } - if (!trackMetadata.getTrackInfo().getEncoderSettings().isNull()) { - writeAPEItem(pTag, "EncoderSettings", - toTagLibString(trackMetadata.getTrackInfo().getEncoderSettings())); - } + writeAPEItem(pTag, "Disc", toTagLibString(discNumbers)); + + writeAPEItem(pTag, "REPLAYGAIN_ALBUM_GAIN", + toTagLibString(formatAlbumGain(trackMetadata))); + writeAPEItem(pTag, "REPLAYGAIN_ALBUM_PEAK", + toTagLibString(formatAlbumPeak(trackMetadata))); + + writeAPEItem(pTag, "MUSICBRAINZ_ARTISTID", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzArtistId())); + writeAPEItem(pTag, "MUSICBRAINZ_TRACKID", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzRecordingId())); + writeAPEItem(pTag, "MUSICBRAINZ_RELEASETRACKID", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzReleaseId())); + writeAPEItem(pTag, "MUSICBRAINZ_WORKID", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzWorkId())); + writeAPEItem(pTag, "MUSICBRAINZ_ALBUMARTISTID", + toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzArtistId())); + writeAPEItem(pTag, "MUSICBRAINZ_ALBUMID", + toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseId())); + writeAPEItem(pTag, "MUSICBRAINZ_RELEASEGROUPID", + toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId())); + + writeAPEItem(pTag, "Conductor", + toTagLibString(trackMetadata.getTrackInfo().getConductor())); + writeAPEItem(pTag, "ISRC", + toTagLibString(trackMetadata.getTrackInfo().getISRC())); + writeAPEItem(pTag, "Language", + toTagLibString(trackMetadata.getTrackInfo().getLanguage())); + writeAPEItem(pTag, "Lyricist", + toTagLibString(trackMetadata.getTrackInfo().getLyricist())); + writeAPEItem(pTag, "Mood", + toTagLibString(trackMetadata.getTrackInfo().getMood())); + writeAPEItem(pTag, "Copyright", + toTagLibString(trackMetadata.getAlbumInfo().getCopyright())); + writeAPEItem(pTag, "LICENSE", + toTagLibString(trackMetadata.getAlbumInfo().getLicense())); + writeAPEItem(pTag, "Label", + toTagLibString(trackMetadata.getAlbumInfo().getRecordLabel())); + writeAPEItem(pTag, "MixArtist", + toTagLibString(trackMetadata.getTrackInfo().getRemixer())); + writeAPEItem(pTag, "Subtitle", + toTagLibString(trackMetadata.getTrackInfo().getSubtitle())); + writeAPEItem(pTag, "EncodedBy", + toTagLibString(trackMetadata.getTrackInfo().getEncoder())); + writeAPEItem(pTag, "EncoderSettings", + toTagLibString(trackMetadata.getTrackInfo().getEncoderSettings())); #endif // __EXTRA_METADATA__ return true; @@ -2777,110 +2681,62 @@ bool exportTrackMetadataIntoXiphComment(TagLib::Ogg::XiphComment* pTag, // calculate a peak value, so leave it untouched in the file if // the value is invalid/absent. Otherwise the comment field would // be deleted. - if (hasTrackPeak(trackMetadata)) { - writeXiphCommentField(pTag, "REPLAYGAIN_TRACK_PEAK", - toTagLibString(formatTrackPeak(trackMetadata))); - } + writeXiphCommentField(pTag, "REPLAYGAIN_TRACK_PEAK", + toTagLibString(formatTrackPeak(trackMetadata))); - // TODO(XXX): The following tags have been added later and are currently - // not stored in the Mixxx library. Only write fields that have non-null - // values to preserve any existing file tags instead of removing them! #if defined(__EXTRA_METADATA__) - if (hasAlbumGain(trackMetadata)) { - writeXiphCommentField(pTag, "REPLAYGAIN_ALBUM_GAIN", - toTagLibString(formatAlbumGain(trackMetadata))); - } - if (hasAlbumPeak(trackMetadata)) { - writeXiphCommentField(pTag, "REPLAYGAIN_ALBUM_PEAK", - toTagLibString(formatAlbumPeak(trackMetadata))); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzArtistId().isNull()) { - writeXiphCommentField(pTag, "MUSICBRAINZ_ARTISTID", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzArtistId().toString())); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzRecordingId().isNull()) { - writeXiphCommentField(pTag, "MUSICBRAINZ_TRACKID", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzRecordingId().toString())); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzReleaseId().isNull()) { - writeXiphCommentField(pTag, "MUSICBRAINZ_RELEASETRACKID", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzReleaseId().toString())); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzWorkId().isNull()) { - writeXiphCommentField(pTag, "MUSICBRAINZ_WORKID", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzWorkId().toString())); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzArtistId().isNull()) { - writeXiphCommentField(pTag, "MUSICBRAINZ_ALBUMARTISTID", - toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzArtistId().toString())); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzReleaseId().isNull()) { - writeXiphCommentField(pTag, "MUSICBRAINZ_ALBUMID", - toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseId().toString())); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId().isNull()) { - writeXiphCommentField(pTag, "MUSICBRAINZ_RELEASEGROUPID", - toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId().toString())); - } - if (!trackMetadata.getTrackInfo().getConductor().isNull()) { - writeXiphCommentField(pTag, "CONDUCTOR", - toTagLibString(trackMetadata.getTrackInfo().getConductor())); - } - if (!trackMetadata.getTrackInfo().getISRC().isNull()) { - writeXiphCommentField(pTag, "ISRC", - toTagLibString(trackMetadata.getTrackInfo().getISRC())); - } - if (!trackMetadata.getTrackInfo().getLanguage().isNull()) { - writeXiphCommentField(pTag, "LANGUAGE", - toTagLibString(trackMetadata.getTrackInfo().getLanguage())); - } - if (!trackMetadata.getTrackInfo().getLyricist().isNull()) { - writeXiphCommentField(pTag, "LYRICIST", - toTagLibString(trackMetadata.getTrackInfo().getLyricist())); - } - if (!trackMetadata.getTrackInfo().getMood().isNull()) { - writeXiphCommentField(pTag, "MOOD", - toTagLibString(trackMetadata.getTrackInfo().getMood())); - } - if (!trackMetadata.getAlbumInfo().getCopyright().isNull()) { - writeXiphCommentField(pTag, "COPYRIGHT", - toTagLibString(trackMetadata.getAlbumInfo().getCopyright())); - } - if (!trackMetadata.getAlbumInfo().getLicense().isNull()) { - writeXiphCommentField(pTag, "LICENSE", - toTagLibString(trackMetadata.getAlbumInfo().getLicense())); - } - if (!trackMetadata.getAlbumInfo().getRecordLabel().isNull()) { - writeXiphCommentField(pTag, "LABEL", - toTagLibString(trackMetadata.getAlbumInfo().getRecordLabel())); - } - if (!trackMetadata.getTrackInfo().getRemixer().isNull()) { - writeXiphCommentField(pTag, "REMIXER", - toTagLibString(trackMetadata.getTrackInfo().getRemixer())); - } - if (!trackMetadata.getTrackInfo().getSubtitle().isNull()) { - writeXiphCommentField(pTag, "SUBTITLE", - toTagLibString(trackMetadata.getTrackInfo().getSubtitle())); - } - if (!trackMetadata.getTrackInfo().getEncoder().isNull()) { - writeXiphCommentField(pTag, "ENCODEDBY", - toTagLibString(trackMetadata.getTrackInfo().getEncoder())); - } - if (!trackMetadata.getTrackInfo().getEncoderSettings().isNull()) { - writeXiphCommentField(pTag, "ENCODERSETTINGS", - toTagLibString(trackMetadata.getTrackInfo().getEncoderSettings())); - } - if (!trackMetadata.getTrackInfo().getDiscNumber().isNull()) { - writeXiphCommentField( - pTag, "DISCNUMBER", toTagLibString(trackMetadata.getTrackInfo().getDiscNumber())); - } // According to https://wiki.xiph.org/Field_names "DISCTOTAL" is // the proposed field name, but some applications use "TOTALDISCS". - if (!trackMetadata.getTrackInfo().getDiscTotal().isNull()) { - const TagLib::String discTotal(toTagLibString(trackMetadata.getTrackInfo().getDiscTotal())); - writeXiphCommentField(pTag, "DISCTOTAL", discTotal); // recommended field - updateXiphCommentField(pTag, "TOTALDISCS", discTotal); // alternative field - } + const TagLib::String discTotal(toTagLibString(trackMetadata.getTrackInfo().getDiscTotal())); + writeXiphCommentField(pTag, "DISCTOTAL", discTotal); // recommended field + updateXiphCommentField(pTag, "TOTALDISCS", discTotal); // alternative field + + writeXiphCommentField(pTag, "REPLAYGAIN_ALBUM_GAIN", + toTagLibString(formatAlbumGain(trackMetadata))); + writeXiphCommentField(pTag, "REPLAYGAIN_ALBUM_PEAK", + toTagLibString(formatAlbumPeak(trackMetadata))); + + writeXiphCommentField(pTag, "MUSICBRAINZ_ARTISTID", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzArtistId())); + writeXiphCommentField(pTag, "MUSICBRAINZ_TRACKID", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzRecordingId())); + writeXiphCommentField(pTag, "MUSICBRAINZ_RELEASETRACKID", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzReleaseId())); + writeXiphCommentField(pTag, "MUSICBRAINZ_WORKID", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzWorkId())); + writeXiphCommentField(pTag, "MUSICBRAINZ_ALBUMARTISTID", + toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzArtistId())); + writeXiphCommentField(pTag, "MUSICBRAINZ_ALBUMID", + toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseId())); + writeXiphCommentField(pTag, "MUSICBRAINZ_RELEASEGROUPID", + toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId())); + + writeXiphCommentField(pTag, "CONDUCTOR", + toTagLibString(trackMetadata.getTrackInfo().getConductor())); + writeXiphCommentField(pTag, "ISRC", + toTagLibString(trackMetadata.getTrackInfo().getISRC())); + writeXiphCommentField(pTag, "LANGUAGE", + toTagLibString(trackMetadata.getTrackInfo().getLanguage())); + writeXiphCommentField(pTag, "LYRICIST", + toTagLibString(trackMetadata.getTrackInfo().getLyricist())); + writeXiphCommentField(pTag, "MOOD", + toTagLibString(trackMetadata.getTrackInfo().getMood())); + writeXiphCommentField(pTag, "COPYRIGHT", + toTagLibString(trackMetadata.getAlbumInfo().getCopyright())); + writeXiphCommentField(pTag, "LICENSE", + toTagLibString(trackMetadata.getAlbumInfo().getLicense())); + writeXiphCommentField(pTag, "LABEL", + toTagLibString(trackMetadata.getAlbumInfo().getRecordLabel())); + writeXiphCommentField(pTag, "REMIXER", + toTagLibString(trackMetadata.getTrackInfo().getRemixer())); + writeXiphCommentField(pTag, "SUBTITLE", + toTagLibString(trackMetadata.getTrackInfo().getSubtitle())); + writeXiphCommentField(pTag, "ENCODEDBY", + toTagLibString(trackMetadata.getTrackInfo().getEncoder())); + writeXiphCommentField(pTag, "ENCODERSETTINGS", + toTagLibString(trackMetadata.getTrackInfo().getEncoderSettings())); + writeXiphCommentField( + pTag, "DISCNUMBER", toTagLibString(trackMetadata.getTrackInfo().getDiscNumber())); #endif // __EXTRA_METADATA__ return true; @@ -2942,18 +2798,9 @@ bool exportTrackMetadataIntoMP4Tag(TagLib::MP4::Tag* pTag, const TrackMetadata& writeMP4Atom(pTag, "----:com.apple.iTunes:replaygain_track_gain", toTagLibString(formatTrackGain(trackMetadata))); - // NOTE(uklotzde, 2018-04-22): The analyzers currently doesn't - // calculate a peak value, so leave it untouched in the file if - // the value is invalid/absent. Otherwise the MP4 atom would be - // deleted. - if (hasTrackPeak(trackMetadata)) { - writeMP4Atom(pTag, "----:com.apple.iTunes:replaygain_track_peak", - toTagLibString(formatTrackPeak(trackMetadata))); - } + writeMP4Atom(pTag, "----:com.apple.iTunes:replaygain_track_peak", + toTagLibString(formatTrackPeak(trackMetadata))); - // TODO(XXX): The following tags have been added later and are currently - // not stored in the Mixxx library. Only write fields that have non-null - // values to preserve any existing file tags instead of removing them! #if defined(__EXTRA_METADATA__) // Write disc number/total pair QString discNumberText; @@ -2964,9 +2811,7 @@ bool exportTrackMetadataIntoMP4Tag(TagLib::MP4::Tag* pTag, const TrackMetadata& trackMetadata.getTrackInfo().getDiscTotal(), &parsedDiscNumbers); switch (discParseResult) { case TrackNumbers::ParseResult::EMPTY: - // Preserve disc numbers in file and do NOT delete them - // if not already stored in the Mixxx database! Support - // for these fields have been added later. + pTag->itemListMap().erase("disk"); break; case TrackNumbers::ParseResult::VALID: pTag->itemListMap()["disk"] = @@ -2979,92 +2824,52 @@ bool exportTrackMetadataIntoMP4Tag(TagLib::MP4::Tag* pTag, const TrackMetadata& trackMetadata.getTrackInfo().getDiscTotal()); } - if (hasAlbumGain(trackMetadata)) { - writeMP4Atom(pTag, "----:com.apple.iTunes:replaygain_album_gain", - toTagLibString(formatAlbumGain(trackMetadata))); - } - if (hasAlbumPeak(trackMetadata)) { - writeMP4Atom(pTag, "----:com.apple.iTunes:replaygain_album_peak", - toTagLibString(formatAlbumPeak(trackMetadata))); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzArtistId().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Artist Id", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzArtistId().toString())); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzRecordingId().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Track Id", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzRecordingId().toString())); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzReleaseId().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Release Track Id", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzReleaseId().toString())); - } - if (!trackMetadata.getTrackInfo().getMusicBrainzWorkId().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Work Id", - toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzWorkId().toString())); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzArtistId().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Album Artist Id", - toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzArtistId().toString())); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzReleaseId().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Album Id", - toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseId().toString())); - } - if (!trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Release Group Id", - toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId().toString())); - } - if (!trackMetadata.getTrackInfo().getConductor().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:CONDUCTOR", - toTagLibString(trackMetadata.getTrackInfo().getConductor())); - } - if (!trackMetadata.getTrackInfo().getISRC().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:ISRC", - toTagLibString(trackMetadata.getTrackInfo().getISRC())); - } - if (!trackMetadata.getTrackInfo().getLanguage().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:LANGUAGE", - toTagLibString(trackMetadata.getTrackInfo().getLanguage())); - } - if (!trackMetadata.getTrackInfo().getLyricist().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:LYRICIST", - toTagLibString(trackMetadata.getTrackInfo().getLyricist())); - } - if (!trackMetadata.getTrackInfo().getMood().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:MOOD", - toTagLibString(trackMetadata.getTrackInfo().getMood())); - } - if (!trackMetadata.getAlbumInfo().getCopyright().isNull()) { - writeMP4Atom(pTag, "cprt", - toTagLibString(trackMetadata.getAlbumInfo().getCopyright())); - } - if (!trackMetadata.getAlbumInfo().getLicense().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:LICENSE", - toTagLibString(trackMetadata.getAlbumInfo().getLicense())); - } - if (!trackMetadata.getAlbumInfo().getRecordLabel().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:LABEL", - toTagLibString(trackMetadata.getAlbumInfo().getRecordLabel())); - } - if (!trackMetadata.getTrackInfo().getRemixer().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:REMIXER", - toTagLibString(trackMetadata.getTrackInfo().getRemixer())); - } - if (!trackMetadata.getTrackInfo().getSubtitle().isNull()) { - writeMP4Atom(pTag, "----:com.apple.iTunes:SUBTITLE", - toTagLibString(trackMetadata.getTrackInfo().getSubtitle())); - } - if (!trackMetadata.getTrackInfo().getEncoder().isNull()) { - writeMP4Atom(pTag, "\251too", - toTagLibString(trackMetadata.getTrackInfo().getEncoder())); - } - if (!trackMetadata.getTrackInfo().getWork().isNull()) { - writeMP4Atom(pTag, "\251wrk", toTagLibString(trackMetadata.getTrackInfo().getWork())); - } - if (!trackMetadata.getTrackInfo().getMovement().isNull()) { - writeMP4Atom(pTag, "\251mvn", toTagLibString(trackMetadata.getTrackInfo().getMovement())); - } + writeMP4Atom(pTag, "----:com.apple.iTunes:replaygain_album_gain", + toTagLibString(formatAlbumGain(trackMetadata))); + writeMP4Atom(pTag, "----:com.apple.iTunes:replaygain_album_peak", + toTagLibString(formatAlbumPeak(trackMetadata))); + + writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Artist Id", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzArtistId())); + writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Track Id", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzRecordingId())); + writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Release Track Id", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzReleaseId())); + writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Work Id", + toTagLibString(trackMetadata.getTrackInfo().getMusicBrainzWorkId())); + writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Album Artist Id", + toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzArtistId())); + writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Album Id", + toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseId())); + writeMP4Atom(pTag, "----:com.apple.iTunes:MusicBrainz Release Group Id", + toTagLibString(trackMetadata.getAlbumInfo().getMusicBrainzReleaseGroupId())); + + writeMP4Atom(pTag, "----:com.apple.iTunes:CONDUCTOR", + toTagLibString(trackMetadata.getTrackInfo().getConductor())); + writeMP4Atom(pTag, "----:com.apple.iTunes:ISRC", + toTagLibString(trackMetadata.getTrackInfo().getISRC())); + writeMP4Atom(pTag, "----:com.apple.iTunes:LANGUAGE", + toTagLibString(trackMetadata.getTrackInfo().getLanguage())); + writeMP4Atom(pTag, "----:com.apple.iTunes:LYRICIST", + toTagLibString(trackMetadata.getTrackInfo().getLyricist())); + writeMP4Atom(pTag, "----:com.apple.iTunes:MOOD", + toTagLibString(trackMetadata.getTrackInfo().getMood())); + writeMP4Atom(pTag, "cprt", + toTagLibString(trackMetadata.getAlbumInfo().getCopyright())); + writeMP4Atom(pTag, "----:com.apple.iTunes:LICENSE", + toTagLibString(trackMetadata.getAlbumInfo().getLicense())); + writeMP4Atom(pTag, "----:com.apple.iTunes:LABEL", + toTagLibString(trackMetadata.getAlbumInfo().getRecordLabel())); + writeMP4Atom(pTag, "----:com.apple.iTunes:REMIXER", + toTagLibString(trackMetadata.getTrackInfo().getRemixer())); + writeMP4Atom(pTag, "----:com.apple.iTunes:SUBTITLE", + toTagLibString(trackMetadata.getTrackInfo().getSubtitle())); + writeMP4Atom(pTag, "\251too", + toTagLibString(trackMetadata.getTrackInfo().getEncoder())); + writeMP4Atom(pTag, "\251wrk", + toTagLibString(trackMetadata.getTrackInfo().getWork())); + writeMP4Atom(pTag, "\251mvn", + toTagLibString(trackMetadata.getTrackInfo().getMovement())); #endif // __EXTRA_METADATA__ return true; diff --git a/src/track/trackrecord.cpp b/src/track/trackrecord.cpp index 26beff8d229..7e8320a9b15 100644 --- a/src/track/trackrecord.cpp +++ b/src/track/trackrecord.cpp @@ -47,4 +47,85 @@ bool TrackRecord::updateGlobalKeyText( return false; } +namespace { + +// Data migration: Reload track total from file tags if not initialized +// yet. The added column "tracktotal" has been initialized with the +// default value "//". +// See also: Schema revision 26 in schema.xml +const QString kReloadTrackTotal = "//"; + +void mergeReplayGainMetadataProperty( + ReplayGain& mergedReplayGain, + const ReplayGain& importedReplayGain) { + // Preserve the values calculated by Mixxx and only merge missing + // values from the imported replay gain. + if (!mergedReplayGain.hasRatio()) { + mergedReplayGain.setRatio(importedReplayGain.getRatio()); + } + if (!mergedReplayGain.hasPeak()) { + mergedReplayGain.setPeak(importedReplayGain.getPeak()); + } +} + +template +void mergeNullableMetadataProperty( + T& mergedProperty, + const T& importedProperty) { + if (mergedProperty.isNull()) { + mergedProperty = importedProperty; + } +} + +} // anonymous namespace + +void TrackRecord::mergeImportedMetadata( + const TrackMetadata& importedFromFile) { + TrackInfo& mergedTrackInfo = refMetadata().refTrackInfo(); + const TrackInfo& importedTrackInfo = importedFromFile.getTrackInfo(); + if (mergedTrackInfo.getTrackTotal() == kReloadTrackTotal) { + mergedTrackInfo.setTrackTotal(importedTrackInfo.getTrackTotal()); + // Also set the track number if it is still empty due + // to insufficient parsing capabilities of Mixxx in + // previous versions. + if (mergedTrackInfo.getTrackNumber().isEmpty() && + !importedTrackInfo.getTrackNumber().isEmpty()) { + mergedTrackInfo.setTrackNumber(importedTrackInfo.getTrackNumber()); + } + } +#if defined(__EXTRA_METADATA__) + mergeNullableMetadataProperty(mergedTrackInfo.refConductor(), importedTrackInfo.getConductor()); + mergeNullableMetadataProperty(mergedTrackInfo.refDiscNumber(), importedTrackInfo.getDiscNumber()); + mergeNullableMetadataProperty(mergedTrackInfo.refDiscTotal(), importedTrackInfo.getDiscTotal()); + mergeNullableMetadataProperty(mergedTrackInfo.refEncoder(), importedTrackInfo.getEncoder()); + mergeNullableMetadataProperty(mergedTrackInfo.refEncoderSettings(), importedTrackInfo.getEncoderSettings()); + mergeNullableMetadataProperty(mergedTrackInfo.refISRC(), importedTrackInfo.getISRC()); + mergeNullableMetadataProperty(mergedTrackInfo.refLanguage(), importedTrackInfo.getLanguage()); + mergeNullableMetadataProperty(mergedTrackInfo.refLyricist(), importedTrackInfo.getLyricist()); + mergeNullableMetadataProperty(mergedTrackInfo.refMood(), importedTrackInfo.getMood()); + mergeNullableMetadataProperty(mergedTrackInfo.refMovement(), importedTrackInfo.getMovement()); + mergeNullableMetadataProperty(mergedTrackInfo.refMusicBrainzArtistId(), importedTrackInfo.getMusicBrainzArtistId()); + mergeNullableMetadataProperty(mergedTrackInfo.refMusicBrainzRecordingId(), importedTrackInfo.getMusicBrainzRecordingId()); + mergeNullableMetadataProperty(mergedTrackInfo.refMusicBrainzReleaseId(), importedTrackInfo.getMusicBrainzReleaseId()); + mergeNullableMetadataProperty(mergedTrackInfo.refMusicBrainzWorkId(), importedTrackInfo.getMusicBrainzWorkId()); + mergeNullableMetadataProperty(mergedTrackInfo.refRemixer(), importedTrackInfo.getRemixer()); + mergeNullableMetadataProperty(mergedTrackInfo.refSubtitle(), importedTrackInfo.getSubtitle()); + mergeNullableMetadataProperty(mergedTrackInfo.refWork(), importedTrackInfo.getWork()); +#endif // __EXTRA_METADATA__ + AlbumInfo& mergedAlbumInfo = refMetadata().refAlbumInfo(); + const AlbumInfo& importedAlbumInfo = importedFromFile.getAlbumInfo(); +#if defined(__EXTRA_METADATA__) + mergeReplayGainMetadataProperty(mergedAlbumInfo.refReplayGain(), importedAlbumInfo.getReplayGain()); + mergeNullableMetadataProperty(mergedAlbumInfo.refCopyright(), importedAlbumInfo.getCopyright()); + mergeNullableMetadataProperty(mergedAlbumInfo.refLicense(), importedAlbumInfo.getLicense()); + mergeNullableMetadataProperty(mergedAlbumInfo.refMusicBrainzArtistId(), importedAlbumInfo.getMusicBrainzArtistId()); + mergeNullableMetadataProperty(mergedAlbumInfo.refMusicBrainzReleaseGroupId(), importedAlbumInfo.getMusicBrainzReleaseGroupId()); + mergeNullableMetadataProperty(mergedAlbumInfo.refMusicBrainzReleaseId(), importedAlbumInfo.getMusicBrainzReleaseId()); + mergeNullableMetadataProperty(mergedAlbumInfo.refRecordLabel(), importedAlbumInfo.getRecordLabel()); +#else + Q_UNUSED(mergedAlbumInfo); + Q_UNUSED(importedAlbumInfo); +#endif // __EXTRA_METADATA__ +} + } //namespace mixxx diff --git a/src/track/trackrecord.h b/src/track/trackrecord.h index 54d9d5f25c0..5175a841e00 100644 --- a/src/track/trackrecord.h +++ b/src/track/trackrecord.h @@ -87,6 +87,12 @@ class TrackRecord final { const QString& keyText, track::io::key::Source keySource); + // Merge the current metadata from the file with the subset of metadata + // that is stored in the library. This is required to delete any tags + // from the file that are not (yet) stored in the library database! + void mergeImportedMetadata( + const TrackMetadata& importedMetadata); + private: Keys m_keys; }; From 9777219d1f30333fced56cbd8abf0372dcfa920a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 22 Dec 2019 23:57:44 +0100 Subject: [PATCH 02/17] Use QStringLiteral --- src/sources/metadatasourcetaglib.cpp | 4 ++-- src/track/trackinfo.cpp | 4 ++-- src/track/trackmetadatataglib.cpp | 2 +- src/track/trackrecord.cpp | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/sources/metadatasourcetaglib.cpp b/src/sources/metadatasourcetaglib.cpp index e70e9eb0ffc..1038c6e729d 100644 --- a/src/sources/metadatasourcetaglib.cpp +++ b/src/sources/metadatasourcetaglib.cpp @@ -26,12 +26,12 @@ Logger kLogger("MetadataSourceTagLib"); const bool kExportTrackMetadataIntoTemporaryFile = true; // Appended to the original file name of the temporary file used for writing -const QString kSafelyWritableTempFileSuffix = "_temp"; +const QString kSafelyWritableTempFileSuffix = QStringLiteral("_temp"); // Appended to the original file name for renaming and before deleting this // file. Should not be longer than kSafelyWritableTempFileSuffix to avoid // potential failures caused by exceeded path length. -const QString kSafelyWritableOrigFileSuffix = "_orig"; +const QString kSafelyWritableOrigFileSuffix = QStringLiteral("_orig"); // Workaround for missing functionality in TagLib 1.11.x that // doesn't support to read text chunks from AIFF files. diff --git a/src/track/trackinfo.cpp b/src/track/trackinfo.cpp index 93baf8867ac..1950f4ffe6c 100644 --- a/src/track/trackinfo.cpp +++ b/src/track/trackinfo.cpp @@ -5,8 +5,8 @@ namespace mixxx { namespace { -const QString kArtistTitleSeparatorWithSpaces = " - "; -const QString kArtistTitleSeparator = "_-_"; +const QString kArtistTitleSeparatorWithSpaces = QStringLiteral(" - "); +const QString kArtistTitleSeparator = QStringLiteral("_-_"); const QChar kFileExtensionSeparator = '.'; diff --git a/src/track/trackmetadatataglib.cpp b/src/track/trackmetadatataglib.cpp index 37f2f6c632f..c505adcbb28 100644 --- a/src/track/trackmetadatataglib.cpp +++ b/src/track/trackmetadatataglib.cpp @@ -71,7 +71,7 @@ bool checkID3v2HeaderVersionSupported(const TagLib::ID3v2::Header& header) { // as written by MusicBrainz Picard 2.1.3 although the mapping table // doesn't mention any "http://" prefix. // See also: https://picard.musicbrainz.org/docs/mappings -const QString kMusicBrainzOwner = "http://musicbrainz.org"; +const QString kMusicBrainzOwner = QStringLiteral("http://musicbrainz.org"); } // anonymous namespace diff --git a/src/track/trackrecord.cpp b/src/track/trackrecord.cpp index 7e8320a9b15..bb3fadedd38 100644 --- a/src/track/trackrecord.cpp +++ b/src/track/trackrecord.cpp @@ -53,7 +53,7 @@ namespace { // yet. The added column "tracktotal" has been initialized with the // default value "//". // See also: Schema revision 26 in schema.xml -const QString kReloadTrackTotal = "//"; +const QString kReloadTrackTotal = QStringLiteral("//"); void mergeReplayGainMetadataProperty( ReplayGain& mergedReplayGain, From 2450e162664928a59875268d6da706c52c591009 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 22 Dec 2019 23:58:56 +0100 Subject: [PATCH 03/17] Fix wrong comment --- src/track/trackrecord.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/track/trackrecord.h b/src/track/trackrecord.h index 5175a841e00..294d1c41850 100644 --- a/src/track/trackrecord.h +++ b/src/track/trackrecord.h @@ -88,7 +88,7 @@ class TrackRecord final { track::io::key::Source keySource); // Merge the current metadata from the file with the subset of metadata - // that is stored in the library. This is required to delete any tags + // that is stored in the library. This is required to NOT delete any tags // from the file that are not (yet) stored in the library database! void mergeImportedMetadata( const TrackMetadata& importedMetadata); From 37b5e395d986747a8cb279b3142bada68d09a8f4 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 23 Dec 2019 00:14:06 +0100 Subject: [PATCH 04/17] Join conditional compilation regions --- src/track/trackrecord.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/track/trackrecord.cpp b/src/track/trackrecord.cpp index bb3fadedd38..84230337494 100644 --- a/src/track/trackrecord.cpp +++ b/src/track/trackrecord.cpp @@ -111,10 +111,8 @@ void TrackRecord::mergeImportedMetadata( mergeNullableMetadataProperty(mergedTrackInfo.refRemixer(), importedTrackInfo.getRemixer()); mergeNullableMetadataProperty(mergedTrackInfo.refSubtitle(), importedTrackInfo.getSubtitle()); mergeNullableMetadataProperty(mergedTrackInfo.refWork(), importedTrackInfo.getWork()); -#endif // __EXTRA_METADATA__ AlbumInfo& mergedAlbumInfo = refMetadata().refAlbumInfo(); const AlbumInfo& importedAlbumInfo = importedFromFile.getAlbumInfo(); -#if defined(__EXTRA_METADATA__) mergeReplayGainMetadataProperty(mergedAlbumInfo.refReplayGain(), importedAlbumInfo.getReplayGain()); mergeNullableMetadataProperty(mergedAlbumInfo.refCopyright(), importedAlbumInfo.getCopyright()); mergeNullableMetadataProperty(mergedAlbumInfo.refLicense(), importedAlbumInfo.getLicense()); @@ -122,9 +120,6 @@ void TrackRecord::mergeImportedMetadata( mergeNullableMetadataProperty(mergedAlbumInfo.refMusicBrainzReleaseGroupId(), importedAlbumInfo.getMusicBrainzReleaseGroupId()); mergeNullableMetadataProperty(mergedAlbumInfo.refMusicBrainzReleaseId(), importedAlbumInfo.getMusicBrainzReleaseId()); mergeNullableMetadataProperty(mergedAlbumInfo.refRecordLabel(), importedAlbumInfo.getRecordLabel()); -#else - Q_UNUSED(mergedAlbumInfo); - Q_UNUSED(importedAlbumInfo); #endif // __EXTRA_METADATA__ } From eb1cb3ed90b8d1992067af2da2bf6151871ca989 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 23 Dec 2019 00:19:26 +0100 Subject: [PATCH 05/17] Rename conditional copy operation --- src/track/trackrecord.cpp | 50 ++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/src/track/trackrecord.cpp b/src/track/trackrecord.cpp index 84230337494..e50247e9cf4 100644 --- a/src/track/trackrecord.cpp +++ b/src/track/trackrecord.cpp @@ -68,8 +68,10 @@ void mergeReplayGainMetadataProperty( } } +// This conditional copy operation only works for nullable properties +// like QString or QUuid. template -void mergeNullableMetadataProperty( +void copyIfNotNull( T& mergedProperty, const T& importedProperty) { if (mergedProperty.isNull()) { @@ -94,32 +96,32 @@ void TrackRecord::mergeImportedMetadata( } } #if defined(__EXTRA_METADATA__) - mergeNullableMetadataProperty(mergedTrackInfo.refConductor(), importedTrackInfo.getConductor()); - mergeNullableMetadataProperty(mergedTrackInfo.refDiscNumber(), importedTrackInfo.getDiscNumber()); - mergeNullableMetadataProperty(mergedTrackInfo.refDiscTotal(), importedTrackInfo.getDiscTotal()); - mergeNullableMetadataProperty(mergedTrackInfo.refEncoder(), importedTrackInfo.getEncoder()); - mergeNullableMetadataProperty(mergedTrackInfo.refEncoderSettings(), importedTrackInfo.getEncoderSettings()); - mergeNullableMetadataProperty(mergedTrackInfo.refISRC(), importedTrackInfo.getISRC()); - mergeNullableMetadataProperty(mergedTrackInfo.refLanguage(), importedTrackInfo.getLanguage()); - mergeNullableMetadataProperty(mergedTrackInfo.refLyricist(), importedTrackInfo.getLyricist()); - mergeNullableMetadataProperty(mergedTrackInfo.refMood(), importedTrackInfo.getMood()); - mergeNullableMetadataProperty(mergedTrackInfo.refMovement(), importedTrackInfo.getMovement()); - mergeNullableMetadataProperty(mergedTrackInfo.refMusicBrainzArtistId(), importedTrackInfo.getMusicBrainzArtistId()); - mergeNullableMetadataProperty(mergedTrackInfo.refMusicBrainzRecordingId(), importedTrackInfo.getMusicBrainzRecordingId()); - mergeNullableMetadataProperty(mergedTrackInfo.refMusicBrainzReleaseId(), importedTrackInfo.getMusicBrainzReleaseId()); - mergeNullableMetadataProperty(mergedTrackInfo.refMusicBrainzWorkId(), importedTrackInfo.getMusicBrainzWorkId()); - mergeNullableMetadataProperty(mergedTrackInfo.refRemixer(), importedTrackInfo.getRemixer()); - mergeNullableMetadataProperty(mergedTrackInfo.refSubtitle(), importedTrackInfo.getSubtitle()); - mergeNullableMetadataProperty(mergedTrackInfo.refWork(), importedTrackInfo.getWork()); + copyIfNotNull(mergedTrackInfo.refConductor(), importedTrackInfo.getConductor()); + copyIfNotNull(mergedTrackInfo.refDiscNumber(), importedTrackInfo.getDiscNumber()); + copyIfNotNull(mergedTrackInfo.refDiscTotal(), importedTrackInfo.getDiscTotal()); + copyIfNotNull(mergedTrackInfo.refEncoder(), importedTrackInfo.getEncoder()); + copyIfNotNull(mergedTrackInfo.refEncoderSettings(), importedTrackInfo.getEncoderSettings()); + copyIfNotNull(mergedTrackInfo.refISRC(), importedTrackInfo.getISRC()); + copyIfNotNull(mergedTrackInfo.refLanguage(), importedTrackInfo.getLanguage()); + copyIfNotNull(mergedTrackInfo.refLyricist(), importedTrackInfo.getLyricist()); + copyIfNotNull(mergedTrackInfo.refMood(), importedTrackInfo.getMood()); + copyIfNotNull(mergedTrackInfo.refMovement(), importedTrackInfo.getMovement()); + copyIfNotNull(mergedTrackInfo.refMusicBrainzArtistId(), importedTrackInfo.getMusicBrainzArtistId()); + copyIfNotNull(mergedTrackInfo.refMusicBrainzRecordingId(), importedTrackInfo.getMusicBrainzRecordingId()); + copyIfNotNull(mergedTrackInfo.refMusicBrainzReleaseId(), importedTrackInfo.getMusicBrainzReleaseId()); + copyIfNotNull(mergedTrackInfo.refMusicBrainzWorkId(), importedTrackInfo.getMusicBrainzWorkId()); + copyIfNotNull(mergedTrackInfo.refRemixer(), importedTrackInfo.getRemixer()); + copyIfNotNull(mergedTrackInfo.refSubtitle(), importedTrackInfo.getSubtitle()); + copyIfNotNull(mergedTrackInfo.refWork(), importedTrackInfo.getWork()); AlbumInfo& mergedAlbumInfo = refMetadata().refAlbumInfo(); const AlbumInfo& importedAlbumInfo = importedFromFile.getAlbumInfo(); mergeReplayGainMetadataProperty(mergedAlbumInfo.refReplayGain(), importedAlbumInfo.getReplayGain()); - mergeNullableMetadataProperty(mergedAlbumInfo.refCopyright(), importedAlbumInfo.getCopyright()); - mergeNullableMetadataProperty(mergedAlbumInfo.refLicense(), importedAlbumInfo.getLicense()); - mergeNullableMetadataProperty(mergedAlbumInfo.refMusicBrainzArtistId(), importedAlbumInfo.getMusicBrainzArtistId()); - mergeNullableMetadataProperty(mergedAlbumInfo.refMusicBrainzReleaseGroupId(), importedAlbumInfo.getMusicBrainzReleaseGroupId()); - mergeNullableMetadataProperty(mergedAlbumInfo.refMusicBrainzReleaseId(), importedAlbumInfo.getMusicBrainzReleaseId()); - mergeNullableMetadataProperty(mergedAlbumInfo.refRecordLabel(), importedAlbumInfo.getRecordLabel()); + copyIfNotNull(mergedAlbumInfo.refCopyright(), importedAlbumInfo.getCopyright()); + copyIfNotNull(mergedAlbumInfo.refLicense(), importedAlbumInfo.getLicense()); + copyIfNotNull(mergedAlbumInfo.refMusicBrainzArtistId(), importedAlbumInfo.getMusicBrainzArtistId()); + copyIfNotNull(mergedAlbumInfo.refMusicBrainzReleaseGroupId(), importedAlbumInfo.getMusicBrainzReleaseGroupId()); + copyIfNotNull(mergedAlbumInfo.refMusicBrainzReleaseId(), importedAlbumInfo.getMusicBrainzReleaseId()); + copyIfNotNull(mergedAlbumInfo.refRecordLabel(), importedAlbumInfo.getRecordLabel()); #endif // __EXTRA_METADATA__ } From 86e5382d6c16ea84ad1f0a6bde7052c15e61ec68 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 23 Dec 2019 00:32:20 +0100 Subject: [PATCH 06/17] Add missing merge operation for SeratoMarkers2 --- src/track/seratomarkers2.h | 4 ++++ src/track/trackrecord.cpp | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/track/seratomarkers2.h b/src/track/seratomarkers2.h index 256284c9c47..4081785827c 100644 --- a/src/track/seratomarkers2.h +++ b/src/track/seratomarkers2.h @@ -387,6 +387,10 @@ class SeratoMarkers2 final { m_allocatedSize = size; } + bool isEmpty() const { + return m_entries.isEmpty(); + } + const QList>& getEntries() const { return m_entries; } diff --git a/src/track/trackrecord.cpp b/src/track/trackrecord.cpp index e50247e9cf4..c9eb3bd8a37 100644 --- a/src/track/trackrecord.cpp +++ b/src/track/trackrecord.cpp @@ -79,6 +79,17 @@ void copyIfNotNull( } } +// This conditional copy operation only works for properties where +// empty = missing. +template +void copyIfNotEmpty( + T& mergedProperty, + const T& importedProperty) { + if (mergedProperty.isEmpty()) { + mergedProperty = importedProperty; + } +} + } // anonymous namespace void TrackRecord::mergeImportedMetadata( @@ -111,6 +122,7 @@ void TrackRecord::mergeImportedMetadata( copyIfNotNull(mergedTrackInfo.refMusicBrainzReleaseId(), importedTrackInfo.getMusicBrainzReleaseId()); copyIfNotNull(mergedTrackInfo.refMusicBrainzWorkId(), importedTrackInfo.getMusicBrainzWorkId()); copyIfNotNull(mergedTrackInfo.refRemixer(), importedTrackInfo.getRemixer()); + copyIfNotEmpty(mergedTrackInfo.refSeratoMarkers2(), importedTrackInfo.getSeratoMarkers2()); copyIfNotNull(mergedTrackInfo.refSubtitle(), importedTrackInfo.getSubtitle()); copyIfNotNull(mergedTrackInfo.refWork(), importedTrackInfo.getWork()); AlbumInfo& mergedAlbumInfo = refMetadata().refAlbumInfo(); From 1ef3b40d14854c0aa7746c2253ebd59093a5a8a7 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 23 Dec 2019 00:32:49 +0100 Subject: [PATCH 07/17] Write "Serato Markers2" ID3v2 frame --- src/track/trackmetadatataglib.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/track/trackmetadatataglib.cpp b/src/track/trackmetadatataglib.cpp index c505adcbb28..2925d14693d 100644 --- a/src/track/trackmetadatataglib.cpp +++ b/src/track/trackmetadatataglib.cpp @@ -2508,6 +2508,10 @@ bool exportTrackMetadataIntoID3v2Tag(TagLib::ID3v2::Tag* pTag, pTag, "TSSE", trackMetadata.getTrackInfo().getEncoderSettings()); + writeID3v2GeneralEncapsulatedObjectFrame( + pTag, + "Serato Markers2", + trackMetadata.getTrackInfo().getSeratoMarkers2().data()); #endif // __EXTRA_METADATA__ return true; From 83a9c32f29363152e91c8705f2fb9504f61a049c Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 31 Dec 2019 10:32:45 +0100 Subject: [PATCH 08/17] Test merging of imported track metadata - Preserve existing properties - Replace missing extra properties --- src/test/trackmetadata_test.cpp | 166 +++++++++++++++++++++++++++++++- src/track/trackrecord.cpp | 10 +- src/track/trackrecord.h | 7 ++ 3 files changed, 175 insertions(+), 8 deletions(-) diff --git a/src/test/trackmetadata_test.cpp b/src/test/trackmetadata_test.cpp index 0bdd95891bc..62115a259f4 100644 --- a/src/test/trackmetadata_test.cpp +++ b/src/test/trackmetadata_test.cpp @@ -1,6 +1,6 @@ #include -#include "track/trackmetadata.h" +#include "track/trackrecord.h" class TrackMetadataTest : public testing::Test { }; @@ -61,3 +61,167 @@ TEST_F(TrackMetadataTest, parseArtistTitleFromFileName) { EXPECT_EQ("again - only_-_title _", trackInfo.getTitle()); } } + +TEST_F(TrackMetadataTest, mergeImportedMetadata) { + // Existing track metadata (stored in the database) without extra properties + mixxx::TrackRecord oldTrackRecord; + mixxx::TrackMetadata& oldTrackMetadata = oldTrackRecord.refMetadata(); + oldTrackMetadata.setBitrate(mixxx::AudioSource::Bitrate(100)); + oldTrackMetadata.setChannels(mixxx::AudioSignal::ChannelCount(1)); + oldTrackMetadata.setDuration(mixxx::Duration::fromSeconds(60)); + oldTrackMetadata.setSampleRate(mixxx::AudioSignal::SampleRate(10000)); + mixxx::TrackInfo& oldTrackInfo = oldTrackMetadata.refTrackInfo(); + oldTrackInfo.setArtist("old artist"); + oldTrackInfo.setBpm(mixxx::Bpm(100)); + oldTrackInfo.setComment("old comment"); + oldTrackInfo.setComposer("old composer"); + oldTrackInfo.setGenre("old genre"); + oldTrackInfo.setGrouping("old grouping"); + oldTrackInfo.setKey("1A"); + oldTrackInfo.setReplayGain(mixxx::ReplayGain(0.1, 1)); + oldTrackInfo.setTitle("old title"); + oldTrackInfo.setTrackNumber("1"); + oldTrackInfo.setTrackTotal("10"); + oldTrackInfo.setYear("2001-01-01"); + mixxx::AlbumInfo& oldAlbumInfo = oldTrackMetadata.refAlbumInfo(); + oldAlbumInfo.setArtist("old album artist"); + oldAlbumInfo.setTitle("old album title"); + + // Imported track metadata (from file tags) with extra properties + mixxx::TrackMetadata newTrackMetadata; + newTrackMetadata.setBitrate(mixxx::AudioSource::Bitrate(200)); + newTrackMetadata.setChannels(mixxx::AudioSignal::ChannelCount(2)); + newTrackMetadata.setDuration(mixxx::Duration::fromSeconds(120)); + newTrackMetadata.setSampleRate(mixxx::AudioSignal::SampleRate(20000)); + mixxx::TrackInfo& newTrackInfo = newTrackMetadata.refTrackInfo(); + newTrackInfo.setArtist("new artist"); + newTrackInfo.setBpm(mixxx::Bpm(200)); + newTrackInfo.setComment("new comment"); + newTrackInfo.setComposer("new composer"); +#if defined(__EXTRA_METADATA__) + newTrackInfo.setConductor("new conductor"); + newTrackInfo.setDiscNumber("1"); + newTrackInfo.setDiscTotal("2"); + newTrackInfo.setEncoder("encoder"); + newTrackInfo.setEncoderSettings("encoder settings"); +#endif // __EXTRA_METADATA__ + newTrackInfo.setGenre("new genre"); + newTrackInfo.setGrouping("new grouping"); +#if defined(__EXTRA_METADATA__) + newTrackInfo.setISRC("isrc"); +#endif // __EXTRA_METADATA__ + newTrackInfo.setKey("1A"); +#if defined(__EXTRA_METADATA__) + newTrackInfo.setLanguage("language"); + newTrackInfo.setLyricist("lyricist"); + newTrackInfo.setMood("mood"); + newTrackInfo.setMovement("movement"); + newTrackInfo.setMusicBrainzArtistId(QUuid("11111111-1111-1111-1111-111111111111")); + newTrackInfo.setMusicBrainzRecordingId(QUuid("22222222-2222-2222-2222-222222222222")); + newTrackInfo.setMusicBrainzReleaseId(QUuid("33333333-3333-3333-3333-333333333333")); + newTrackInfo.setMusicBrainzWorkId(QUuid("44444444-4444-4444-4444-444444444444")); + newTrackInfo.setRemixer("remixer"); +#endif // __EXTRA_METADATA__ + newTrackInfo.setReplayGain(mixxx::ReplayGain(0.2, 2)); + newTrackInfo.setTitle("new title"); + newTrackInfo.setTrackNumber("2"); + newTrackInfo.setTrackTotal("20"); +#if defined(__EXTRA_METADATA__) + newTrackInfo.setWork("work"); +#endif // __EXTRA_METADATA__ + newTrackInfo.setYear("2002-02-02"); + mixxx::AlbumInfo& newAlbumInfo = newTrackMetadata.refAlbumInfo(); + newAlbumInfo.setArtist("new album artist"); +#if defined(__EXTRA_METADATA__) + newAlbumInfo.setCopyright("copyright"); + newAlbumInfo.setLicense("license"); + newAlbumInfo.setMusicBrainzArtistId(QUuid("55555555-5555-5555-5555-555555555555")); + newAlbumInfo.setMusicBrainzReleaseGroupId(QUuid("66666666-6666-6666-6666-666666666666")); + newAlbumInfo.setMusicBrainzReleaseId(QUuid("77777777-7777-7777-7777-777777777777")); + newAlbumInfo.setRecordLabel("copyright"); + newAlbumInfo.setReplayGain(mixxx::ReplayGain(0.3, 3)); +#endif // __EXTRA_METADATA__ + newAlbumInfo.setTitle("new album title"); + + mixxx::TrackRecord mergedTrackRecord = oldTrackRecord; + ASSERT_EQ(mergedTrackRecord.getMetadata(), oldTrackRecord.getMetadata()); + ASSERT_NE(newTrackMetadata, oldTrackMetadata); + mergedTrackRecord.mergeImportedMetadata(newTrackMetadata); + + mixxx::TrackMetadata& mergedTrackMetadata = mergedTrackRecord.refMetadata(); + EXPECT_EQ(oldTrackMetadata.getBitrate(), mergedTrackMetadata.getBitrate()); + EXPECT_EQ(oldTrackMetadata.getChannels(), mergedTrackMetadata.getChannels()); + EXPECT_EQ(oldTrackMetadata.getDuration(), mergedTrackMetadata.getDuration()); + EXPECT_EQ(oldTrackMetadata.getSampleRate(), mergedTrackMetadata.getSampleRate()); + mixxx::TrackInfo& mergedTrackInfo = mergedTrackMetadata.refTrackInfo(); + EXPECT_EQ(oldTrackInfo.getArtist(), mergedTrackInfo.getArtist()); + EXPECT_EQ(oldTrackInfo.getBpm(), mergedTrackInfo.getBpm()); + EXPECT_EQ(oldTrackInfo.getComment(), mergedTrackInfo.getComment()); + EXPECT_EQ(oldTrackInfo.getComposer(), mergedTrackInfo.getComposer()); +#if defined(__EXTRA_METADATA__) + EXPECT_EQ(newTrackInfo.getConductor(), mergedTrackInfo.getConductor()); + EXPECT_EQ(newTrackInfo.getDiscNumber(), mergedTrackInfo.getDiscNumber()); + EXPECT_EQ(newTrackInfo.getDiscTotal(), mergedTrackInfo.getDiscTotal()); + EXPECT_EQ(newTrackInfo.getEncoder(), mergedTrackInfo.getEncoder()); + EXPECT_EQ(newTrackInfo.getEncoderSettings(), mergedTrackInfo.getEncoderSettings()); +#endif // __EXTRA_METADATA__ + EXPECT_EQ(oldTrackInfo.getGenre(), mergedTrackInfo.getGenre()); + EXPECT_EQ(oldTrackInfo.getGrouping(), mergedTrackInfo.getGrouping()); +#if defined(__EXTRA_METADATA__) + EXPECT_EQ(newTrackInfo.getISRC(), mergedTrackInfo.getISRC()); +#endif // __EXTRA_METADATA__ + EXPECT_EQ(oldTrackInfo.getKey(), mergedTrackInfo.getKey()); +#if defined(__EXTRA_METADATA__) + EXPECT_EQ(newTrackInfo.getLanguage(), mergedTrackInfo.getLanguage()); + EXPECT_EQ(newTrackInfo.getLyricist(), mergedTrackInfo.getLyricist()); + EXPECT_EQ(newTrackInfo.getMood(), mergedTrackInfo.getMood()); + EXPECT_EQ(newTrackInfo.getMovement(), mergedTrackInfo.getMovement()); + EXPECT_EQ(newTrackInfo.getMusicBrainzArtistId(), mergedTrackInfo.getMusicBrainzArtistId()); + EXPECT_EQ(newTrackInfo.getMusicBrainzRecordingId(), mergedTrackInfo.getMusicBrainzRecordingId()); + EXPECT_EQ(newTrackInfo.getMusicBrainzReleaseId(), mergedTrackInfo.getMusicBrainzReleaseId()); + EXPECT_EQ(newTrackInfo.getMusicBrainzWorkId(), mergedTrackInfo.getMusicBrainzWorkId()); + EXPECT_EQ(newTrackInfo.getRemixer(), mergedTrackInfo.getRemixer()); +#endif // __EXTRA_METADATA__ + EXPECT_EQ(oldTrackInfo.getReplayGain(), mergedTrackInfo.getReplayGain()); + EXPECT_EQ(oldTrackInfo.getTitle(), mergedTrackInfo.getTitle()); + EXPECT_EQ(oldTrackInfo.getTrackNumber(), mergedTrackInfo.getTrackNumber()); + EXPECT_EQ(oldTrackInfo.getTrackTotal(), mergedTrackInfo.getTrackTotal()); +#if defined(__EXTRA_METADATA__) + EXPECT_EQ(newTrackInfo.getWork(), mergedTrackInfo.getWork()); +#endif // __EXTRA_METADATA__ + EXPECT_EQ(oldTrackInfo.getYear(), mergedTrackInfo.getYear()); + mixxx::AlbumInfo& mergedAlbumInfo = mergedTrackMetadata.refAlbumInfo(); + EXPECT_EQ(oldAlbumInfo.getArtist(), mergedAlbumInfo.getArtist()); +#if defined(__EXTRA_METADATA__) + EXPECT_EQ(newAlbumInfo.getCopyright(), mergedAlbumInfo.getCopyright()); + EXPECT_EQ(newAlbumInfo.getLicense(), mergedAlbumInfo.getLicense()); + EXPECT_EQ(newAlbumInfo.getMusicBrainzArtistId(), mergedAlbumInfo.getMusicBrainzArtistId()); + EXPECT_EQ(newAlbumInfo.getMusicBrainzReleaseGroupId(), mergedAlbumInfo.getMusicBrainzReleaseGroupId()); + EXPECT_EQ(newAlbumInfo.getMusicBrainzReleaseId(), mergedAlbumInfo.getMusicBrainzReleaseId()); + EXPECT_EQ(newAlbumInfo.getRecordLabel(), mergedAlbumInfo.getRecordLabel()); + EXPECT_EQ(newAlbumInfo.getReplayGain(), mergedAlbumInfo.getReplayGain()); +#endif // __EXTRA_METADATA__ + EXPECT_EQ(oldAlbumInfo.getTitle(), mergedAlbumInfo.getTitle()); + + // Check that all existing properties are preserved, even if empty or missing + mergedTrackInfo.setArtist(""); + mergedTrackInfo.setTitle(QString()); + mergedAlbumInfo.setArtist(""); + mergedAlbumInfo.setTitle(QString()); + mergedTrackRecord.mergeImportedMetadata(newTrackMetadata); + EXPECT_EQ(QString(""), mergedTrackInfo.getArtist()); + EXPECT_EQ(QString(), mergedTrackInfo.getTitle()); + EXPECT_EQ(QString(""), mergedAlbumInfo.getArtist()); + EXPECT_EQ(QString(), mergedAlbumInfo.getTitle()); + + // Check that the placeholder for track total is replaced with the actual property + ASSERT_NE(mixxx::TrackRecord::kTrackTotalPlaceholder, newTrackInfo.getTrackTotal()); + mergedTrackInfo.setTrackTotal(mixxx::TrackRecord::kTrackTotalPlaceholder); + mergedTrackRecord.mergeImportedMetadata(newTrackMetadata); + EXPECT_EQ(newTrackInfo.getTrackTotal(), mergedTrackInfo.getTrackTotal()); + // ...but if track total is missing entirely it should be preserved + ASSERT_NE(QString(), newTrackInfo.getTrackTotal()); + mergedTrackInfo.setTrackTotal(QString()); + mergedTrackRecord.mergeImportedMetadata(newTrackMetadata); + EXPECT_EQ(QString(), mergedTrackInfo.getTrackTotal()); +} diff --git a/src/track/trackrecord.cpp b/src/track/trackrecord.cpp index c9eb3bd8a37..effb1e4e0c2 100644 --- a/src/track/trackrecord.cpp +++ b/src/track/trackrecord.cpp @@ -5,6 +5,8 @@ namespace mixxx { +/*static*/ const QString TrackRecord::kTrackTotalPlaceholder = QStringLiteral("//"); + TrackRecord::TrackRecord(TrackId id) : m_id(std::move(id)), m_metadataSynchronized(false), @@ -49,12 +51,6 @@ bool TrackRecord::updateGlobalKeyText( namespace { -// Data migration: Reload track total from file tags if not initialized -// yet. The added column "tracktotal" has been initialized with the -// default value "//". -// See also: Schema revision 26 in schema.xml -const QString kReloadTrackTotal = QStringLiteral("//"); - void mergeReplayGainMetadataProperty( ReplayGain& mergedReplayGain, const ReplayGain& importedReplayGain) { @@ -96,7 +92,7 @@ void TrackRecord::mergeImportedMetadata( const TrackMetadata& importedFromFile) { TrackInfo& mergedTrackInfo = refMetadata().refTrackInfo(); const TrackInfo& importedTrackInfo = importedFromFile.getTrackInfo(); - if (mergedTrackInfo.getTrackTotal() == kReloadTrackTotal) { + if (mergedTrackInfo.getTrackTotal() == kTrackTotalPlaceholder) { mergedTrackInfo.setTrackTotal(importedTrackInfo.getTrackTotal()); // Also set the track number if it is still empty due // to insufficient parsing capabilities of Mixxx in diff --git a/src/track/trackrecord.h b/src/track/trackrecord.h index 294d1c41850..cdcc5dfcfb2 100644 --- a/src/track/trackrecord.h +++ b/src/track/trackrecord.h @@ -49,6 +49,13 @@ class TrackRecord final { PROPERTY_SET_BYVAL_GET_BYREF(bool, bpmLocked, BpmLocked) public: + // Data migration: Reload track total from file tags if not initialized + // yet. The added column "tracktotal" has been initialized with the + // default value "//". + // See also: Schema revision 26 in schema.xml + // Public only for testing purposes! + static const QString kTrackTotalPlaceholder; + explicit TrackRecord(TrackId id = TrackId()); TrackRecord(TrackRecord&&) = default; TrackRecord(const TrackRecord&) = default; From 7f371ecdcef569ea8fbb951eb2b4e622417f7746 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 1 Jan 2020 11:56:58 +0100 Subject: [PATCH 09/17] Delete undefined member functions --- src/track/track.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/track/track.h b/src/track/track.h index 87a9635fc6c..99d0e1c5d54 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -240,9 +240,6 @@ class Track : public QObject { ConstWaveformPointer getWaveformSummary() const; void setWaveformSummary(ConstWaveformPointer pWaveform); - void setAnalyzerProgress(int progress); - int getAnalyzerProgress() const; - // Get the track's main cue point CuePosition getCuePoint() const; // Set the track's main cue point From 519a857277adbc3b407422dca878f3a635821694 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 3 Jan 2020 09:39:53 +0100 Subject: [PATCH 10/17] Initialize undefined coverart locations in database with NULL ...instead of with an empty string. Both work, but NULL is more explicit. --- src/library/dao/trackdao.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 923dd3da279..a78f6abd5c2 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1941,7 +1941,7 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel, // TODO() here we may introduce a duplicate hash code updateQuery.bindValue(":coverart_hash", CoverArtUtils::calculateHash(image)); - updateQuery.bindValue(":coverart_location", ""); + updateQuery.bindValue(":coverart_location", QString()); // NULL updateQuery.bindValue(":track_id", track.trackId.toVariant()); if (!updateQuery.exec()) { LOG_FAILED_QUERY(updateQuery) << "failed to write metadata cover"; @@ -1951,6 +1951,8 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel, continue; } + // This optimization is the reason for not using CoverArtUtils::guessCoverInfo() + // that needs to determine the possible cover for every single track. if (track.directoryPath != currentDirectoryPath) { possibleCovers.clear(); currentDirectoryPath = track.directoryPath; @@ -1961,6 +1963,7 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel, CoverInfoRelative coverInfo = CoverArtUtils::selectCoverArtForTrack( trackFile, track.trackAlbum, possibleCovers); + DEBUG_ASSERT(coverInfo.source != CoverInfo::UNKNOWN); updateQuery.bindValue(":coverart_type", static_cast(coverInfo.type)); From 6d77d320b76c51c774261f737bef6aba8e562381 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 3 Jan 2020 09:42:30 +0100 Subject: [PATCH 11/17] Use CoverInfoRelative instead of CoverInfo --- src/library/coverartcache.cpp | 22 ++++++++------- src/library/coverartutils.cpp | 43 ++++++++++++----------------- src/library/coverartutils.h | 5 ++-- src/library/dlgcoverartfullsize.cpp | 17 ++++++------ src/library/dlgtrackinfo.cpp | 4 +-- src/sources/soundsourceproxy.cpp | 6 ++-- src/test/coverartutils_test.cpp | 5 ++-- 7 files changed, 48 insertions(+), 54 deletions(-) diff --git a/src/library/coverartcache.cpp b/src/library/coverartcache.cpp index 296be44e23b..23955351470 100644 --- a/src/library/coverartcache.cpp +++ b/src/library/coverartcache.cpp @@ -5,6 +5,7 @@ #include "library/coverartcache.h" #include "library/coverartutils.h" +#include "util/compatibility.h" #include "util/logger.h" @@ -201,15 +202,16 @@ void CoverArtCache::requestGuessCover(TrackPointer pTrack) { } void CoverArtCache::guessCover(TrackPointer pTrack) { - if (pTrack) { - if (kLogger.debugEnabled()) { - kLogger.debug() - << "Guessing cover art for" - << pTrack->getFileInfo(); - } - CoverInfo cover = CoverArtUtils::guessCoverInfo(*pTrack); - pTrack->setCoverInfo(cover); + VERIFY_OR_DEBUG_ASSERT(pTrack) { + return; + } + if (kLogger.debugEnabled()) { + kLogger.debug() + << "Guessing cover art for" + << pTrack->getFileInfo(); } + pTrack->setCoverInfo( + CoverArtUtils::guessCoverInfo(*pTrack)); } void CoverArtCache::guessCovers(QList tracks) { @@ -217,9 +219,9 @@ void CoverArtCache::guessCovers(QList tracks) { kLogger.debug() << "Guessing cover art for" << tracks.size() - << "tracks"; + << "track(s)"; } - foreach (TrackPointer pTrack, tracks) { + for (TrackPointer pTrack : qAsConst(tracks)) { guessCover(pTrack); } } diff --git a/src/library/coverartutils.cpp b/src/library/coverartutils.cpp index 209c69cbd86..268c8a486d5 100644 --- a/src/library/coverartutils.cpp +++ b/src/library/coverartutils.cpp @@ -80,32 +80,23 @@ QImage CoverArtUtils::loadCover(const CoverInfo& info) { } //static -CoverInfo CoverArtUtils::guessCoverInfo(const Track& track) { - CoverInfo coverInfo; - - coverInfo.trackLocation = track.getLocation(); - coverInfo.source = CoverInfo::GUESSED; - +CoverInfoRelative CoverArtUtils::guessCoverInfo( + const Track& track) { const auto trackFile = track.getFileInfo(); + QImage image = extractEmbeddedCover(trackFile, track.getSecurityToken()); if (!image.isNull()) { - // TODO() here we my introduce a duplicate hash code - coverInfo.hash = calculateHash(image); - coverInfo.coverLocation = QString(); + CoverInfoRelative coverInfo; + coverInfo.source = CoverInfo::GUESSED; coverInfo.type = CoverInfo::METADATA; - qDebug() << "CoverArtUtils::guessCover found metadata art" << coverInfo; + coverInfo.hash = calculateHash(image); + DEBUG_ASSERT(coverInfo.coverLocation.isNull()); return coverInfo; } - QLinkedList possibleCovers = findPossibleCoversInFolder( - trackFile.directory()); - coverInfo = selectCoverArtForTrack(track, possibleCovers); - if (coverInfo.type == CoverInfo::FILE) { - qDebug() << "CoverArtUtils::guessCover found file art" << coverInfo; - } else { - qDebug() << "CoverArtUtils::guessCover didn't find art" << coverInfo; - } - return coverInfo; + const QLinkedList possibleCovers = + findPossibleCoversInFolder(trackFile.directory()); + return selectCoverArtForTrack(track, possibleCovers); } //static @@ -130,13 +121,14 @@ QLinkedList CoverArtUtils::findPossibleCoversInFolder(const QString& } //static -CoverInfo CoverArtUtils::selectCoverArtForTrack( +CoverInfoRelative CoverArtUtils::selectCoverArtForTrack( const Track& track, const QLinkedList& covers) { - CoverInfoRelative coverInfoRelative = - selectCoverArtForTrack(track.getFileInfo(), track.getAlbum(), covers); - return CoverInfo(coverInfoRelative, track.getLocation()); + return selectCoverArtForTrack( + track.getFileInfo(), + track.getAlbum(), + covers); } //static @@ -145,6 +137,9 @@ CoverInfoRelative CoverArtUtils::selectCoverArtForTrack( const QString& albumName, const QLinkedList& covers) { CoverInfoRelative coverInfoRelative; + DEBUG_ASSERT(coverInfoRelative.type == CoverInfo::NONE); + DEBUG_ASSERT(coverInfoRelative.hash == 0); + DEBUG_ASSERT(coverInfoRelative.coverLocation.isNull()); coverInfoRelative.source = CoverInfo::GUESSED; if (covers.isEmpty()) { return coverInfoRelative; @@ -205,12 +200,10 @@ CoverInfoRelative CoverArtUtils::selectCoverArtForTrack( if (bestInfo != NULL) { QImage image(bestInfo->filePath()); if (!image.isNull()) { - coverInfoRelative.source = CoverInfo::GUESSED; coverInfoRelative.type = CoverInfo::FILE; // TODO() here we may introduce a duplicate hash code coverInfoRelative.hash = calculateHash(image); coverInfoRelative.coverLocation = bestInfo->fileName(); - return coverInfoRelative; } } diff --git a/src/library/coverartutils.h b/src/library/coverartutils.h index d4e440a2818..bab76a023ac 100644 --- a/src/library/coverartutils.h +++ b/src/library/coverartutils.h @@ -51,13 +51,14 @@ class CoverArtUtils { }; // Guesses the cover art for the provided track. - static CoverInfo guessCoverInfo(const Track& track); + static CoverInfoRelative guessCoverInfo( + const Track& track); static QLinkedList findPossibleCoversInFolder( const QString& folder); // Selects an appropriate cover file from provided list of image files. - static CoverInfo selectCoverArtForTrack( + static CoverInfoRelative selectCoverArtForTrack( const Track& track, const QLinkedList& covers); diff --git a/src/library/dlgcoverartfullsize.cpp b/src/library/dlgcoverartfullsize.cpp index 3cc573d700d..6b20dcaf428 100644 --- a/src/library/dlgcoverartfullsize.cpp +++ b/src/library/dlgcoverartfullsize.cpp @@ -166,18 +166,19 @@ void DlgCoverArtFullSize::slotCoverFound(const QObject* pRequestor, // slots to handle signals from the context menu void DlgCoverArtFullSize::slotReloadCoverArt() { - if (m_pLoadedTrack != nullptr) { - auto coverInfo = - CoverArtUtils::guessCoverInfo(*m_pLoadedTrack); - slotCoverInfoSelected(coverInfo); + if (!m_pLoadedTrack) { + return; } + slotCoverInfoSelected( + CoverArtUtils::guessCoverInfo(*m_pLoadedTrack)); } -void DlgCoverArtFullSize::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) { - // qDebug() << "DlgCoverArtFullSize::slotCoverInfoSelected" << coverInfo; - if (m_pLoadedTrack != nullptr) { - m_pLoadedTrack->setCoverInfo(coverInfo); +void DlgCoverArtFullSize::slotCoverInfoSelected( + const CoverInfoRelative& coverInfo) { + if (!m_pLoadedTrack) { + return; } + m_pLoadedTrack->setCoverInfo(coverInfo); } void DlgCoverArtFullSize::mousePressEvent(QMouseEvent* event) { diff --git a/src/library/dlgtrackinfo.cpp b/src/library/dlgtrackinfo.cpp index 1ea3b099482..97c8ec8fe7d 100644 --- a/src/library/dlgtrackinfo.cpp +++ b/src/library/dlgtrackinfo.cpp @@ -281,9 +281,7 @@ void DlgTrackInfo::slotReloadCoverArt() { VERIFY_OR_DEBUG_ASSERT(m_pLoadedTrack) { return; } - CoverInfo coverInfo = - CoverArtUtils::guessCoverInfo(*m_pLoadedTrack); - slotCoverInfoSelected(coverInfo); + slotCoverInfoSelected(CoverArtUtils::guessCoverInfo(*m_pLoadedTrack)); } void DlgTrackInfo::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) { diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index 09a64e3e737..56d75ccd047 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -342,9 +342,9 @@ void SoundSourceProxy::updateTrackFromSource( DEBUG_ASSERT(coverImg.isNull()); QImage* pCoverImg = nullptr; // pointer also serves as a flag if (!mergeImportedMetadata) { - const CoverInfoRelative coverInfoOld = m_pTrack->getCoverInfo(); - if (coverInfoOld.source == CoverInfo::USER_SELECTED && - coverInfoOld.type == CoverInfo::FILE) { + const auto coverInfo = m_pTrack->getCoverInfo(); + if (coverInfo.source == CoverInfo::USER_SELECTED && + coverInfo.type == CoverInfo::FILE) { // Ignore embedded cover art if (kLogger.debugEnabled()) { kLogger.debug() diff --git a/src/test/coverartutils_test.cpp b/src/test/coverartutils_test.cpp index c4594a54345..8697fc2019e 100644 --- a/src/test/coverartutils_test.cpp +++ b/src/test/coverartutils_test.cpp @@ -118,12 +118,11 @@ TEST_F(CoverArtUtilTest, searchImage) { TrackPointer pTrack(Track::newTemporary(kTrackLocationTest)); QLinkedList covers; - CoverInfo res; + CoverInfoRelative res; // looking for cover in an empty directory res = CoverArtUtils::selectCoverArtForTrack(*pTrack, covers); - CoverInfo expected1; + CoverInfoRelative expected1; expected1.source = CoverInfo::GUESSED; - expected1.trackLocation = pTrack->getLocation(); EXPECT_EQ(expected1, res); // Looking for a track with embedded cover. From 2a8eb5b2efd0fcbc6f918fc725faba48dc9b94fc Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 3 Jan 2020 09:43:58 +0100 Subject: [PATCH 12/17] Use CoverArtUtils::guessCoverInfo() when updating track from source ...to consider not only embedded cover art but also image files in the folder of the track's file. Otherwise the source of the cover info would be reset to UNKNOWN. --- src/library/coverartutils.cpp | 10 ++++++++-- src/library/coverartutils.h | 7 ++++++- src/sources/soundsourceproxy.cpp | 29 +++++------------------------ 3 files changed, 19 insertions(+), 27 deletions(-) diff --git a/src/library/coverartutils.cpp b/src/library/coverartutils.cpp index 268c8a486d5..ade5ec123d7 100644 --- a/src/library/coverartutils.cpp +++ b/src/library/coverartutils.cpp @@ -81,10 +81,16 @@ QImage CoverArtUtils::loadCover(const CoverInfo& info) { //static CoverInfoRelative CoverArtUtils::guessCoverInfo( - const Track& track) { + const Track& track, + QImage embeddedCover) { const auto trackFile = track.getFileInfo(); - QImage image = extractEmbeddedCover(trackFile, track.getSecurityToken()); + QImage image; + if (embeddedCover.isNull()) { + image = extractEmbeddedCover(trackFile, track.getSecurityToken()); + } else { + image = std::move(embeddedCover); + } if (!image.isNull()) { CoverInfoRelative coverInfo; coverInfo.source = CoverInfo::GUESSED; diff --git a/src/library/coverartutils.h b/src/library/coverartutils.h index bab76a023ac..380a9cb9372 100644 --- a/src/library/coverartutils.h +++ b/src/library/coverartutils.h @@ -51,8 +51,13 @@ class CoverArtUtils { }; // Guesses the cover art for the provided track. + // If a non-null image is provided as an optional argument + // then this is assumed to contain the embedded cover art + // that has already been extracted from the track's file + // beforehand. static CoverInfoRelative guessCoverInfo( - const Track& track); + const Track& track, + QImage embeddedCover = QImage()); static QLinkedList findPossibleCoversInFolder( const QString& folder); diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index 56d75ccd047..a8d37c786aa 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -438,30 +438,11 @@ void SoundSourceProxy::updateTrackFromSource( if (pCoverImg) { // If the pointer is not null then the cover art should be guessed - // from the embedded metadata - CoverInfoRelative coverInfoNew; - DEBUG_ASSERT(coverInfoNew.coverLocation.isNull()); - if (pCoverImg->isNull()) { - // Cover art will be cleared - DEBUG_ASSERT(coverInfoNew.source == CoverInfo::UNKNOWN); - DEBUG_ASSERT(coverInfoNew.type == CoverInfo::NONE); - if (kLogger.debugEnabled()) { - kLogger.debug() - << "No embedded cover art found in file" - << getUrl().toString(); - } - } else { - coverInfoNew.source = CoverInfo::GUESSED; - coverInfoNew.type = CoverInfo::METADATA; - // TODO(XXX) here we may introduce a duplicate hash code - coverInfoNew.hash = CoverArtUtils::calculateHash(coverImg); - if (kLogger.debugEnabled()) { - kLogger.debug() - << "Embedded cover art found in file" - << getUrl().toString(); - } - } - m_pTrack->setCoverInfo(coverInfoNew); + auto coverInfo = + CoverArtUtils::guessCoverInfo( + *m_pTrack, *pCoverImg); + DEBUG_ASSERT(coverInfo.source == CoverInfo::GUESSED); + m_pTrack->setCoverInfo(coverInfo); } } From c2e22697cc8b3f9776ed82d3aabbf8d0ba14ebba Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 4 Jan 2020 10:49:09 +0100 Subject: [PATCH 13/17] Cache possible covers from last folder Removes redundant code out of TrackDAO that doesn't belong there! --- src/library/coverartcache.cpp | 2 +- src/library/coverartutils.cpp | 72 ++++++++++++++++------------- src/library/coverartutils.h | 40 ++++++++++------ src/library/dao/trackdao.cpp | 44 ++++-------------- src/library/dlgcoverartfullsize.cpp | 3 +- src/library/dlgtrackinfo.cpp | 4 +- src/sources/soundsourceproxy.cpp | 6 ++- src/test/coverartutils_test.cpp | 4 +- 8 files changed, 89 insertions(+), 86 deletions(-) diff --git a/src/library/coverartcache.cpp b/src/library/coverartcache.cpp index 23955351470..81510c460a3 100644 --- a/src/library/coverartcache.cpp +++ b/src/library/coverartcache.cpp @@ -211,7 +211,7 @@ void CoverArtCache::guessCover(TrackPointer pTrack) { << pTrack->getFileInfo(); } pTrack->setCoverInfo( - CoverArtUtils::guessCoverInfo(*pTrack)); + CoverInfoGuesser().guessCoverInfoForTrack(*pTrack)); } void CoverArtCache::guessCovers(QList tracks) { diff --git a/src/library/coverartutils.cpp b/src/library/coverartutils.cpp index ade5ec123d7..6aa0a247559 100644 --- a/src/library/coverartutils.cpp +++ b/src/library/coverartutils.cpp @@ -80,33 +80,7 @@ QImage CoverArtUtils::loadCover(const CoverInfo& info) { } //static -CoverInfoRelative CoverArtUtils::guessCoverInfo( - const Track& track, - QImage embeddedCover) { - const auto trackFile = track.getFileInfo(); - - QImage image; - if (embeddedCover.isNull()) { - image = extractEmbeddedCover(trackFile, track.getSecurityToken()); - } else { - image = std::move(embeddedCover); - } - if (!image.isNull()) { - CoverInfoRelative coverInfo; - coverInfo.source = CoverInfo::GUESSED; - coverInfo.type = CoverInfo::METADATA; - coverInfo.hash = calculateHash(image); - DEBUG_ASSERT(coverInfo.coverLocation.isNull()); - return coverInfo; - } - - const QLinkedList possibleCovers = - findPossibleCoversInFolder(trackFile.directory()); - return selectCoverArtForTrack(track, possibleCovers); -} - -//static -QLinkedList CoverArtUtils::findPossibleCoversInFolder(const QString& folder) { +QList CoverArtUtils::findPossibleCoversInFolder(const QString& folder) { // Search for image files in the track directory. QRegExp coverArtFilenames(supportedCoverArtExtensionsRegex(), Qt::CaseInsensitive); @@ -114,7 +88,7 @@ QLinkedList CoverArtUtils::findPossibleCoversInFolder(const QString& QDir::Dirs | QDir::Files | QDir::NoDotAndDotDot); QFile currentFile; QFileInfo currentFileInfo; - QLinkedList possibleCovers; + QList possibleCovers; while (it.hasNext()) { it.next(); currentFileInfo = it.fileInfo(); @@ -129,8 +103,7 @@ QLinkedList CoverArtUtils::findPossibleCoversInFolder(const QString& //static CoverInfoRelative CoverArtUtils::selectCoverArtForTrack( const Track& track, - const QLinkedList& covers) { - + const QList& covers) { return selectCoverArtForTrack( track.getFileInfo(), track.getAlbum(), @@ -141,7 +114,7 @@ CoverInfoRelative CoverArtUtils::selectCoverArtForTrack( CoverInfoRelative CoverArtUtils::selectCoverArtForTrack( const TrackFile& trackFile, const QString& albumName, - const QLinkedList& covers) { + const QList& covers) { CoverInfoRelative coverInfoRelative; DEBUG_ASSERT(coverInfoRelative.type == CoverInfo::NONE); DEBUG_ASSERT(coverInfoRelative.hash == 0); @@ -215,3 +188,40 @@ CoverInfoRelative CoverArtUtils::selectCoverArtForTrack( return coverInfoRelative; } + +CoverInfoRelative CoverInfoGuesser::guessCoverInfo( + const TrackFile& trackFile, + const QString& albumName, + const QImage& embeddedCover) { + if (!embeddedCover.isNull()) { + CoverInfoRelative coverInfo; + coverInfo.source = CoverInfo::GUESSED; + coverInfo.type = CoverInfo::METADATA; + coverInfo.hash = CoverArtUtils::calculateHash(embeddedCover); + DEBUG_ASSERT(coverInfo.coverLocation.isNull()); + return coverInfo; + } + + const auto trackFolder = trackFile.directory(); + if (trackFolder != m_cachedFolder) { + m_cachedFolder = trackFile.directory(); + m_cachedPossibleCoversInFolder = + CoverArtUtils::findPossibleCoversInFolder( + m_cachedFolder); + } + return CoverArtUtils::selectCoverArtForTrack( + trackFile, + albumName, + m_cachedPossibleCoversInFolder); +} + +CoverInfoRelative CoverInfoGuesser::guessCoverInfoForTrack( + const Track& track) { + const auto trackFile = track.getFileInfo(); + return guessCoverInfo( + trackFile, + track.getAlbum(), + CoverArtUtils::extractEmbeddedCover( + trackFile, + track.getSecurityToken())); +} diff --git a/src/library/coverartutils.h b/src/library/coverartutils.h index 380a9cb9372..b53bd7635f2 100644 --- a/src/library/coverartutils.h +++ b/src/library/coverartutils.h @@ -1,12 +1,11 @@ -#ifndef COVERARTUTILS_H -#define COVERARTUTILS_H +#pragma once #include #include #include #include #include -#include +#include #include "track/track.h" #include "util/sandbox.h" @@ -14,6 +13,7 @@ class CoverInfo; class CoverInfoRelative; + class CoverArtUtils { public: static QString defaultCoverLocation(); @@ -51,21 +51,16 @@ class CoverArtUtils { }; // Guesses the cover art for the provided track. - // If a non-null image is provided as an optional argument - // then this is assumed to contain the embedded cover art - // that has already been extracted from the track's file - // beforehand. static CoverInfoRelative guessCoverInfo( - const Track& track, - QImage embeddedCover = QImage()); + const Track& track); - static QLinkedList findPossibleCoversInFolder( + static QList findPossibleCoversInFolder( const QString& folder); // Selects an appropriate cover file from provided list of image files. static CoverInfoRelative selectCoverArtForTrack( const Track& track, - const QLinkedList& covers); + const QList& covers); // Selects an appropriate cover file from provided list of image // files. Assumes a SecurityTokenPointer is held by the caller for all files @@ -73,11 +68,30 @@ class CoverArtUtils { static CoverInfoRelative selectCoverArtForTrack( const TrackFile& trackFile, const QString& albumName, - const QLinkedList& covers); + const QList& covers); private: CoverArtUtils() {} }; -#endif /* COVERARTUTILS_H */ +// Stateful guessing of cover art by caching the possible +// covers from the last visited folder. +class CoverInfoGuesser { + public: + // Guesses the cover art for the provided track. + // An embedded cover must be extracted beforehand and provided. + CoverInfoRelative guessCoverInfo( + const TrackFile& trackFile, + const QString& albumName, + const QImage& embeddedCover); + + // Extracts an embedded cover image if available and guesses + // the cover art for the provided track. + CoverInfoRelative guessCoverInfoForTrack( + const Track& track); + + private: + QString m_cachedFolder; + QList m_cachedPossibleCoversInFolder; +}; diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index a78f6abd5c2..7008ea10dbc 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1914,10 +1914,7 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel, "WHERE id=:track_id"); - QString currentDirectoryPath; - MDir currentDirectory; - QLinkedList possibleCovers; - + CoverInfoGuesser coverInfoGuesser; for (const auto& track: tracksWithoutCover) { if (*pCancel) { return; @@ -1932,37 +1929,14 @@ void TrackDAO::detectCoverArtForTracksWithoutCover(volatile const bool* pCancel, continue; } - QImage image(CoverArtUtils::extractEmbeddedCover(trackFile)); - if (!image.isNull()) { - updateQuery.bindValue(":coverart_type", - static_cast(CoverInfo::METADATA)); - updateQuery.bindValue(":coverart_source", - static_cast(CoverInfo::GUESSED)); - // TODO() here we may introduce a duplicate hash code - updateQuery.bindValue(":coverart_hash", - CoverArtUtils::calculateHash(image)); - updateQuery.bindValue(":coverart_location", QString()); // NULL - updateQuery.bindValue(":track_id", track.trackId.toVariant()); - if (!updateQuery.exec()) { - LOG_FAILED_QUERY(updateQuery) << "failed to write metadata cover"; - } else { - pTracksChanged->insert(track.trackId); - } - continue; - } - - // This optimization is the reason for not using CoverArtUtils::guessCoverInfo() - // that needs to determine the possible cover for every single track. - if (track.directoryPath != currentDirectoryPath) { - possibleCovers.clear(); - currentDirectoryPath = track.directoryPath; - currentDirectory = MDir(currentDirectoryPath); - possibleCovers = CoverArtUtils::findPossibleCoversInFolder( - currentDirectoryPath); - } - - CoverInfoRelative coverInfo = CoverArtUtils::selectCoverArtForTrack( - trackFile, track.trackAlbum, possibleCovers); + const auto embeddedCover = + CoverArtUtils::extractEmbeddedCover( + trackFile); + const auto coverInfo = + coverInfoGuesser.guessCoverInfo( + trackFile, + track.trackAlbum, + embeddedCover); DEBUG_ASSERT(coverInfo.source != CoverInfo::UNKNOWN); updateQuery.bindValue(":coverart_type", diff --git a/src/library/dlgcoverartfullsize.cpp b/src/library/dlgcoverartfullsize.cpp index 6b20dcaf428..73a3c31b7c7 100644 --- a/src/library/dlgcoverartfullsize.cpp +++ b/src/library/dlgcoverartfullsize.cpp @@ -170,7 +170,8 @@ void DlgCoverArtFullSize::slotReloadCoverArt() { return; } slotCoverInfoSelected( - CoverArtUtils::guessCoverInfo(*m_pLoadedTrack)); + CoverInfoGuesser().guessCoverInfoForTrack( + *m_pLoadedTrack)); } void DlgCoverArtFullSize::slotCoverInfoSelected( diff --git a/src/library/dlgtrackinfo.cpp b/src/library/dlgtrackinfo.cpp index 97c8ec8fe7d..374e19e9dd3 100644 --- a/src/library/dlgtrackinfo.cpp +++ b/src/library/dlgtrackinfo.cpp @@ -281,7 +281,9 @@ void DlgTrackInfo::slotReloadCoverArt() { VERIFY_OR_DEBUG_ASSERT(m_pLoadedTrack) { return; } - slotCoverInfoSelected(CoverArtUtils::guessCoverInfo(*m_pLoadedTrack)); + slotCoverInfoSelected( + CoverInfoGuesser().guessCoverInfoForTrack( + *m_pLoadedTrack)); } void DlgTrackInfo::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) { diff --git a/src/sources/soundsourceproxy.cpp b/src/sources/soundsourceproxy.cpp index a8d37c786aa..ba98e8e2c4f 100644 --- a/src/sources/soundsourceproxy.cpp +++ b/src/sources/soundsourceproxy.cpp @@ -439,8 +439,10 @@ void SoundSourceProxy::updateTrackFromSource( if (pCoverImg) { // If the pointer is not null then the cover art should be guessed auto coverInfo = - CoverArtUtils::guessCoverInfo( - *m_pTrack, *pCoverImg); + CoverInfoGuesser().guessCoverInfo( + m_pTrack->getFileInfo(), + m_pTrack->getAlbum(), + *pCoverImg); DEBUG_ASSERT(coverInfo.source == CoverInfo::GUESSED); m_pTrack->setCoverInfo(coverInfo); } diff --git a/src/test/coverartutils_test.cpp b/src/test/coverartutils_test.cpp index 8697fc2019e..74e106314f7 100644 --- a/src/test/coverartutils_test.cpp +++ b/src/test/coverartutils_test.cpp @@ -117,7 +117,7 @@ TEST_F(CoverArtUtilTest, searchImage) { const QString kTrackLocationTest(kTestDir.absoluteFilePath("cover-test-png.mp3")); TrackPointer pTrack(Track::newTemporary(kTrackLocationTest)); - QLinkedList covers; + QList covers; CoverInfoRelative res; // looking for cover in an empty directory res = CoverArtUtils::selectCoverArtForTrack(*pTrack, covers); @@ -192,7 +192,7 @@ TEST_F(CoverArtUtilTest, searchImage) { extraCovers << cLoc_big3; // saving more covers using the preferred names in the right order - QLinkedList prefCovers; + QList prefCovers; // 1. track_filename.jpg QString cLoc_filename = QString(trackdir % "/cover-test." % qFormat); EXPECT_TRUE(img.scaled(500,500).save(cLoc_filename, format)); From 3cc61cb4e72cd53612d11a9ced1b473c8eafb576 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 5 Jan 2020 12:20:13 +0100 Subject: [PATCH 14/17] Restate re-export of ID3 tags for fractional bpm values --- src/track/bpm.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/track/bpm.h b/src/track/bpm.h index ab7ffdc2abf..d371fb8c817 100644 --- a/src/track/bpm.h +++ b/src/track/bpm.h @@ -31,6 +31,9 @@ class Bpm final { // fractional value the ID3 metadata is always detected as modified // and will be exported regardless if it has actually been modified // or not. + // TL;DR: If metadata export is enabled ID3 tags will be rewritten + // for all files with a fractional bpm values even if their metadata + // has not been modified. void normalizeBeforeExport() { m_value = normalizeValue(m_value); } From 2dfa71c2b879a6fc238a10bd1032db3dcf8aa9ab Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 5 Jan 2020 12:42:16 +0100 Subject: [PATCH 15/17] Add different options for comparing bpm values --- src/track/bpm.h | 22 +++++++++++++- src/track/trackinfo.cpp | 64 +++++++++++++++++++++-------------------- src/track/trackinfo.h | 9 +++++- 3 files changed, 62 insertions(+), 33 deletions(-) diff --git a/src/track/bpm.h b/src/track/bpm.h index d371fb8c817..e87856bb9fd 100644 --- a/src/track/bpm.h +++ b/src/track/bpm.h @@ -61,13 +61,33 @@ class Bpm final { return std::round(value); } + enum class Comparison { + Default, // full precision + Integer, // rounded + String, // stringified + }; + + bool compareEq( + const Bpm& bpm, + Comparison cmp = Comparison::Default) const { + switch (cmp) { + case Comparison::Integer: + return Bpm::valueToInteger(getValue()) == Bpm::valueToInteger(bpm.getValue()); + case Comparison::String: + return Bpm::valueToString(getValue()) == Bpm::valueToString(bpm.getValue()); + case Comparison::Default: + default: + return getValue() == bpm.getValue(); + } + } + private: double m_value; }; inline bool operator==(const Bpm& lhs, const Bpm& rhs) { - return lhs.getValue() == rhs.getValue(); + return lhs.compareEq(rhs); } inline diff --git a/src/track/trackinfo.cpp b/src/track/trackinfo.cpp index 1950f4ffe6c..0cd1da6781b 100644 --- a/src/track/trackinfo.cpp +++ b/src/track/trackinfo.cpp @@ -42,47 +42,49 @@ bool TrackInfo::parseArtistTitleFromFileName( return modified; } -bool operator==(const TrackInfo& lhs, const TrackInfo& rhs) { - return (lhs.getArtist() == rhs.getArtist()) && - (lhs.getBpm() == rhs.getBpm()) && - (lhs.getComment() == rhs.getComment()) && - (lhs.getComposer() == rhs.getComposer()) && +bool TrackInfo::compareEq( + const TrackInfo& trackInfo, + Bpm::Comparison cmpBpm) const { + return (getArtist() == trackInfo.getArtist()) && + getBpm().compareEq(trackInfo.getBpm(), cmpBpm) && + (getComment() == trackInfo.getComment()) && + (getComposer() == trackInfo.getComposer()) && #if defined(__EXTRA_METADATA__) - (lhs.getConductor() == rhs.getConductor()) && - (lhs.getDiscNumber() == rhs.getDiscNumber()) && - (lhs.getDiscTotal() == rhs.getDiscTotal()) && - (lhs.getEncoder() == rhs.getEncoder()) && - (lhs.getEncoderSettings() == rhs.getEncoderSettings()) && + (getConductor() == trackInfo.getConductor()) && + (getDiscNumber() == trackInfo.getDiscNumber()) && + (getDiscTotal() == trackInfo.getDiscTotal()) && + (getEncoder() == trackInfo.getEncoder()) && + (getEncoderSettings() == trackInfo.getEncoderSettings()) && #endif // __EXTRA_METADATA__ - (lhs.getGenre() == rhs.getGenre()) && - (lhs.getGrouping() == rhs.getGrouping()) && + (getGenre() == trackInfo.getGenre()) && + (getGrouping() == trackInfo.getGrouping()) && #if defined(__EXTRA_METADATA__) - (lhs.getISRC() == rhs.getISRC()) && + (getISRC() == trackInfo.getISRC()) && #endif // __EXTRA_METADATA__ - (lhs.getKey() == rhs.getKey()) && + (getKey() == trackInfo.getKey()) && #if defined(__EXTRA_METADATA__) - (lhs.getLanguage() == rhs.getLanguage()) && - (lhs.getLyricist() == rhs.getLyricist()) && - (lhs.getMood() == rhs.getMood()) && - (lhs.getMovement() == rhs.getMovement()) && - (lhs.getMusicBrainzArtistId() == rhs.getMusicBrainzArtistId()) && - (lhs.getMusicBrainzRecordingId() == rhs.getMusicBrainzRecordingId()) && - (lhs.getMusicBrainzReleaseId() == rhs.getMusicBrainzReleaseId()) && - (lhs.getMusicBrainzWorkId() == rhs.getMusicBrainzWorkId()) && - (lhs.getRemixer() == rhs.getRemixer()) && + (getLanguage() == trackInfo.getLanguage()) && + (getLyricist() == trackInfo.getLyricist()) && + (getMood() == trackInfo.getMood()) && + (getMovement() == trackInfo.getMovement()) && + (getMusicBrainzArtistId() == trackInfo.getMusicBrainzArtistId()) && + (getMusicBrainzRecordingId() == trackInfo.getMusicBrainzRecordingId()) && + (getMusicBrainzReleaseId() == trackInfo.getMusicBrainzReleaseId()) && + (getMusicBrainzWorkId() == trackInfo.getMusicBrainzWorkId()) && + (getRemixer() == trackInfo.getRemixer()) && #endif // __EXTRA_METADATA__ - (lhs.getReplayGain() == rhs.getReplayGain()) && + (getReplayGain() == trackInfo.getReplayGain()) && #if defined(__EXTRA_METADATA__) - (lhs.getSeratoMarkers2() == rhs.getSeratoMarkers2()) && - (lhs.getSubtitle() == rhs.getSubtitle()) && + (getSeratoMarkers2() == trackInfo.getSeratoMarkers2()) && + (getSubtitle() == trackInfo.getSubtitle()) && #endif // __EXTRA_METADATA__ - (lhs.getTitle() == rhs.getTitle()) && - (lhs.getTrackNumber() == rhs.getTrackNumber()) && - (lhs.getTrackTotal() == rhs.getTrackTotal()) && + (getTitle() == trackInfo.getTitle()) && + (getTrackNumber() == trackInfo.getTrackNumber()) && + (getTrackTotal() == trackInfo.getTrackTotal()) && #if defined(__EXTRA_METADATA__) - (lhs.getWork() == rhs.getWork()) && + (getWork() == trackInfo.getWork()) && #endif // __EXTRA_METADATA__ - (lhs.getYear() == rhs.getYear()); + (getYear() == trackInfo.getYear()); } QDebug operator<<(QDebug dbg, const TrackInfo& arg) { diff --git a/src/track/trackinfo.h b/src/track/trackinfo.h index a46b89958da..e322587bd69 100644 --- a/src/track/trackinfo.h +++ b/src/track/trackinfo.h @@ -78,9 +78,16 @@ class TrackInfo final { refBpm().normalizeBeforeExport(); refReplayGain().normalizeBeforeExport(); } + + bool compareEq( + const TrackInfo& trackInfo, + Bpm::Comparison cmpBpm = Bpm::Comparison::Default) const; }; -bool operator==(const TrackInfo& lhs, const TrackInfo& rhs); +inline +bool operator==(const TrackInfo& lhs, const TrackInfo& rhs) { + return lhs.compareEq(rhs); +} inline bool operator!=(const TrackInfo& lhs, const TrackInfo& rhs) { From 192283b6653b6f448077d62c3b66877434ead0f0 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 5 Jan 2020 13:04:08 +0100 Subject: [PATCH 16/17] Reduce precision for comparing bpm values when exporting metadata --- src/track/track.cpp | 11 ++++++++++- src/track/trackmetadata.cpp | 5 +++-- src/track/trackmetadata.h | 8 +++++++- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/track/track.cpp b/src/track/track.cpp index 33fbd8c73db..6aa9e800242 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -979,8 +979,17 @@ ExportTrackMetadataResult Track::exportMetadata( // copy all extra properties that are not (yet) stored in the library before // checking for differences! If an export has been requested explicitly then // we will continue even if no differences are detected. + // NOTE(uklotzde, 2020-01-05): Detection of modified bpm values is restricted + // to integer precision to avoid re-exporting of unmodified ID3 tags in case + // of fractional bpm values. As a consequence small changes in bpm values + // cannot be detected and file tags with fractional values might not be + // updated as expected! In these edge cases users need to explicitly + // trigger the re-export of file tags or they could modify other metadata + // properties. if (!m_bMarkedForMetadataExport && - !m_record.getMetadata().anyFileTagsModified(importedFromFile)) { + !m_record.getMetadata().anyFileTagsModified( + importedFromFile, + mixxx::Bpm::Comparison::Integer)) { // The file tags are in-sync with the track's metadata and don't need // to be updated. if (kLogger.debugEnabled()) { diff --git a/src/track/trackmetadata.cpp b/src/track/trackmetadata.cpp index 6bfd6f58b3d..b372430a186 100644 --- a/src/track/trackmetadata.cpp +++ b/src/track/trackmetadata.cpp @@ -72,13 +72,14 @@ void TrackMetadata::normalizeBeforeExport() { } bool TrackMetadata::anyFileTagsModified( - const TrackMetadata& importedFromFile) const { + const TrackMetadata& importedFromFile, + Bpm::Comparison cmpBpm) const { // NOTE(uklotzde): The read-only audio properties that are stored // directly as members of this class might differ after they have // been updated while decoding audio data. They are read-only and // must not be considered when exporting metadata! return getAlbumInfo() != importedFromFile.getAlbumInfo() || - getTrackInfo() != importedFromFile.getTrackInfo(); + !getTrackInfo().compareEq(importedFromFile.getTrackInfo(), cmpBpm); } bool operator==(const TrackMetadata& lhs, const TrackMetadata& rhs) { diff --git a/src/track/trackmetadata.h b/src/track/trackmetadata.h index 9d4888841a2..9ca165c4bed 100644 --- a/src/track/trackmetadata.h +++ b/src/track/trackmetadata.h @@ -40,7 +40,13 @@ class TrackMetadata final { // Returns true if the current metadata differs from the imported metadata // and needs to be exported. A result of false indicates that no export // is needed. - bool anyFileTagsModified(const TrackMetadata& importedFromFile) const; + // NOTE: Some tag formats like ID3v1/v2 only support integer precision + // for storing bpm values. To avoid re-exporting unmodified track metadata + // with fractional bpm values over and over again the comparison of bpm + // values should be restricted to integer. + bool anyFileTagsModified( + const TrackMetadata& importedFromFile, + Bpm::Comparison cmpBpm = Bpm::Comparison::Default) const; // Parse an format date/time values according to ISO 8601 static QDate parseDate(QString str) { From 94ef069ba591e8b009275557885fd4a16df3ad03 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 8 Jan 2020 22:33:29 +0100 Subject: [PATCH 17/17] Update comment --- src/track/bpm.h | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/track/bpm.h b/src/track/bpm.h index e87856bb9fd..968d351c783 100644 --- a/src/track/bpm.h +++ b/src/track/bpm.h @@ -24,16 +24,13 @@ class Bpm final { static double normalizeValue(double value); // Adjusts floating-point values to match their string representation - // in file tags to account for rounding errors. - // NOTE(2019-02-19, uklotzde): The pre-normalization cannot prevent - // repeated export of metadata for files with ID3 tags that are only - // able to store the BPM value with integer precision! In case of a - // fractional value the ID3 metadata is always detected as modified - // and will be exported regardless if it has actually been modified - // or not. - // TL;DR: If metadata export is enabled ID3 tags will be rewritten - // for all files with a fractional bpm values even if their metadata - // has not been modified. + // in file tags to account for rounding errors and false positives + // when checking for modifications. + // NOTE(2020-01-08, uklotzde): Since bpm values are stored with + // integer precision in ID3 tags, bpm values are only considered + // as modified if their rounded integer values differ. But even + // then this pre-normalization step should not be skipped to prevent + // fluttering values for other tag formats. void normalizeBeforeExport() { m_value = normalizeValue(m_value); }