Merge remote-tracking branch 'upstream/develop' into develop

SUBSONIC.md

@@ -6,40 +6,41 @@ Given the API limitations of folder navigation commands, it is recommended to pl
 
 The Subsonic API is enabled by default.
 
-__Note__: since _LMS_ may store hashed and salted passwords or may forward authentication requests to external services, it cannot handle the __token authentication__ method. You may need to check your client to make sure to use the __password__ authentication method. Since logins/passwords are passed in plain text through URLs, it is highly recommended to use a unique password when using the Subsonic API. Note that this may affect the use of authentication via PAM. In any case, ensure that read access to the web server logs (and to the proxy, if applicable) is well protected.
+__Note__: since _LMS_ may store hashed and salted passwords or may forward authentication requests to external services, it cannot handle the __token authentication__ method. You may need to check your client to make sure to use the __password__ authentication method. Since logins/passwords are passed in plain text through URLs, it is highly recommended to use a unique password when using the Subsonic API. Note that this may affect the use of authentication via PAM. In any case, ensure the web server logs (and proxy logs, if applicable) are properly secured.
 
 # OpenSubsonic API
 OpenSubsonic is an initiative to patch and extend the legacy Subsonic API. You'll find more details in the [official documentation](https://opensubsonic.netlify.app/)
 
 ## Extra fields
 The following extra fields are implemented:
 * `Album` response:
-  * `mediaType`
-  * `played`
-  * `musicBrainzId`
-  * `genres`
   * `artists`
+  * `discTitles`: discs with no subtitle are omitted
   * `displayArtist`
-  * `releaseTypes`
+  * `genres`
+  * `isCompilation`
+  * `played`
+  * `mediaType`
   * `moods`
+  * `musicBrainzId`
   * `originalReleaseDate`
-  * `isCompilation`
-  * `discTitles`: discs with no subtitle are omitted
+  * `releaseTypes`
 * `Child` response:
+  * `albumArtists`
+  * `artists`
   * `bitDepth`
-  * `samplingRate`
   * `channelCount`
-  * `mediaType`
-  * `played`
-  * `musicBrainzId`: note this is actually the recording MBID when this response refers to a song
-  * `genres`
-  * `artists`
-  * `displayArtist`
-  * `albumArtists`
-  * `displayAlbumArtist`
+  * `comment`
   * `contributors`
+  * `displayAlbumArtist`
+  * `displayArtist`
+  * `genres`
+  * `mediaType`
   * `moods`
+  * `musicBrainzId`: note this is actually the recording MBID when this response refers to a song
+  * `played`
   * `replayGain`
+  * `samplingRate`
 * `Artist` response:
   * `mediaType`
   * `musicBrainzId`

approot/tracks.xml

@@ -141,6 +141,10 @@
 								${playcount}
 							</div>
 						</div>
+						${<if-has-comment>}
+						<hr/>
+						<pre>${comment}</pre>
+						${</if-has-comment>}
 					</div>
 				</div>
 				<div class="modal-footer">

src/libs/database/impl/Migration.cpp

@@ -35,7 +35,7 @@ namespace lms::db
 {
     namespace
     {
-        static constexpr Version LMS_DATABASE_VERSION{ 62 };
+        static constexpr Version LMS_DATABASE_VERSION{ 63 };
     }
 
     VersionInfo::VersionInfo()
@@ -669,6 +669,15 @@ SELECT
         session.getDboSession()->execute("UPDATE scan_settings SET scan_version = scan_version + 1");
     }
 
+    void migrateFromV62(Session& session)
+    {
+        // Add a new column comment
+        session.getDboSession()->execute("ALTER TABLE track ADD comment TEXT NOT NULL DEFAULT ''");
+
+        // Just increment the scan version of the settings to make the next scheduled scan rescan everything
+        session.getDboSession()->execute("UPDATE scan_settings SET scan_version = scan_version + 1");
+    }
+
     bool doDbMigration(Session& session)
     {
         static const std::string outdatedMsg{ "Outdated database, please rebuild it (delete the .db file and restart)" };
@@ -707,6 +716,7 @@ SELECT
             { 59, migrateFromV59 },
             { 60, migrateFromV60 },
             { 61, migrateFromV61 },
+            { 62, migrateFromV62 },
         };
 
         bool migrationPerformed{};

