FLAC: Don't error by default on invalid picture types

FLAC picture blocks have `u32::MAX` possible picture types, but only 20 are actually defined. In the [spec] it states, "Others are reserved and should not be used."

We used to only allow a picture type of up to 255 to be in sync with ID3v2's APIC, where this model comes from. Now any picture type can be specified and it will just be clamped to 255, so long as `ParsingMode::Strict` is not being used.

Author: Serial-ATA

CHANGELOG.md

@@ -24,10 +24,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - `Ilst::remove` will now return all of the removed atoms
   - `Ilst::insert_picture` will now combine all pictures into a single `covr` atom
   - `Ilst::insert` will now merge atoms with the same identifier into a single atom
-- **FLAC**: Allow multiple Vorbis Comment blocks when not using `ParsingMode::Strict`
-  - This is not allowed [by spec](https://xiph.org/flac/format.html#def_VORBIS_COMMENT), but is still possible
-    to encounter in the wild. Now we will just tag whichever tag happens to be latest in the stream and
-    use it, they **will not be merged**.
+- **FLAC**:
+  - Allow multiple Vorbis Comment blocks when not using `ParsingMode::Strict`
+    - This is not allowed [by spec](https://xiph.org/flac/format.html#def_VORBIS_COMMENT), but is still possible
+      to encounter in the wild. Now we will just tag whichever tag happens to be latest in the stream and
+      use it, they **will not be merged**.
+  - Allow picture types greater than 255 when not using `ParsingMode::Strict`
+    - This is not allowed [by spec](https://xiph.org/flac/format.html#metadata_block_picture), but has been encountered
+      in the wild. Now we will just cap the picture type at 255.
 
 ## Fixed
 - **WavPack**: Custom sample rates will no longer be overwritten

src/flac/read.rs

@@ -100,9 +100,17 @@ where
 		}
 
 		if block.ty == BLOCK_ID_PICTURE {
-			flac_file
-				.pictures
-				.push(Picture::from_flac_bytes(&block.content, false)?)
+			match Picture::from_flac_bytes(&block.content, false, parse_options.parsing_mode) {
+				Ok(picture) => flac_file.pictures.push(picture),
+				Err(e) => {
+					if parse_options.parsing_mode == ParsingMode::Strict {
+						return Err(e);
+					}
+
+					log::warn!("Unable to read FLAC picture block, discarding");
+					continue;
+				},
+			}
 		}
 	}
 

src/ogg/read.rs

@@ -94,7 +94,7 @@ where
 
 		match key {
 			k if k.eq_ignore_ascii_case(b"METADATA_BLOCK_PICTURE") => {
-				match Picture::from_flac_bytes(value, true) {
+				match Picture::from_flac_bytes(value, true, parse_mode) {
 					Ok(picture) => tag.pictures.push(picture),
 					Err(e) => {
 						if parse_mode == ParsingMode::Strict {

src/picture.rs

@@ -1,5 +1,6 @@
 use crate::error::{ErrorKind, LoftyError, Result};
 use crate::macros::err;
+use crate::probe::ParsingMode;
 
 use std::borrow::Cow;
 use std::fmt::{Debug, Formatter};
@@ -625,18 +626,25 @@ impl Picture {
 	///
 	/// This function will return [`NotAPicture`][ErrorKind::NotAPicture] if
 	/// at any point it's unable to parse the data
-	pub fn from_flac_bytes(bytes: &[u8], encoded: bool) -> Result<(Self, PictureInformation)> {
+	pub fn from_flac_bytes(
+		bytes: &[u8],
+		encoded: bool,
+		parse_mode: ParsingMode,
+	) -> Result<(Self, PictureInformation)> {
 		if encoded {
 			let data = base64::engine::general_purpose::STANDARD
 				.decode(bytes)
 				.map_err(|_| LoftyError::new(ErrorKind::NotAPicture))?;
-			Self::from_flac_bytes_inner(&data)
+			Self::from_flac_bytes_inner(&data, parse_mode)
 		} else {
-			Self::from_flac_bytes_inner(bytes)
+			Self::from_flac_bytes_inner(bytes, parse_mode)
 		}
 	}
 
-	fn from_flac_bytes_inner(content: &[u8]) -> Result<(Self, PictureInformation)> {
+	fn from_flac_bytes_inner(
+		content: &[u8],
+		parse_mode: ParsingMode,
+	) -> Result<(Self, PictureInformation)> {
 		use crate::macros::try_vec;
 
 		let mut size = content.len();
@@ -652,7 +660,7 @@ impl Picture {
 		// ID3v2 APIC uses a single byte for picture type.
 		// Anything greater than that is probably invalid, so
 		// we just stop early
-		if pic_ty > 255 {
+		if pic_ty > 255 && parse_mode == ParsingMode::Strict {
 			err!(NotAPicture);
 		}