ID3v2.3: Fix some writing issues

* TDAT is DDMM, was previously written as MMDD
* TCON conversion will properly handle refinements
* TIPL and TMCL are now merged into IPLS

Author: Serial-ATA

lofty/src/id3/v2/read.rs

@@ -87,7 +87,7 @@ fn construct_tdrc_from_v3(tag: &mut Id3v2Tag) {
 				break 'build;
 			}
 
-			let (Ok(month), Ok(day)) = (date[..2].parse::<u8>(), date[2..].parse::<u8>()) else {
+			let (Ok(day), Ok(month)) = (date[..2].parse::<u8>(), date[2..].parse::<u8>()) else {
 				log::warn!("Invalid TDAT frame, retaining.");
 				break 'build;
 			};

lofty/src/id3/v2/tag.rs

@@ -547,7 +547,7 @@ impl Id3v2Tag {
 	/// ID3v2.4-style multi-value fields will be split as normal.
 	pub fn genres(&self) -> Option<impl Iterator<Item = &str>> {
 		if let Some(Frame::Text(TextInformationFrame { ref value, .. })) = self.get(&GENRE_ID) {
-			return Some(GenresIter::new(value));
+			return Some(GenresIter::new(value, false));
 		}
 
 		None
@@ -570,11 +570,16 @@ impl Id3v2Tag {
 pub(crate) struct GenresIter<'a> {
 	value: &'a str,
 	pos: usize,
+	preserve_indexes: bool,
 }
 
 impl<'a> GenresIter<'a> {
-	pub fn new(value: &'a str) -> GenresIter<'_> {
-		GenresIter { value, pos: 0 }
+	pub fn new(value: &'a str, preserve_indexes: bool) -> GenresIter<'_> {
+		GenresIter {
+			value,
+			pos: 0,
+			preserve_indexes,
+		}
 	}
 }
 
@@ -592,7 +597,7 @@ impl<'a> Iterator for GenresIter<'a> {
 			let start = self.pos;
 			let end = self.pos + idx;
 			self.pos = end + 1;
-			return Some(parse_genre(&self.value[start..end]));
+			return Some(parse_genre(&self.value[start..end], self.preserve_indexes));
 		}
 
 		if remainder.starts_with('(') && remainder.contains(')') {
@@ -603,20 +608,20 @@ impl<'a> Iterator for GenresIter<'a> {
 			if remainder.starts_with("((") {
 				end += 1;
 			}
-			return Some(parse_genre(&self.value[start..end]));
+			return Some(parse_genre(&self.value[start..end], self.preserve_indexes));
 		}
 
 		self.pos = self.value.len();
-		Some(parse_genre(remainder))
+		Some(parse_genre(remainder, self.preserve_indexes))
 	}
 }
 
-fn parse_genre(genre: &str) -> &str {
+fn parse_genre(genre: &str, preserve_indexes: bool) -> &str {
 	if genre.len() > 3 {
 		return genre;
 	}
 	if let Ok(id) = genre.parse::<usize>() {
-		if id < GENRES.len() {
+		if id < GENRES.len() && !preserve_indexes {
 			GENRES[id]
 		} else {
 			genre
@@ -1047,7 +1052,7 @@ fn handle_tag_split(tag: &mut Tag, frame: &mut Frame<'_>) -> bool {
 			value: content,
 			..
 		}) if id.as_str() == "TCON" => {
-			let genres = GenresIter::new(content);
+			let genres = GenresIter::new(content, false);
 			for genre in genres {
 				tag.items.push(TagItem::new(
 					ItemKey::Genre,

lofty/src/id3/v2/tag/tests.rs

@@ -1527,7 +1527,7 @@ fn split_tdrc_on_id3v23_save() {
 	let date = tag_re_read
 		.get_text(&FrameId::Valid(Cow::Borrowed("TDAT")))
 		.expect("Expected TDAT frame");
-	assert_eq!(date, "0603");
+	assert_eq!(date, "0306");
 
 	let time = tag_re_read
 		.get_text(&FrameId::Valid(Cow::Borrowed("TIME")))

lofty/src/id3/v2/write/frame.rs

@@ -1,12 +1,12 @@
 use crate::error::{Id3v2Error, Id3v2ErrorKind, Result};
 use crate::id3::v2::frame::{FrameFlags, FrameRef};
-use crate::id3::v2::tag::GenresIter;
 use crate::id3::v2::util::synchsafe::SynchsafeInteger;
-use crate::id3::v2::{Frame, FrameId, TextInformationFrame};
+use crate::id3::v2::{Frame, FrameId, KeyValueFrame, TextInformationFrame};
 use crate::tag::items::Timestamp;
 
 use std::io::Write;
 
+use crate::id3::v2::tag::GenresIter;
 use byteorder::{BigEndian, WriteBytesExt};
 
 pub(in crate::id3::v2) fn create_items<W>(
@@ -43,12 +43,15 @@ where
 {
 	// These are all frames from ID3v2.4
 	const FRAMES_TO_DISCARD: &[&str] = &[
-		"ASPI", "EQU2", "RVA2", "SEEK", "SIGN", "TDEN", "TDRL", "TDTG", "TIPL", "TMCL", "TMOO",
-		"TPRO", "TSOA", "TSOP", "TSOT", "TSST",
+		"ASPI", "EQU2", "RVA2", "SEEK", "SIGN", "TDEN", "TDRL", "TDTG", "TMOO", "TPRO", "TSOA",
+		"TSOP", "TSOT", "TSST",
 	];
 
+	const IPLS_ID: &str = "IPLS";
+
 	let is_id3v23 = true;
 
+	let mut ipls = None;
 	for mut frame in frames {
 		let id = frame.id_str();
 
@@ -99,7 +102,7 @@ where
 					)));
 
 					if let (Some(month), Some(day)) = (timestamp.month, timestamp.day) {
-						let date = format!("{:02}{:02}", month, day);
+						let date = format!("{:02}{:02}", day, month);
 						new_frames.push(Frame::Text(TextInformationFrame::new(
 							FrameId::Valid("TDAT".into()),
 							f.encoding.to_id3v23(),
@@ -142,19 +145,63 @@ where
 				};
 
 				let mut new_genre_string = String::new();
-				for genre in GenresIter::new(&f.value) {
-					match genre {
+				let genres = GenresIter::new(&f.value, true).collect::<Vec<_>>();
+				for (i, genre) in genres.iter().enumerate() {
+					match *genre {
 						"Remix" => new_genre_string.push_str("(RX)"),
 						"Cover" => new_genre_string.push_str("(CR)"),
-						_ => {
+						_ if i == genres.len() - 1 && genre.parse::<u8>().is_err() => {
 							new_genre_string.push_str(genre);
 						},
+						_ => {
+							new_genre_string.push_str(&format!("({genre})"));
+						},
 					}
 				}
 
 				f.value = new_genre_string;
 				frame.0 = value;
 			},
+			// TIPL (Involved people list) and TMCL (Musician credits list) are
+			// both key-value pairs. ID3v2.3 does not distinguish between the two,
+			// so we must merge them into a single IPLS frame.
+			"TIPL" | "TMCL" => {
+				let mut value = frame.0.clone();
+				let Frame::KeyValue(KeyValueFrame {
+					ref mut key_value_pairs,
+					encoding,
+					..
+				}) = value.to_mut()
+				else {
+					log::warn!("Discarding frame: {}, not supported in ID3v2.3", id);
+					continue;
+				};
+
+				let ipls_frame;
+				match ipls {
+					Some(ref mut frame) => {
+						ipls_frame = frame;
+					},
+					None => {
+						ipls = Some(TextInformationFrame::new(
+							FrameId::Valid("IPLS".into()),
+							encoding.to_id3v23(),
+							String::new(),
+						));
+						ipls_frame = ipls.as_mut().unwrap();
+					},
+				}
+
+				for (key, value) in key_value_pairs.drain(..) {
+					if !ipls_frame.value.is_empty() {
+						ipls_frame.value.push('\0');
+					}
+
+					ipls_frame.value.push_str(&format!("{}\0{}", key, value));
+				}
+
+				continue;
+			},
 			_ => {},
 		}
 
@@ -169,6 +216,12 @@ where
 		)?;
 	}
 
+	if let Some(ipls) = ipls {
+		let frame = Frame::Text(ipls);
+		let value = frame.as_bytes(is_id3v23)?;
+		write_frame(writer, IPLS_ID, frame.flags(), &value, is_id3v23)?;
+	}
+
 	Ok(())
 }
 
@@ -252,10 +305,14 @@ where
 		);
 	}
 
-	if let Some(len) = flags.data_length_indicator {
+	if let Some(mut len) = flags.data_length_indicator {
 		if len > 0 {
 			write_frame_header(writer, name, (value.len() + 1) as u32, flags, is_id3v23)?;
-			writer.write_u32::<BigEndian>(len.synch()?)?;
+			if !is_id3v23 {
+				len = len.synch()?;
+			}
+
+			writer.write_u32::<BigEndian>(len)?;
 			writer.write_u8(method_symbol)?;
 			writer.write_all(value)?;
 
@@ -282,11 +339,11 @@ where
 		flags.as_id3v24_bytes()
 	};
 
+	writer.write_all(name.as_bytes())?;
 	if !is_id3v23 {
 		len = len.synch()?;
 	}
 
-	writer.write_all(name.as_bytes())?;
 	writer.write_u32::<BigEndian>(len)?;
 	writer.write_u16::<BigEndian>(flags)?;