src/libs/database/include/database/Track.hpp

@@ -235,6 +235,7 @@ namespace lms::db
         void setTrackReplayGain(std::optional<float> replayGain) { _trackReplayGain = replayGain; }
         void setReleaseReplayGain(std::optional<float> replayGain) { _releaseReplayGain = replayGain; } // may be by disc!
         void setArtistDisplayName(std::string_view name) { _artistDisplayName = name; }
+        void setComment(std::string_view comment) { _comment = comment; }
         void clearArtistLinks();
         void addArtistLink(const ObjectPtr<TrackArtistLink>& artistLink);
         void setRelease(ObjectPtr<Release> release) { _release = getDboPtr(release); }
@@ -272,6 +273,8 @@ namespace lms::db
         std::optional<float> getTrackReplayGain() const { return _trackReplayGain; }
         std::optional<float> getReleaseReplayGain() const { return _releaseReplayGain; }
         std::string_view getArtistDisplayName() const { return _artistDisplayName; }
+        std::string_view getComment() const { return _comment; }
+
         // no artistLinkTypes means get all
         std::vector<ObjectPtr<Artist>> getArtists(core::EnumSet<TrackArtistLinkType> artistLinkTypes) const; // no type means all
         std::vector<ArtistId> getArtistIds(core::EnumSet<TrackArtistLinkType> artistLinkTypes) const;        // no type means all
@@ -316,6 +319,8 @@ namespace lms::db
             Wt::Dbo::field(a, _trackReplayGain, "track_replay_gain");
             Wt::Dbo::field(a, _releaseReplayGain, "release_replay_gain"); // here in Track since Release does not have concept of "disc" (yet?)
             Wt::Dbo::field(a, _artistDisplayName, "artist_display_name");
+            Wt::Dbo::field(a, _comment, "comment");
+
             Wt::Dbo::belongsTo(a, _release, "release", Wt::Dbo::OnDeleteCascade);
             Wt::Dbo::belongsTo(a, _mediaLibrary, "media_library", Wt::Dbo::OnDeleteSetNull); // don't delete track on media library removal, we want to wait for the next scan to have a chance to migrate files
             Wt::Dbo::belongsTo(a, _directory, "directory", Wt::Dbo::OnDeleteCascade);
@@ -360,6 +365,7 @@ namespace lms::db
         std::optional<float> _trackReplayGain;
         std::optional<float> _releaseReplayGain;
         std::string _artistDisplayName;
+        std::string _comment;
 
         Wt::Dbo::ptr<Release> _release;
         Wt::Dbo::ptr<MediaLibrary> _mediaLibrary;

src/libs/database/test/Track.cpp

@@ -363,4 +363,24 @@ namespace lms::db::tests
             EXPECT_EQ(track->getSampleRate(), 44100);
         }
     }
+
+    TEST_F(DatabaseFixture, Track_comment)
+    {
+        ScopedTrack track{ session };
+
+        {
+            auto transaction{ session.createReadTransaction() };
+            EXPECT_EQ(track->getComment(), "");
+        }
+
+        {
+            auto transaction{ session.createWriteTransaction() };
+            track.get().modify()->setComment("MyComment");
+        }
+
+        {
+            auto transaction{ session.createReadTransaction() };
+            EXPECT_EQ(track->getComment(), "MyComment");
+        }
+    }
 } // namespace lms::db::tests

src/libs/metadata/impl/Parser.cpp

@@ -273,6 +273,7 @@ namespace lms::metadata
             track.originalYear = utils::parseYear(*dateStr);
         }
 
