Ilst: Change rules for gnre upgrades

CHANGELOG.md

@@ -26,6 +26,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
     - It can be converted to and from a `u32`
   - `AtomData::data_type()` to get the data type code of the atom content.
 
+### Changed
+- **Ilst**: Add new rules for `gnre` atom upgrades ([issue](https://github.com/Serial-ATA/lofty-rs/issues/409)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/485))
+  - In the case that a `©gen` and `gnre` atom are present in a file, there was no way to tell which `©gen` atoms were upgraded.
+    the new rules are:
+    - `gnre` present + no `©gen` present, `gnre` gets upgraded as normal
+    - `gnre` present + `©gen` present, `©gen` takes precedence and `gnre` is discarded
+      - With [ParsingOptions::implicit_conversions](https://docs.rs/lofty/latest/lofty/config/struct.ParseOptions.html#method.implicit_conversions)
+        set to `false`, `gnre` will be retained as an atom of type `Unknown`.
+
 ### Fixed
 - **MusePack**: Fix potential panic when the beginning silence makes up the entire sample count ([PR](https://github.com/Serial-ATA/lofty-rs/pull/449))
 - **Timestamp**:

lofty/src/mp4/ilst/atom.rs

@@ -28,6 +28,14 @@ impl AtomDataStorage {
 			AtomDataStorage::Multiple(v) => v.iter().all(|p| matches!(p, AtomData::Picture(_))),
 		}
 	}
+
+	pub(super) fn from_vec(mut v: Vec<AtomData>) -> Option<Self> {
+		match v.len() {
+			0 => None,
+			1 => Some(AtomDataStorage::Single(v.remove(0))),
+			_ => Some(AtomDataStorage::Multiple(v)),
+		}
+	}
 }
 
 impl Debug for AtomDataStorage {

lofty/src/mp4/ilst/mod.rs

@@ -892,22 +892,24 @@ mod tests {
 
 	use std::io::{Cursor, Read as _, Seek as _, Write as _};
 
-	fn read_ilst(path: &str, parse_mode: ParsingMode) -> Ilst {
+	fn read_ilst(path: &str, parse_options: ParseOptions) -> Ilst {
 		let tag = std::fs::read(path).unwrap();
-		read_ilst_raw(&tag, parse_mode)
+		read_ilst_raw(&tag, parse_options)
 	}
 
-	fn read_ilst_raw(bytes: &[u8], parse_mode: ParsingMode) -> Ilst {
-		let options = ParseOptions::new().parsing_mode(parse_mode);
-		read_ilst_with_options(bytes, options)
+	fn read_ilst_raw(bytes: &[u8], parse_options: ParseOptions) -> Ilst {
+		read_ilst_with_options(bytes, parse_options)
 	}
 
 	fn read_ilst_strict(path: &str) -> Ilst {
-		read_ilst(path, ParsingMode::Strict)
+		read_ilst(path, ParseOptions::new().parsing_mode(ParsingMode::Strict))
 	}
 
 	fn read_ilst_bestattempt(path: &str) -> Ilst {
-		read_ilst(path, ParsingMode::BestAttempt)
+		read_ilst(
+			path,
+			ParseOptions::new().parsing_mode(ParsingMode::BestAttempt),
+		)
 	}
 
 	fn read_ilst_with_options(bytes: &[u8], parse_options: ParseOptions) -> Ilst {
@@ -1427,7 +1429,10 @@ mod tests {
 
 		tag_bytes.drain(..8); // Remove the ilst identifier and size for `read_ilst`
 
-		let tag_re_read = read_ilst_raw(&tag_bytes[..], ParsingMode::Strict);
+		let tag_re_read = read_ilst_raw(
+			&tag_bytes[..],
+			ParseOptions::new().parsing_mode(ParsingMode::Strict),
+		);
 		assert_eq!(tag, tag_re_read);
 
 		// Now write from `Tag`
@@ -1439,7 +1444,10 @@ mod tests {
 
 		tag_bytes.drain(..8); // Remove the ilst identifier and size for `read_ilst`
 
-		let generic_tag_re_read = read_ilst_raw(&tag_bytes[..], ParsingMode::Strict);
+		let generic_tag_re_read = read_ilst_raw(
+			&tag_bytes[..],
+			ParseOptions::new().parsing_mode(ParsingMode::Strict),
+		);
 		assert_eq!(tag_re_read, generic_tag_re_read);
 	}
 
@@ -1465,4 +1473,49 @@ mod tests {
 		assert_eq!(ilst.len(), 1); // Artist, no picture
 		assert!(ilst.artist().is_some());
 	}
+
+	#[test_log::test]
+	fn gnre_conversion_case_1() {
+		// Case 1: 1 `gnre` atom present, no `©gen` present. `gnre` gets converted without issue.
+		let ilst = read_ilst_bestattempt("tests/tags/assets/ilst/gnre_conversion_case_1.ilst");
+
+		assert_eq!(ilst.len(), 2);
+		assert_eq!(ilst.artist().unwrap(), "Foo artist"); // Sanity check
+		assert_eq!(ilst.genre().unwrap(), "Funk");
+	}
+
+	#[test_log::test]
+	fn gnre_conversion_case_2() {
+		// Case 2: 1 `gnre` atom present, 1 `©gen` present. `gnre` gets discarded.
+		let ilst = read_ilst_bestattempt("tests/tags/assets/ilst/gnre_conversion_case_2.ilst");
+
+		assert_eq!(ilst.len(), 2);
+		assert_eq!(ilst.artist().unwrap(), "Foo artist"); // Sanity check
+		assert_eq!(ilst.genre().unwrap(), "My Custom Genre");
+	}
+
+	#[test_log::test]
+	fn gnre_conversion_case_3() {
+		// Case 2: 1 `gnre` atom present, 1 `©gen` present. implicit conversion are disabled, `gnre` is retained
+		//         as an unknown atom.
+		let ilst = read_ilst(
+			"tests/tags/assets/ilst/gnre_conversion_case_2.ilst",
+			ParseOptions::new().implicit_conversions(false),
+		);
+
+		assert_eq!(ilst.len(), 3);
+		assert_eq!(ilst.artist().unwrap(), "Foo artist"); // Sanity check
+		assert_eq!(ilst.genre().unwrap(), "My Custom Genre");
+		assert_eq!(
+			ilst.get(&AtomIdent::Fourcc(*b"gnre"))
+				.unwrap()
+				.data()
+				.next()
+				.unwrap(),
+			&AtomData::Unknown {
+				code: DataType::BeSignedInteger,
+				data: vec![0, 6]
+			}
+		);
+	}
 }

lofty/src/mp4/ilst/read.rs

@@ -9,6 +9,7 @@ use crate::mp4::atom_info::{AtomInfo, ATOM_HEADER_LEN};
 use crate::mp4::ilst::atom::AtomDataStorage;
 use crate::mp4::read::{skip_atom, AtomReader};
 use crate::picture::{MimeType, Picture, PictureType};
+use crate::tag::TagExt;
 use crate::util::text::{utf16_decode_bytes, utf8_decode};
 
 use std::borrow::Cow;
@@ -33,6 +34,7 @@ where
 
 	let mut tag = Ilst::default();
 
+	let mut upgraded_gnres = Vec::new();
 	while let Ok(Some(atom)) = ilst_reader.next() {
 		if let AtomIdent::Fourcc(ref fourcc) = atom.ident {
 			match fourcc {
@@ -50,31 +52,44 @@ where
 					continue;
 				},
 				// Upgrade this to a \xa9gen atom
-				b"gnre" => {
+				b"gnre" if parse_options.implicit_conversions => {
 					log::warn!("Encountered outdated 'gnre' atom, attempting to upgrade to '©gen'");
 
 					if let Some(atom_data) =
 						parse_data_inner(&mut ilst_reader, parsing_mode, &atom)?
 					{
-						let mut data = Vec::new();
-
 						for (_, content) in atom_data {
 							if content.len() >= 2 {
 								let index = content[1] as usize;
 								if index > 0 && index <= GENRES.len() {
-									data.push(AtomData::UTF8(String::from(GENRES[index - 1])));
+									upgraded_gnres
+										.push(AtomData::UTF8(String::from(GENRES[index - 1])));
 								}
 							}
 						}
+					}
 
-						if !data.is_empty() {
-							let storage = match data.len() {
-								1 => AtomDataStorage::Single(data.remove(0)),
-								_ => AtomDataStorage::Multiple(data),
-							};
+					continue;
+				},
+				// Just insert these normally, the caller will deal with them (hopefully)
+				b"gnre" => {
+					log::warn!("Encountered outdated 'gnre' atom");
+
+					if let Some(atom_data) =
+						parse_data_inner(&mut ilst_reader, parsing_mode, &atom)?
+					{
+						let mut data = Vec::new();
+
+						for (code, content) in atom_data {
+							data.push(AtomData::Unknown {
+								code,
+								data: content,
+							});
+						}
 
+						if let Some(storage) = AtomDataStorage::from_vec(data) {
 							tag.atoms.push(Atom {
-								ident: AtomIdent::Fourcc(*b"\xa9gen"),
+								ident: AtomIdent::Fourcc(*b"gnre"),
 								data: storage,
 							})
 						}
@@ -139,6 +154,20 @@ where
 		parse_data(&mut ilst_reader, parsing_mode, &mut tag, atom)?;
 	}
 
+	if parse_options.implicit_conversions && !upgraded_gnres.is_empty() {
+		if tag.contains(&AtomIdent::Fourcc(*b"\xa9gen")) {
+			log::warn!("Encountered '©gen' atom, discarding upgraded 'gnre' atom(s)");
+			return Ok(tag);
+		}
+
+		if let Some(storage) = AtomDataStorage::from_vec(upgraded_gnres) {
+			tag.atoms.push(Atom {
+				ident: AtomIdent::Fourcc(*b"\xa9gen"),
+				data: storage,
+			})
+		}
+	}
+
 	Ok(tag)
 }