Fix METADATA_BLOCK_PICTURE parsing/writing

src/ogg/flac/write.rs

@@ -155,7 +155,7 @@ fn create_picture_blocks(
 	for (pic, info) in pictures {
 		writer.write_u8(byte)?;
 
-		let pic_bytes = pic.as_flac_bytes(info);
+		let pic_bytes = pic.as_flac_bytes(info, false);
 		let pic_len = pic_bytes.len() as u32;
 
 		if pic_len > 65535 {
@@ -163,7 +163,7 @@ fn create_picture_blocks(
 		}
 
 		writer.write_all(&pic_len.to_be_bytes()[1..])?;
-		writer.write_all(pic_bytes.as_bytes())?;
+		writer.write_all(pic_bytes.as_slice())?;
 	}
 
 	Ok(())

src/ogg/write.rs

@@ -46,6 +46,8 @@ pub(crate) fn create_comments(
 
 #[cfg(feature = "vorbis_comments")]
 fn create_pages(tag: &mut VorbisCommentsRef, writer: &mut Cursor<Vec<u8>>) -> Result<Vec<Page>> {
+	const PICTURE_KEY: &str = "METADATA_BLOCK_PICTURE=";
+
 	let item_count_pos = writer.seek(SeekFrom::Current(0))?;
 
 	writer.write_u32::<LittleEndian>(0)?;
@@ -54,19 +56,16 @@ fn create_pages(tag: &mut VorbisCommentsRef, writer: &mut Cursor<Vec<u8>>) -> Re
 	create_comments(writer, &mut count, &mut tag.items)?;
 
 	for (pic, _) in &mut tag.pictures {
-		let picture = format!(
+		let picture = pic.as_flac_bytes(PictureInformation::from_picture(pic)?, true);
-			"METADATA_BLOCK_PICTURE={}",
-			base64::encode(pic.as_flac_bytes(PictureInformation::from_picture(pic)?))
-		);
 
-		let picture_b = picture.as_bytes();
+		let bytes_len = picture.len() + PICTURE_KEY.len();
-		let bytes_len = picture_b.len();
 
 		if u32::try_from(bytes_len as u64).is_ok() {
 			count += 1;
 
 			writer.write_u32::<LittleEndian>(bytes_len as u32)?;
-			writer.write_all(picture_b)?;
+			writer.write_all(PICTURE_KEY.as_bytes())?;
+			writer.write_all(&*picture)?;
 		}
 	}
 

src/types/picture.rs

@@ -666,11 +666,14 @@ impl Picture {
 	#[cfg(feature = "vorbis_comments")]
 	/// Convert a [`Picture`] to a base64 encoded FLAC `METADATA_BLOCK_PICTURE` String
 	///
+	/// Use `encode` to convert the picture to a base64 encoded String ([RFC 4648 §4](http://www.faqs.org/rfcs/rfc4648.html))
+	///
 	/// NOTES:
 	///
 	/// * This does not include a key (Vorbis comments) or METADATA_BLOCK_HEADER (FLAC blocks)
 	/// * FLAC blocks have different size requirements than OGG Vorbis/Opus, size is not checked here
-	pub fn as_flac_bytes(&self, picture_information: PictureInformation) -> String {
+	/// * When writing to Vorbis comments, the data **must** be base64 encoded
+	pub fn as_flac_bytes(&self, picture_information: PictureInformation, encode: bool) -> Vec<u8> {
 		let mut data = Vec::<u8>::new();
 
 		let picture_type = u32::from(self.pic_type.as_u8()).to_be_bytes();
@@ -688,6 +691,8 @@ impl Picture {
 
 			data.extend(desc_len.to_be_bytes().iter());
 			data.extend(desc_str.as_bytes().iter());
+		} else {
+			data.extend([0; 4].iter());
 		}
 
 		data.extend(picture_information.width.to_be_bytes().iter());
@@ -701,7 +706,11 @@ impl Picture {
 		data.extend(pic_data_len.to_be_bytes().iter());
 		data.extend(pic_data.iter());
 
-		base64::encode(data)
+		if encode {
+			base64::encode(data).into_bytes()
+		} else {
+			data
+		}
 	}
 
 	#[cfg(feature = "vorbis_comments")]
@@ -718,8 +727,8 @@ impl Picture {
 
 		let mut cursor = Cursor::new(data);
 
-		if let Ok(bytes) = cursor.read_u32::<BigEndian>() {
+		if let Ok(pic_ty) = cursor.read_u32::<BigEndian>() {
-			let picture_type = PictureType::from_u8(bytes as u8);
+			let picture_type = PictureType::from_u8(pic_ty as u8);
 
 			if let Ok(mime_len) = cursor.read_u32::<BigEndian>() {
 				let mut buf = vec![0; mime_len as usize];
@@ -731,17 +740,13 @@ impl Picture {
 					let mut description = None;
 
 					if let Ok(desc_len) = cursor.read_u32::<BigEndian>() {
-						if cursor.get_ref().len() >= (cursor.position() as u32 + desc_len) as usize
+						if desc_len > 0 {
-						{
 							let mut buf = vec![0; desc_len as usize];
 							cursor.read_exact(&mut buf)?;
 
 							if let Ok(desc) = String::from_utf8(buf) {
 								description = Some(Cow::from(desc));
 							}
-						} else {
-							cursor.set_position(cursor.position() - 4)
-						}
 						}
 
 						if let (Ok(width), Ok(height), Ok(color_depth), Ok(num_colors)) = (
@@ -774,6 +779,7 @@ impl Picture {
 					}
 				}
 			}
+		}
 
 		Err(LoftyError::NotAPicture)
 	}

tests/picture/format_parsers.rs

@@ -105,11 +105,7 @@ fn as_flac_bytes() {
 	let original_picture_information =
 		PictureInformation::from_png(original_picture.data()).unwrap();
 
-	let original_as_flac = original_picture.as_flac_bytes(original_picture_information);
+	let original_as_flac = original_picture.as_flac_bytes(original_picture_information, true);
 
-	// `Picture::as_flac_bytes` returns a base64 encoded string
+	assert_eq!(&*buf, original_as_flac);
-	// but the asset just has binary data
-	let original_decoded = base64::decode(original_as_flac).unwrap();
-
-	assert_eq!(&*buf, original_decoded);
 }