Split and rejoin tags for read/modify/write round trips

src/ape/tag/mod.rs

@@ -6,7 +6,7 @@ use crate::ape::tag::item::{ApeItem, ApeItemRef};
 use crate::error::{LoftyError, Result};
 use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
-use crate::traits::{Accessor, TagExt};
+use crate::traits::{Accessor, SplitAndRejoinTag, TagExt};
 
 use std::borrow::Cow;
 use std::convert::TryInto;
@@ -289,8 +289,8 @@ impl TagExt for ApeTag {
 	}
 }
 
-impl From<ApeTag> for Tag {
-	fn from(input: ApeTag) -> Self {
+impl SplitAndRejoinTag for ApeTag {
+	fn split_tag(&mut self) -> Tag {
 		fn split_pair(
 			content: &str,
 			tag: &mut Tag,
@@ -312,7 +312,7 @@ impl From<ApeTag> for Tag {
 
 		let mut tag = Tag::new(TagType::APE);
 
-		for item in input.items {
+		for item in std::mem::take(&mut self.items) {
 			let item_key = ItemKey::from_key(TagType::APE, item.key());
 
 			// The text pairs need some special treatment
@@ -321,13 +321,13 @@ impl From<ApeTag> for Tag {
 					if split_pair(val, &mut tag, ItemKey::TrackNumber, ItemKey::TrackTotal)
 						.is_some() =>
 				{
-					continue
+					continue; // Item consumed
 				},
 				(ItemKey::DiscNumber | ItemKey::DiscTotal, ItemValue::Text(val))
 					if split_pair(val, &mut tag, ItemKey::DiscNumber, ItemKey::DiscTotal)
 						.is_some() =>
 				{
-					continue
+					continue; // Item consumed
 				},
 				(ItemKey::MovementNumber | ItemKey::MovementTotal, ItemValue::Text(val))
 					if split_pair(
@@ -338,36 +338,46 @@ impl From<ApeTag> for Tag {
 					)
 					.is_some() =>
 				{
-					continue
+					continue; // Item consumed
+				},
+				(k, _) => {
+					tag.items.push(TagItem::new(k, item.value));
 				},
-				(k, _) => tag.items.push(TagItem::new(k, item.value)),
 			}
 		}
 
 		tag
 	}
+
+	fn rejoin_tag(&mut self, tag: Tag) {
+		for item in tag.items {
+			if let Ok(ape_item) = item.try_into() {
+				self.insert(ape_item)
+			}
+		}
+
+		for pic in tag.pictures {
+			if let Some(key) = pic.pic_type.as_ape_key() {
+				if let Ok(item) =
+					ApeItem::new(key.to_string(), ItemValue::Binary(pic.as_ape_bytes()))
+				{
+					self.insert(item)
+				}
+			}
+		}
+	}
+}
+
+impl From<ApeTag> for Tag {
+	fn from(mut input: ApeTag) -> Self {
+		input.split_tag()
+	}
 }
 
 impl From<Tag> for ApeTag {
 	fn from(input: Tag) -> Self {
 		let mut ape_tag = Self::default();
-
-		for item in input.items {
-			if let Ok(ape_item) = item.try_into() {
-				ape_tag.insert(ape_item)
-			}
-		}
-
-		for pic in input.pictures {
-			if let Some(key) = pic.pic_type.as_ape_key() {
-				if let Ok(item) =
-					ApeItem::new(key.to_string(), ItemValue::Binary(pic.as_ape_bytes()))
-				{
-					ape_tag.insert(item)
-				}
-			}
-		}
-
+		ape_tag.rejoin_tag(input);
 		ape_tag
 	}
 }

src/id3/v1/tag.rs

@@ -2,7 +2,7 @@ use crate::error::{LoftyError, Result};
 use crate::id3::v1::constants::GENRES;
 use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
-use crate::traits::{Accessor, TagExt};
+use crate::traits::{Accessor, SplitAndRejoinTag, TagExt};
 
 use std::borrow::Cow;
 use std::fs::{File, OpenOptions};
@@ -238,26 +238,32 @@ impl TagExt for ID3v1Tag {
 	}
 }
 
-impl From<ID3v1Tag> for Tag {
-	fn from(input: ID3v1Tag) -> Self {
-		let mut tag = Self::new(TagType::ID3v1);
+impl SplitAndRejoinTag for ID3v1Tag {
+	fn split_tag(&mut self) -> Tag {
+		let mut tag = Tag::new(TagType::ID3v1);
 
-		input.title.map(|t| tag.insert_text(ItemKey::TrackTitle, t));
-		input
-			.artist
+		self.title
+			.take()
+			.map(|t| tag.insert_text(ItemKey::TrackTitle, t));
+		self.artist
+			.take()
 			.map(|a| tag.insert_text(ItemKey::TrackArtist, a));
-		input.album.map(|a| tag.insert_text(ItemKey::AlbumTitle, a));
-		input.year.map(|y| tag.insert_text(ItemKey::Year, y));
-		input.comment.map(|c| tag.insert_text(ItemKey::Comment, c));
+		self.album
+			.take()
+			.map(|a| tag.insert_text(ItemKey::AlbumTitle, a));
+		self.year.take().map(|y| tag.insert_text(ItemKey::Year, y));
+		self.comment
+			.take()
+			.map(|c| tag.insert_text(ItemKey::Comment, c));
 
-		if let Some(t) = input.track_number {
+		if let Some(t) = self.track_number.take() {
 			tag.items.push(TagItem::new(
 				ItemKey::TrackNumber,
 				ItemValue::Text(t.to_string()),
 			))
 		}
 
-		if let Some(genre_index) = input.genre {
+		if let Some(genre_index) = self.genre.take() {
 			if let Some(genre) = GENRES.get(genre_index as usize) {
 				tag.insert_text(ItemKey::Genre, (*genre).to_string());
 			}
@@ -265,6 +271,16 @@ impl From<ID3v1Tag> for Tag {
 
 		tag
 	}
+
+	fn rejoin_tag(&mut self, tag: Tag) {
+		*self = tag.into();
+	}
+}
+
+impl From<ID3v1Tag> for Tag {
+	fn from(mut input: ID3v1Tag) -> Self {
+		input.split_tag()
+	}
 }
 
 impl From<Tag> for ID3v1Tag {

src/id3/v2/tag.rs

@@ -6,10 +6,10 @@ use crate::error::{LoftyError, Result};
 use crate::id3::v2::frame::FrameRef;
 use crate::id3::v2::items::encoded_text_frame::EncodedTextFrame;
 use crate::id3::v2::items::language_frame::LanguageFrame;
-use crate::picture::{Picture, PictureType};
+use crate::picture::{Picture, PictureType, TOMBSTONE_PICTURE};
 use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
-use crate::traits::{Accessor, TagExt};
+use crate::traits::{Accessor, SplitAndRejoinTag, TagExt};
 use crate::util::text::TextEncoding;
 
 use std::borrow::Cow;
@@ -533,8 +533,8 @@ impl TagExt for ID3v2Tag {
 	}
 }
 
-impl From<ID3v2Tag> for Tag {
-	fn from(input: ID3v2Tag) -> Self {
+impl SplitAndRejoinTag for ID3v2Tag {
+	fn split_tag(&mut self) -> Tag {
 		fn split_pair(
 			content: &str,
 			tag: &mut Tag,
@@ -555,13 +555,13 @@ impl From<ID3v2Tag> for Tag {
 			Some(())
 		}
 
-		let mut tag = Self::new(TagType::ID3v2);
+		let mut tag = Tag::new(TagType::ID3v2);
 
-		for frame in input.frames {
-			let id = frame.id;
+		self.frames.retain_mut(|frame| {
+			let id = &frame.id;
 
 			// The text pairs need some special treatment
-			match (id.as_str(), frame.value) {
+			match (id.as_str(), &mut frame.value) {
 				("TRCK", FrameValue::Text { value: content, .. })
 					if split_pair(
 						&content,
@@ -571,13 +571,13 @@ impl From<ID3v2Tag> for Tag {
 					)
 					.is_some() =>
 				{
-					continue
+					false // Frame consumed
 				},
 				("TPOS", FrameValue::Text { value: content, .. })
 					if split_pair(&content, &mut tag, ItemKey::DiscNumber, ItemKey::DiscTotal)
 						.is_some() =>
 				{
-					continue
+					false // Frame consumed
 				},
 				("MVIN", FrameValue::Text { value: content, .. })
 					if split_pair(
@@ -588,7 +588,7 @@ impl From<ID3v2Tag> for Tag {
 					)
 					.is_some() =>
 				{
-					continue
+					false // Frame consumed
 				},
 				// Store TXXX/WXXX frames by their descriptions, rather than their IDs
 				(
@@ -606,6 +606,7 @@ impl From<ID3v2Tag> for Tag {
 							ItemValue::Text(c.to_string()),
 						));
 					}
+					false // Frame consumed
 				},
 				(
 					"WXXX",
@@ -622,6 +623,7 @@ impl From<ID3v2Tag> for Tag {
 							ItemValue::Locator(c.to_string()),
 						));
 					}
+					false // Frame consumed
 				},
 				(id, value) => {
 					let item_key = ItemKey::from_key(TagType::ID3v2, id);
@@ -642,13 +644,14 @@ impl From<ID3v2Tag> for Tag {
 							description,
 							..
 						}) => {
-							if description == EMPTY_CONTENT_DESCRIPTOR {
+							if *description == EMPTY_CONTENT_DESCRIPTOR {
 								for c in content.split(V4_MULTI_VALUE_SEPARATOR) {
 									tag.items.push(TagItem::new(
 										item_key.clone(),
 										ItemValue::Text(c.to_string()),
 									));
 								}
+								return false; // Frame consumed
 							}
 							// ...else do not convert text frames with a non-empty content
 							// descriptor that would otherwise unintentionally be modified
@@ -656,8 +659,7 @@ impl From<ID3v2Tag> for Tag {
 							// TODO: How to convert these frames consistently and safely
 							// such that the content descriptor is preserved during read/write
 							// round trips?
-
-							continue;
+							return true; // Keep frame
 						},
 						FrameValue::Text { value: content, .. } => {
 							for c in content.split(V4_MULTI_VALUE_SEPARATOR) {
@@ -666,34 +668,34 @@ impl From<ID3v2Tag> for Tag {
 									ItemValue::Text(c.to_string()),
 								));
 							}
-
-							continue;
+							return false; // Frame consumed
 						},
 						FrameValue::URL(content)
-						| FrameValue::UserURL(EncodedTextFrame { content, .. }) => ItemValue::Locator(content),
+						| FrameValue::UserURL(EncodedTextFrame { content, .. }) => {
+							ItemValue::Locator(std::mem::take(content))
+						},
 						FrameValue::Picture { picture, .. } => {
-							tag.push_picture(picture);
-							continue;
+							tag.push_picture(std::mem::replace(picture, TOMBSTONE_PICTURE));
+							return false; // Frame consumed
 						},
 						FrameValue::Popularimeter(popularimeter) => {
 							ItemValue::Binary(popularimeter.as_bytes())
 						},
-						FrameValue::Binary(binary) => ItemValue::Binary(binary),
+						FrameValue::Binary(binary) => ItemValue::Binary(std::mem::take(binary)),
 					};
 
 					tag.items.push(TagItem::new(item_key, item_value));
+					false // Frame consumed
 				},
 			}
-		}
+		});
 
 		tag
 	}
-}
 
-impl From<Tag> for ID3v2Tag {
-	fn from(mut input: Tag) -> Self {
-		fn join_items(input: &mut Tag, key: &ItemKey) -> String {
-			let mut iter = input.take_strings(key);
+	fn rejoin_tag(&mut self, mut tag: Tag) {
+		fn join_items(tag: &mut Tag, key: &ItemKey) -> String {
+			let mut iter = tag.take_strings(key);
 
 			match iter.next() {
 				None => String::new(),
@@ -710,25 +712,22 @@ impl From<Tag> for ID3v2Tag {
 			}
 		}
 
-		let mut id3v2_tag = ID3v2Tag {
-			frames: Vec::with_capacity(input.item_count() as usize),
-			..ID3v2Tag::default()
-		};
+		self.frames.reserve(tag.item_count() as usize);
 
-		let artists = join_items(&mut input, &ItemKey::TrackArtist);
-		id3v2_tag.set_artist(artists);
+		let artists = join_items(&mut tag, &ItemKey::TrackArtist);
+		self.set_artist(artists);
 
-		for item in input.items {
+		for item in tag.items {
 			let frame: Frame<'_> = match item.into() {
 				Some(frame) => frame,
 				None => continue,
 			};
 
-			id3v2_tag.insert(frame);
+			self.insert(frame);
 		}
 
-		for picture in input.pictures {
-			id3v2_tag.frames.push(Frame {
+		for picture in tag.pictures {
+			self.frames.push(Frame {
 				id: FrameID::Valid(Cow::Borrowed("APIC")),
 				value: FrameValue::Picture {
 					encoding: TextEncoding::UTF8,
@@ -737,7 +736,19 @@ impl From<Tag> for ID3v2Tag {
 				flags: FrameFlags::default(),
 			})
 		}
+	}
+}
 
+impl From<ID3v2Tag> for Tag {
+	fn from(mut input: ID3v2Tag) -> Self {
+		input.split_tag()
+	}
+}
+
+impl From<Tag> for ID3v2Tag {
+	fn from(input: Tag) -> Self {
+		let mut id3v2_tag = ID3v2Tag::default();
+		id3v2_tag.rejoin_tag(input);
 		id3v2_tag
 	}
 }

src/iff/aiff/tag.rs

@@ -3,7 +3,7 @@ use crate::iff::chunk::Chunks;
 use crate::macros::err;
 use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
-use crate::traits::{Accessor, TagExt};
+use crate::traits::{Accessor, SplitAndRejoinTag, TagExt};
 
 use std::borrow::Cow;
 use std::convert::TryFrom;
@@ -216,9 +216,19 @@ impl TagExt for AIFFTextChunks {
 	}
 }
 
+impl SplitAndRejoinTag for AIFFTextChunks {
+	fn split_tag(&mut self) -> Tag {
+		std::mem::take(self).into()
+	}
+
+	fn rejoin_tag(&mut self, tag: Tag) {
+		*self = tag.into();
+	}
+}
+
 impl From<AIFFTextChunks> for Tag {
 	fn from(input: AIFFTextChunks) -> Self {
-		let mut tag = Tag::new(TagType::AIFFText);
+		let mut tag = Self::new(TagType::AIFFText);
 
 		let push_item = |field: Option<String>, item_key: ItemKey, tag: &mut Tag| {
 			if let Some(text) = field {

src/iff/wav/tag/mod.rs

@@ -4,7 +4,7 @@ mod write;
 use crate::error::{LoftyError, Result};
 use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
-use crate::traits::{Accessor, TagExt};
+use crate::traits::{Accessor, SplitAndRejoinTag, TagExt};
 
 use std::borrow::Cow;
 use std::fs::{File, OpenOptions};
@@ -211,9 +211,19 @@ impl TagExt for RIFFInfoList {
 	}
 }
 
+impl SplitAndRejoinTag for RIFFInfoList {
+	fn split_tag(&mut self) -> Tag {
+		std::mem::take(self).into()
+	}
+
+	fn rejoin_tag(&mut self, tag: Tag) {
+		*self = tag.into();
+	}
+}
+
 impl From<RIFFInfoList> for Tag {
 	fn from(input: RIFFInfoList) -> Self {
-		let mut tag = Tag::new(TagType::RIFFInfo);
+		let mut tag = Self::new(TagType::RIFFInfo);
 
 		for (k, v) in input.items {
 			let item_key = ItemKey::from_key(TagType::RIFFInfo, &k);

src/lib.rs

@@ -178,7 +178,7 @@ pub use crate::tag::{Tag, TagType};
 pub use tag::item::{ItemKey, ItemValue, TagItem};
 pub use util::text::TextEncoding;
 
-pub use crate::traits::{Accessor, TagExt};
+pub use crate::traits::{Accessor, SplitAndRejoinTag, TagExt};
 
 pub use picture::PictureInformation;
 

src/mp4/ilst/atom.rs

@@ -12,10 +12,10 @@ pub(super) enum AtomDataStorage {
 }
 
 impl AtomDataStorage {
-	pub(super) fn take_first(self) -> AtomData {
+	pub(super) fn first_mut(&mut self) -> &mut AtomData {
 		match self {
 			AtomDataStorage::Single(val) => val,
-			AtomDataStorage::Multiple(mut data) => data.swap_remove(0),
+			AtomDataStorage::Multiple(data) => data.first_mut().expect("not empty"),
 		}
 	}
 }

src/mp4/ilst/mod.rs

@@ -7,10 +7,10 @@ pub(crate) mod write;
 use super::AtomIdent;
 use crate::error::LoftyError;
 use crate::mp4::ilst::atom::AtomDataStorage;
-use crate::picture::{Picture, PictureType};
+use crate::picture::{Picture, PictureType, TOMBSTONE_PICTURE};
 use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
-use crate::traits::{Accessor, TagExt};
+use crate::traits::{Accessor, SplitAndRejoinTag, TagExt};
 use atom::{AdvisoryRating, Atom, AtomData};
 
 use std::borrow::Cow;
@@ -377,20 +377,23 @@ impl TagExt for Ilst {
 	}
 }
 
-impl From<Ilst> for Tag {
-	fn from(input: Ilst) -> Self {
-		let mut tag = Self::new(TagType::MP4ilst);
+impl SplitAndRejoinTag for Ilst {
+	fn split_tag(&mut self) -> Tag {
+		let mut tag = Tag::new(TagType::MP4ilst);
 
-		for atom in input.atoms {
+		self.atoms.retain_mut(|atom| {
 			let Atom { ident, data } = atom;
-			let value = match data.take_first() {
-				AtomData::UTF8(text) | AtomData::UTF16(text) => ItemValue::Text(text),
-				AtomData::Picture(pic) => {
-					tag.pictures.push(pic);
-					continue;
+			let value = match data.first_mut() {
+				AtomData::UTF8(text) | AtomData::UTF16(text) => {
+					ItemValue::Text(std::mem::take(text))
+				},
+				AtomData::Picture(picture) => {
+					tag.pictures
+						.push(std::mem::replace(picture, TOMBSTONE_PICTURE));
+					return false; // Atom consumed
 				},
 				AtomData::Bool(b) => {
-					let text = if b { "1".to_owned() } else { "0".to_owned() };
+					let text = if *b { "1".to_owned() } else { "0".to_owned() };
 					ItemValue::Text(text)
 				},
 				// We have to special case track/disc numbers since they are stored together
@@ -403,6 +406,7 @@ impl From<Ilst> for Tag {
 
 								tag.insert_text(ItemKey::TrackNumber, current.to_string());
 								tag.insert_text(ItemKey::TrackTotal, total.to_string());
+								return false; // Atom consumed
 							},
 							b"disk" => {
 								let current = u16::from_be_bytes([data[2], data[3]]);
@@ -410,14 +414,17 @@ impl From<Ilst> for Tag {
 
 								tag.insert_text(ItemKey::DiscNumber, current.to_string());
 								tag.insert_text(ItemKey::DiscTotal, total.to_string());
+								return false; // Atom consumed
 							},
 							_ => {},
 						}
 					}
 
-					continue;
+					return true; // Keep atom
+				},
+				_ => {
+					return true; // Keep atom
 				},
-				_ => continue,
 			};
 
 			let key = ItemKey::from_key(
@@ -433,14 +440,13 @@ impl From<Ilst> for Tag {
 			);
 
 			tag.items.push(TagItem::new(key, value));
-		}
+			false // Atom consumed
+		});
 
 		tag
 	}
-}
 
-impl From<Tag> for Ilst {
-	fn from(input: Tag) -> Self {
+	fn rejoin_tag(&mut self, tag: Tag) {
 		fn convert_to_uint(space: &mut Option<u16>, cont: &str) {
 			if let Ok(num) = cont.parse::<u16>() {
 				*space = Some(num);
@@ -465,13 +471,11 @@ impl From<Tag> for Ilst {
 			}
 		}
 
-		let mut ilst = Self::default();
-
 		// Storage for integer pairs
 		let mut tracks: (Option<u16>, Option<u16>) = (None, None);
 		let mut discs: (Option<u16>, Option<u16>) = (None, None);
 
-		for item in input.items {
+		for item in tag.items {
 			let key = item.item_key;
 
 			if let Ok(ident) = TryInto::<AtomIdent<'_>>::try_into(&key) {
@@ -495,13 +499,13 @@ impl From<Tag> for Ilst {
 									continue;
 								},
 							};
-							ilst.atoms.push(Atom {
+							self.atoms.push(Atom {
 								ident: ident.into_owned(),
 								data: AtomDataStorage::Single(AtomData::Bool(data)),
 							})
 						}
 					},
-					_ => ilst.atoms.push(Atom {
+					_ => self.atoms.push(Atom {
 						ident: ident.into_owned(),
 						data: AtomDataStorage::Single(AtomData::UTF8(data)),
 					}),
@@ -509,20 +513,32 @@ impl From<Tag> for Ilst {
 			}
 		}
 
-		for mut picture in input.pictures {
+		for mut picture in tag.pictures {
 			// Just for correctness, since we can't actually
 			// assign a picture type in this format
 			picture.pic_type = PictureType::Other;
 
-			ilst.atoms.push(Atom {
+			self.atoms.push(Atom {
 				ident: AtomIdent::Fourcc([b'c', b'o', b'v', b'r']),
 				data: AtomDataStorage::Single(AtomData::Picture(picture)),
 			})
 		}
 
-		create_int_pair(&mut ilst, *b"trkn", tracks);
-		create_int_pair(&mut ilst, *b"disk", discs);
+		create_int_pair(self, *b"trkn", tracks);
+		create_int_pair(self, *b"disk", discs);
+	}
+}
 
+impl From<Ilst> for Tag {
+	fn from(mut input: Ilst) -> Self {
+		input.split_tag()
+	}
+}
+
+impl From<Tag> for Ilst {
+	fn from(input: Tag) -> Self {
+		let mut ilst = Self::default();
+		ilst.rejoin_tag(input);
 		ilst
 	}
 }

src/ogg/tag.rs

@@ -7,7 +7,7 @@ use crate::picture::{Picture, PictureInformation};
 use crate::probe::Probe;
 use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
-use crate::traits::{Accessor, TagExt};
+use crate::traits::{Accessor, SplitAndRejoinTag, TagExt};
 
 use std::borrow::Cow;
 use std::fs::{File, OpenOptions};
@@ -328,11 +328,11 @@ impl TagExt for VorbisComments {
 	}
 }
 
-impl From<VorbisComments> for Tag {
-	fn from(input: VorbisComments) -> Self {
+impl SplitAndRejoinTag for VorbisComments {
+	fn split_tag(&mut self) -> Tag {
 		let mut tag = Tag::new(TagType::VorbisComments);
 
-		for (k, v) in input.items {
+		for (k, v) in std::mem::take(&mut self.items) {
 			tag.items.push(TagItem::new(
 				ItemKey::from_key(TagType::VorbisComments, &k),
 				ItemValue::Text(v),
@@ -347,31 +347,28 @@ impl From<VorbisComments> for Tag {
 		{
 			tag.items.push(TagItem::new(
 				ItemKey::EncoderSoftware,
-				ItemValue::Text(input.vendor),
+				// Preserve the original vendor by cloning
+				ItemValue::Text(self.vendor.clone()),
 			));
 		}
 
-		for (pic, _info) in input.pictures {
+		for (pic, _info) in std::mem::take(&mut self.pictures) {
 			tag.push_picture(pic)
 		}
 
 		tag
 	}
-}
-
-impl From<Tag> for VorbisComments {
-	fn from(mut input: Tag) -> Self {
-		let mut vorbis_comments = Self::default();
 
+	fn rejoin_tag(&mut self, mut tag: Tag) {
 		if let Some(TagItem {
 			item_value: ItemValue::Text(val),
 			..
-		}) = input.take(&ItemKey::EncoderSoftware).next()
+		}) = tag.take(&ItemKey::EncoderSoftware).next()
 		{
-			vorbis_comments.vendor = val;
+			self.vendor = val;
 		}
 
-		for item in input.items {
+		for item in tag.items {
 			let item_key = item.item_key;
 			let item_value = item.item_value;
 
@@ -386,15 +383,27 @@ impl From<Tag> for VorbisComments {
 				Some(k) => k,
 			};
 
-			vorbis_comments.items.push((key.to_string(), val));
+			self.items.push((key.to_string(), val));
 		}
 
-		for picture in input.pictures {
+		for picture in tag.pictures {
 			if let Ok(information) = PictureInformation::from_picture(&picture) {
-				vorbis_comments.pictures.push((picture, information))
+				self.pictures.push((picture, information))
 			}
 		}
+	}
+}
 
+impl From<VorbisComments> for Tag {
+	fn from(mut input: VorbisComments) -> Self {
+		input.split_tag()
+	}
+}
+
+impl From<Tag> for VorbisComments {
+	fn from(input: Tag) -> Self {
+		let mut vorbis_comments = Self::default();
+		vorbis_comments.rejoin_tag(input);
 		vorbis_comments
 	}
 }

src/picture.rs

@@ -502,8 +502,8 @@ impl Picture {
 		Self {
 			pic_type,
 			mime_type,
-			description: description.map(Cow::from),
-			data: Cow::from(data),
+			description: description.map(Cow::Owned),
+			data: Cow::Owned(data),
 		}
 	}
 
@@ -890,3 +890,11 @@ impl Picture {
 		}
 	}
 }
+
+// A placeholder that is needed during conversions.
+pub(crate) const TOMBSTONE_PICTURE: Picture = Picture {
+	pic_type: PictureType::Other,
+	mime_type: MimeType::Unknown(String::new()),
+	description: None,
+	data: Cow::Owned(Vec::new()),
+};

src/tag/mod.rs

@@ -6,7 +6,7 @@ use crate::file::FileType;
 use crate::macros::err;
 use crate::picture::{Picture, PictureType};
 use crate::probe::Probe;
-use crate::traits::{Accessor, TagExt};
+use crate::traits::{Accessor, SplitAndRejoinTag, TagExt};
 use item::{ItemKey, ItemValue, TagItem};
 
 use std::borrow::Cow;
@@ -583,6 +583,16 @@ impl TagExt for Tag {
 	}
 }
 
+impl SplitAndRejoinTag for Tag {
+	fn split_tag(&mut self) -> Self {
+		std::mem::replace(self, Self::new(self.tag_type))
+	}
+
+	fn rejoin_tag(&mut self, tag: Self) {
+		*self = tag;
+	}
+}
+
 /// The tag's format
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 #[non_exhaustive]

src/traits.rs

@@ -233,6 +233,30 @@ pub trait TagExt: Accessor + Into<Tag> + Sized {
 	fn clear(&mut self);
 }
 
+/// Split and rejoin tags.
+///
+/// Useful and required for implementing read/modify/write round trips.
+pub trait SplitAndRejoinTag {
+	/// Extract and split generic contents into a [`Tag`].
+	///
+	/// Leaves the remainder that cannot be represented in the
+	/// resulting tag in `self`. This is useful if the modified [`Tag`]
+	/// is rejoined later using [`SplitAndRejoinTag::rejoin_tag`].
+	// NOTE: Using the "typestate pattern" (http://cliffle.com/blog/rust-typestate/)
+	// to represent the intermediate state turned out as less flexible
+	// and useful than expected.
+	fn split_tag(&mut self) -> Tag;
+
+	/// Rejoin a [`Tag`].
+	///
+	/// Rejoin a tag that has previously been obtained with
+	/// [`SplitAndRejoinTag::split_tag`].
+	///
+	/// Restores the original representation merged with the contents of
+	/// `tag` for further processing, e.g. writing back into a file.
+	fn rejoin_tag(&mut self, tag: Tag);
+}
+
 // TODO: https://github.com/rust-lang/rust/issues/59359
 pub(crate) trait SeekStreamLen: std::io::Seek {
 	fn stream_len(&mut self) -> crate::error::Result<u64> {