Parse more itunes keys, optimize taglib wrapper (navidrome#2680)

* parse more itunes keys * Move special iTunes M4A logic to Go code * Simplify ASF/WMA tags handling * Simplify ASF/WMA tags handling even more, moving compilation logic to `metadata` normalizer * Remove strdups from C++ code, `C.GoString` already duplicates the strings * reduced set * remove strdup * Small nitpick --------- Co-authored-by: Deluan <[email protected]>
MrEddX · Dec 3, 2023 · 742fd16 · 742fd16
1 parent 7766ee0
commit 742fd16
Show file tree

Hide file tree

Showing 10 changed files with 70 additions and 58 deletions.
diff --git a/scanner/metadata/metadata.go b/scanner/metadata/metadata.go
@@ -107,7 +107,7 @@ func (t Tags) Comment() string             { return t.getFirstTagValue("comment"
 func (t Tags) Lyrics() string {
 	return t.getFirstTagValue("lyrics", "lyrics-eng", "unsynced_lyrics", "unsynced lyrics", "unsyncedlyrics")
 }
-func (t Tags) Compilation() bool       { return t.getBool("tcmp", "compilation") }
+func (t Tags) Compilation() bool       { return t.getBool("tcmp", "compilation", "wm/iscompilation") }
 func (t Tags) TrackNumber() (int, int) { return t.getTuple("track", "tracknumber") }
 func (t Tags) DiscNumber() (int, int)  { return t.getTuple("disc", "discnumber") }
 func (t Tags) DiscSubtitle() string {

diff --git a/scanner/metadata/metadata_test.go b/scanner/metadata/metadata_test.go
@@ -16,9 +16,9 @@ var _ = Describe("Tags", func() {
 		})
 
 		It("correctly parses metadata from all files in folder", func() {
-			mds, err := metadata.Extract("tests/fixtures/test.mp3", "tests/fixtures/test.ogg")
+			mds, err := metadata.Extract("tests/fixtures/test.mp3", "tests/fixtures/test.ogg", "tests/fixtures/test.wma")
 			Expect(err).NotTo(HaveOccurred())
-			Expect(mds).To(HaveLen(2))
+			Expect(mds).To(HaveLen(3))
 
 			m := mds["tests/fixtures/test.mp3"]
 			Expect(m.Title()).To(Equal("Song"))
@@ -65,6 +65,17 @@ var _ = Describe("Tags", func() {
 			// TabLib 1.12 returns 18, previous versions return 39.
 			// See https://github.com/taglib/taglib/commit/2f238921824741b2cfe6fbfbfc9701d9827ab06b
 			Expect(m.BitRate()).To(BeElementOf(18, 39, 40, 49))
+
+			m = mds["tests/fixtures/test.wma"]
+			Expect(err).To(BeNil())
+			Expect(m.Compilation()).To(BeTrue())
+			Expect(m.Title()).To(Equal("Title"))
+			Expect(m.HasPicture()).To(BeFalse())
+			Expect(m.Duration()).To(BeNumerically("~", 1.02, 0.01))
+			Expect(m.Suffix()).To(Equal("wma"))
+			Expect(m.FilePath()).To(Equal("tests/fixtures/test.wma"))
+			Expect(m.Size()).To(Equal(int64(21431)))
+			Expect(m.BitRate()).To(BeElementOf(128))
 		})
 	})
 })
diff --git a/scanner/metadata/taglib/taglib_test.go b/scanner/metadata/taglib/taglib_test.go
@@ -72,7 +72,6 @@ var _ = Describe("Extractor", func() {
 			Expect(m).To(HaveKey("bitrate"))
 			Expect(m["bitrate"][0]).To(BeElementOf("18", "39", "40", "49"))
 		})
-
 		DescribeTable("Format-Specific tests",
 			func(file, duration, channels, albumGain, albumPeak, trackGain, trackPeak string) {
 				file = "tests/fixtures/" + file
@@ -91,15 +90,24 @@ var _ = Describe("Extractor", func() {
 				Expect(m).To(HaveKeyWithValue("album", []string{"Album", "Album"}))
 				Expect(m).To(HaveKeyWithValue("artist", []string{"Artist", "Artist"}))
 				Expect(m).To(HaveKeyWithValue("albumartist", []string{"Album Artist"}))
-				Expect(m).To(HaveKeyWithValue("compilation", []string{"1"}))
 				Expect(m).To(HaveKeyWithValue("genre", []string{"Rock"}))
 				Expect(m).To(HaveKeyWithValue("date", []string{"2014", "2014"}))
 
+				// Special for M4A, do not catch keys that have no actual name
+				Expect(m).ToNot(HaveKey(""))
+
 				Expect(m).To(HaveKey("discnumber"))
 				discno := m["discnumber"]
 				Expect(discno).To(HaveLen(1))
 				Expect(discno[0]).To(BeElementOf([]string{"1", "1/2"}))
 
+				// WMA does not have a "compilation" tag, but "wm/iscompilation"
+				if _, ok := m["compilation"]; ok {
+					Expect(m).To(HaveKeyWithValue("compilation", []string{"1"}))
+				} else {
+					Expect(m).To(HaveKeyWithValue("wm/iscompilation", []string{"1"}))
+				}
+
 				Expect(m).NotTo(HaveKeyWithValue("has_picture", []string{"true"}))
 				Expect(m).To(HaveKeyWithValue("duration", []string{duration}))
 
@@ -118,6 +126,7 @@ var _ = Describe("Extractor", func() {
 			Entry("correctly parses flac tags", "test.flac", "1.00", "1", "+4.06 dB", "0.12496948", "+4.06 dB", "0.12496948"),
 
 			Entry("Correctly parses m4a (aac) gain tags", "01 Invisible (RED) Edit Version.m4a", "1.04", "2", "0.37", "0.48", "0.37", "0.48"),
+			Entry("Correctly parses m4a (aac) gain tags (uppercase)", "test.m4a", "1.04", "2", "0.37", "0.48", "0.37", "0.48"),
 
 			Entry("correctly parses ogg (vorbis) tags", "test.ogg", "1.04", "2", "+7.64 dB", "0.11772506", "+7.64 dB", "0.11772506"),
 
@@ -133,7 +142,6 @@ var _ = Describe("Extractor", func() {
 
 			// ffmpeg -f lavfi -i "sine=frequency=1400:duration=1" test.aiff
 			//Entry("correctly parses aiff tags", "test.aiff", "1.00", "1", "2.00 dB", "0.124972", "2.00 dB", "0.124972"),
-
 		)
 	})
 

diff --git a/scanner/metadata/taglib/taglib_wrapper.cpp b/scanner/metadata/taglib/taglib_wrapper.cpp
@@ -15,13 +15,6 @@
 
 #include "taglib_wrapper.h"
 
-// Tags necessary for M4a parsing
-const char *RG_TAGS[] = {
-    "replaygain_album_gain",
-    "replaygain_album_peak",
-    "replaygain_track_gain",
-    "replaygain_track_peak"};
-
 char has_cover(const TagLib::FileRef f);
 
 int taglib_read(const FILENAME_CHAR_T *filename, unsigned long id) {
@@ -42,6 +35,7 @@ int taglib_read(const FILENAME_CHAR_T *filename, unsigned long id) {
   go_map_put_int(id, (char *)"bitrate", props->bitrate());
   go_map_put_int(id, (char *)"channels", props->channels());
 
+  // Create a map to collect all the tags
   TagLib::PropertyMap tags = f.file()->properties();
 
   // Make sure at least the basic properties are extracted
@@ -77,71 +71,49 @@ int taglib_read(const FILENAME_CHAR_T *filename, unsigned long id) {
     }
   }
 
+  // M4A may have some iTunes specific tags
   TagLib::MP4::File *m4afile(dynamic_cast<TagLib::MP4::File *>(f.file()));
-  if (m4afile != NULL)
-  {
-    const auto itemListMap = m4afile->tag();
-    {
-      char buf[200];
-
-      for (const char *key : RG_TAGS)
-      {
-        snprintf(buf, sizeof(buf), "----:com.apple.iTunes:%s", key);
-        const auto item = itemListMap->item(buf);
-        if (item.isValid())
-        {
-          char *dup = ::strdup(key);
-          char *val = ::strdup(item.toStringList().front().toCString(true));
-          go_map_put_str(id, dup, val);
-          free(dup);
-          free(val);
-        }
+  if (m4afile != NULL) {
+    const auto itemListMap = m4afile->tag()->itemMap();
+    for (const auto item: itemListMap) {
+      char *key = (char *)item.first.toCString(true);
+      for (const auto value: item.second.toStringList()) {
+        char *val = (char *)value.toCString(true);
+        go_map_put_m4a_str(id, key, val);
       }
     }
   }
 
   // WMA/ASF files may have additional tags not captured by the general iterator
   TagLib::ASF::File *asfFile(dynamic_cast<TagLib::ASF::File *>(f.file()));
-  if (asfFile != NULL) 
-  {
+  if (asfFile != NULL) {
     const TagLib::ASF::Tag *asfTags{asfFile->tag()};
     const auto itemListMap = asfTags->attributeListMap();
     for (const auto item : itemListMap) {
-      char *key = ::strdup(item.first.toCString(true));
-      char *val = ::strdup(item.second.front().toString().toCString());
-      go_map_put_str(id, key, val);
-      free(key);
-      free(val); 
-    }
-
-    // Compilation tag needs to be handled differently
-    const auto compilation = asfTags->attribute("WM/IsCompilation");
-    if (!compilation.isEmpty()) {
-      char *val = ::strdup(compilation.front().toString().toCString());
-      go_map_put_str(id, (char *)"compilation", val);
-      free(val);
+        tags.insert(item.first, item.second.front().toString());
     }
   }
 
-  if (has_cover(f)) {
-    go_map_put_str(id, (char *)"has_picture", (char *)"true");
-  }
-
+  // Send all collected tags to the Go map
   for (TagLib::PropertyMap::ConstIterator i = tags.begin(); i != tags.end();
        ++i) {
+    char *key = (char *)i->first.toCString(true);
     for (TagLib::StringList::ConstIterator j = i->second.begin();
          j != i->second.end(); ++j) {
-      char *key = ::strdup(i->first.toCString(true));
-      char *val = ::strdup((*j).toCString(true));
+      char *val = (char *)(*j).toCString(true);
       go_map_put_str(id, key, val);
-      free(key);
-      free(val);
     }
   }
 
+  // Cover art has to be handled separately
+  if (has_cover(f)) {
+    go_map_put_str(id, (char *)"has_picture", (char *)"true");
+  }
+
   return 0;
 }
 
+// Detect if the file has cover art. Returns 1 if the file has cover art, 0 otherwise.
 char has_cover(const TagLib::FileRef f) {
   char hasCover = 0;
   // ----- MP3

diff --git a/scanner/metadata/taglib/taglib_wrapper.go b/scanner/metadata/taglib/taglib_wrapper.go
@@ -23,6 +23,8 @@ import (
 	"github.com/navidrome/navidrome/log"
 )
 
+const iTunesKeyPrefix = "----:com.apple.itunes:"
+
 func Read(filename string) (tags map[string][]string, err error) {
 	// Do not crash on failures in the C code/library
 	debug.SetPanicOnFault(true)
@@ -79,14 +81,31 @@ func deleteMap(id uint32) {
 	delete(maps, id)
 }
 
+//export go_map_put_m4a_str
+func go_map_put_m4a_str(id C.ulong, key *C.char, val *C.char) {
+	k := strings.ToLower(C.GoString(key))
+
+	// Special for M4A, do not catch keys that have no actual name
+	k = strings.TrimPrefix(k, iTunesKeyPrefix)
+	do_put_map(id, k, val)
+}
+
 //export go_map_put_str
 func go_map_put_str(id C.ulong, key *C.char, val *C.char) {
+	k := strings.ToLower(C.GoString(key))
+	do_put_map(id, k, val)
+}
+
+func do_put_map(id C.ulong, key string, val *C.char) {
+	if key == "" {
+		return
+	}
+
 	lock.RLock()
 	defer lock.RUnlock()
 	m := maps[uint32(id)]
-	k := strings.ToLower(C.GoString(key))
 	v := strings.TrimSpace(C.GoString(val))
-	m[k] = append(m[k], v)
+	m[key] = append(m[key], v)
 }
 
 //export go_map_put_int

diff --git a/scanner/metadata/taglib/taglib_wrapper.h b/scanner/metadata/taglib/taglib_wrapper.h
@@ -11,6 +11,7 @@ extern "C" {
 #define FILENAME_CHAR_T char
 #endif
 
+extern void go_map_put_m4a_str(unsigned long id, char *key, char *val);
 extern void go_map_put_str(unsigned long id, char *key, char *val);
 extern void go_map_put_int(unsigned long id, char *key, int val);
 int taglib_read(const FILENAME_CHAR_T *filename, unsigned long id);

diff --git a/scanner/tag_scanner_test.go b/scanner/tag_scanner_test.go
@@ -10,9 +10,10 @@ var _ = Describe("TagScanner", func() {
 		It("return all audio files from the folder", func() {
 			files, err := loadAllAudioFiles("tests/fixtures")
 			Expect(err).ToNot(HaveOccurred())
-			Expect(files).To(HaveLen(10))
+			Expect(files).To(HaveLen(11))
 			Expect(files).To(HaveKey("tests/fixtures/test.aiff"))
 			Expect(files).To(HaveKey("tests/fixtures/test.flac"))
+			Expect(files).To(HaveKey("tests/fixtures/test.m4a"))
 			Expect(files).To(HaveKey("tests/fixtures/test.mp3"))
 			Expect(files).To(HaveKey("tests/fixtures/test.ogg"))
 			Expect(files).To(HaveKey("tests/fixtures/test.wav"))

diff --git a/scanner/walk_dir_tree_test.go b/scanner/walk_dir_tree_test.go
@@ -34,7 +34,7 @@ var _ = Describe("walk_dir_tree", func() {
 			Expect(collected[baseDir]).To(MatchFields(IgnoreExtras, Fields{
 				"Images":          BeEmpty(),
 				"HasPlaylist":     BeFalse(),
-				"AudioFilesCount": BeNumerically("==", 11),
+				"AudioFilesCount": BeNumerically("==", 12),
 			}))
 			Expect(collected[filepath.Join(baseDir, "artist", "an-album")]).To(MatchFields(IgnoreExtras, Fields{
 				"Images":          ConsistOf("cover.jpg", "front.png", "artist.png"),

diff --git a/tests/fixtures/01 Invisible (RED) Edit Version.m4a b/tests/fixtures/01 Invisible (RED) Edit Version.m4a
diff --git a/tests/fixtures/test.m4a b/tests/fixtures/test.m4a