+        track.comments = getTagValuesAs<std::string>(tagReader, TagType::Comment, {} /* no custom delimiter on comments */);
         track.copyright = getTagValueAs<std::string>(tagReader, TagType::Copyright).value_or("");
         track.copyrightURL = getTagValueAs<std::string>(tagReader, TagType::CopyrightURL).value_or("");
         track.replayGain = getTagValueAs<float>(tagReader, TagType::ReplayGainTrackGain);
@@ -297,11 +298,26 @@ namespace lms::metadata
         track.medium = getMedium(tagReader);
         track.artists = getArtists(tagReader, { TagType::Artists, TagType::Artist }, { TagType::ArtistSortOrder }, { TagType::MusicBrainzArtistID }, _artistTagDelimiters);
 
-        // We consider the artist display name is put in the Artist tag (picard case)
-        // But to please most users, if we find a custom delimiter in the Artist tag, we construct the artist diplay string with a "nicer" join
-        if (!_artistTagDelimiters.empty()
-            && track.artists.size() > 1
-            && getTagValuesAs<std::string>(tagReader, TagType::Artist, _artistTagDelimiters).size() > 1)
+        auto needReconstructArtistDisplayName{ [&] {
+            // We consider the artist display name is put in the Artist tag (picard case)
+
+            // To please most users, if we find a custom delimiter in the Artist tag, we construct the artist display string with a "nicer" join
+            if (!_artistTagDelimiters.empty()
+                && track.artists.size() > 1
+                && getTagValuesAs<std::string>(tagReader, TagType::Artist, _artistTagDelimiters).size() > 1)
+            {
+                return true;
+            }
+            // We have (true) multiple entries in the Artist tag or nothing
+            else if (getTagValuesAs<std::string>(tagReader, TagType::Artist, {}).size() != 1)
+            {
+                return true;
+            }
+
+            return false;
+        } };
+
+        if (needReconstructArtistDisplayName())
         {
             std::vector<std::string_view> artistNames;
             std::transform(std::cbegin(track.artists), std::cend(track.artists), std::back_inserter(artistNames), [](const Artist& artist) -> std::string_view { return artist.name; });

src/libs/metadata/impl/TagLibTagReader.cpp

@@ -21,6 +21,7 @@
 
 #include <unordered_map>
 
+#include <taglib/aifffile.h>
 #include <taglib/apeproperties.h>
 #include <taglib/apetag.h>
 #include <taglib/asffile.h>
@@ -37,6 +38,7 @@
 #include <taglib/tag.h>
 #include <taglib/tpropertymap.h>
 #include <taglib/vorbisfile.h>
+#include <taglib/wavfile.h>
 #include <taglib/wavpackfile.h>
 
 #include "core/ILogger.hpp"
@@ -284,6 +286,27 @@ namespace lms::metadata
             TagLib::MP4::CoverArtList coverArtList{ coverItem.toCoverArtList() };
             if (!coverArtList.isEmpty())
                 _hasEmbeddedCover = true;
+
+            if (!_propertyMap.contains("ORIGINALDATE"))
+            {
+                // For now:
+                // * TagLib 2.0 only parses ----:com.apple.iTunes:ORIGINALDATE
+                // / TagLib <2.0 only parses ----:com.apple.iTunes:originaldate
+                const auto& tags{ mp4File->tag()->itemMap() };
+                for (const auto& origDateString : { "----:com.apple.iTunes:originaldate", "----:com.apple.iTunes:ORIGINALDATE" })
+                {
+                    auto itOrigDateTag{ tags.find(origDateString) };
+                    if (itOrigDateTag != std::cend(tags))
+                    {
+                        const TagLib::StringList dates{ itOrigDateTag->second.toStringList() };
+                        if (!dates.isEmpty())
+                        {
+                            _propertyMap["ORIGINALDATE"] = dates.front();
+                            break;
+                        }
+                    }
+                }
+            }
         }
         // MPC
         else if (TagLib::MPC::File * mpcFile{ dynamic_cast<TagLib::MPC::File*>(_file.file()) })
@@ -311,6 +334,26 @@ namespace lms::metadata
             if (!opusFile->tag()->pictureList().isEmpty())
                 _hasEmbeddedCover = true;
         }
+        else if (TagLib::RIFF::AIFF::File * aiffFile{ dynamic_cast<TagLib::RIFF::AIFF::File*>(_file.file()) })
+        {
+            if (aiffFile->hasID3v2Tag())
+            {
+                const auto& frameListMap{ aiffFile->tag()->frameListMap() };
+
+                if (!frameListMap["APIC"].isEmpty())
+                    _hasEmbeddedCover = true;
+            }
+        }
+        else if (TagLib::RIFF::WAV::File * wavFile{ dynamic_cast<TagLib::RIFF::WAV::File*>(_file.file()) })
+        {
+            if (wavFile->hasID3v2Tag())
+            {
+                const auto& frameListMap{ wavFile->ID3v2Tag()->frameListMap() };
+
+                if (!frameListMap["APIC"].isEmpty())
+                    _hasEmbeddedCover = true;
+            }
+        }
 
         if (debug && core::Service<core::logging::ILogger>::get()->isSeverityActive(core::logging::Severity::DEBUG))
         {
@@ -345,6 +388,10 @@ namespace lms::metadata
             _audioProperties.bitsPerSample = mp4Properties->bitsPerSample();
         else if (const auto* wavePackProperties{ dynamic_cast<const TagLib::WavPack::Properties*>(properties) })
             _audioProperties.bitsPerSample = wavePackProperties->bitsPerSample();
+        else if (const auto* aiffProperties{ dynamic_cast<const TagLib::RIFF::AIFF::Properties*>(properties) })
+            _audioProperties.bitsPerSample = aiffProperties->bitsPerSample();
+        else if (const auto* wavProperties{ dynamic_cast<const TagLib::RIFF::WAV::Properties*>(properties) })
+            _audioProperties.bitsPerSample = wavProperties->bitsPerSample();
 #if TAGLIB_MAJOR_VERSION >= 2
         else if (const auto* dsfProperties{ dynamic_cast<const TagLib::DSF::Properties*>(properties) })
             _audioProperties.bitsPerSample = dsfProperties->bitsPerSample();

src/libs/metadata/include/metadata/Types.hpp

@@ -118,6 +118,7 @@ namespace lms::metadata
         std::optional<core::UUID> acoustID;
         std::string copyright;
         std::string copyrightURL;
+        std::vector<std::string> comments;
         std::optional<float> replayGain;
         std::string artistDisplayName;
         std::vector<Artist> artists;

src/libs/metadata/test/Parser.cpp

@@ -41,6 +41,7 @@ namespace lms::metadata
                 { TagType::AlbumArtist, { "MyAlbumArtist1 & MyAlbumArtist2" } },
                 { TagType::AlbumArtists, { "MyAlbumArtist1", "MyAlbumArtist2" } },
                 { TagType::AlbumArtistsSortOrder, { "MyAlbumArtist1SortName", "MyAlbumArtist2SortName" } },
+                { TagType::Comment, { "Comment1", "Comment2" } },
                 { TagType::Composer, { "MyComposer1", "MyComposer2" } },
                 { TagType::ComposerSortOrder, { "MyComposerSortOrder1", "MyComposerSortOrder2" } },
                 { TagType::Conductor, { "MyConductor1", "MyConductor2" } },
@@ -104,6 +105,9 @@ namespace lms::metadata
         EXPECT_EQ(track->artists[1].name, "MyArtist2");
         EXPECT_EQ(track->artists[1].sortName, "MyArtist2SortName");
         EXPECT_EQ(track->artists[1].mbid, core::UUID::fromString("5e2cf87f-c8d7-4504-8a86-954dc0840229"));
+        ASSERT_EQ(track->comments.size(), 2);
+        EXPECT_EQ(track->comments[0], "Comment1");
+        EXPECT_EQ(track->comments[1], "Comment2");
         ASSERT_EQ(track->composerArtists.size(), 2);
         EXPECT_EQ(track->composerArtists[0].name, "MyComposer1");
         EXPECT_EQ(track->composerArtists[0].sortName, "MyComposerSortOrder1");
@@ -255,4 +259,68 @@ namespace lms::metadata
         EXPECT_EQ(track->artists[1].name, "Other Artist");
         EXPECT_EQ(track->artistDisplayName, "This /  is ; One Artist, Other Artist"); // reconstruct artist display name since a custom delimiter is hit
     }
+
+    TEST(Parser, noArtistInArtist)
+    {
+        const TestTagReader testTags{
+            {
+                // nothing in Artist!
+            }
+        };
+
+        std::unique_ptr<Track> track{ Parser{}.parse(testTags) };
+
+        ASSERT_EQ(track->artists.size(), 0);
+        EXPECT_EQ(track->artistDisplayName, "");
+    }
+
+    TEST(Parser, singleArtistInArtist)
+    {
+        const TestTagReader testTags{
+            {
+                // nothing in Artist!
+                { TagType::Artists, { "Artist1" } },
+            }
+        };
+
+        std::unique_ptr<Track> track{ Parser{}.parse(testTags) };
+
+        ASSERT_EQ(track->artists.size(), 1);
+        EXPECT_EQ(track->artists[0].name, "Artist1");
+        EXPECT_EQ(track->artistDisplayName, "Artist1");
+    }
+
+    TEST(Parser, multipleArtistsInArtist)
+    {
+        const TestTagReader testTags{
+            {
+                // nothing in Artists!
+                { TagType::Artist, { "Artist1", "Artist2" } },
+            }
+        };
+
+        std::unique_ptr<Track> track{ Parser{}.parse(testTags) };
+
+        ASSERT_EQ(track->artists.size(), 2);
+        EXPECT_EQ(track->artists[0].name, "Artist1");
+        EXPECT_EQ(track->artists[1].name, "Artist2");
+        EXPECT_EQ(track->artistDisplayName, "Artist1, Artist2"); // reconstruct artist display name since multiple entries are found
+    }
+
+    TEST(Parser, multipleArtistsInArtists)
+    {
+        const TestTagReader testTags{
+            {
+                // nothing in Artist!
+                { TagType::Artists, { "Artist1", "Artist2" } },
+            }
+        };
+
+        std::unique_ptr<Track> track{ Parser{}.parse(testTags) };
+
+        ASSERT_EQ(track->artists.size(), 2);
+        EXPECT_EQ(track->artists[0].name, "Artist1");
+        EXPECT_EQ(track->artists[1].name, "Artist2");
+        EXPECT_EQ(track->artistDisplayName, "Artist1, Artist2"); // reconstruct artist display name since multiple entries are found and nothing is set in artist
+    }
 } // namespace lms::metadata

src/libs/services/scanner/impl/ScanStepScanFiles.cpp

@@ -711,6 +711,7 @@ namespace lms::scanner
         track.modify()->setHasCover(trackMetadata->hasCover);
         track.modify()->setCopyright(trackMetadata->copyright);
         track.modify()->setCopyrightURL(trackMetadata->copyrightURL);
+        track.modify()->setComment(!trackMetadata->comments.empty() ? trackMetadata->comments.front() : ""); // only take the first one for now
         track.modify()->setTrackReplayGain(trackMetadata->replayGain);
         track.modify()->setArtistDisplayName(trackMetadata->artistDisplayName);
 

src/libs/subsonic/impl/responses/Song.cpp

@@ -153,6 +153,7 @@ namespace lms::api::subsonic
         if (!context.enableOpenSubsonic)
             return trackResponse;
 
+        trackResponse.setAttribute("comment", track->getComment());
         trackResponse.setAttribute("bitDepth", track->getBitsPerSample());
         trackResponse.setAttribute("samplingRate", track->getSampleRate());
         trackResponse.setAttribute("channelCount", track->getChannelCount());