From 47a28402dbcff6f19e132c724bc355e2911a300f Mon Sep 17 00:00:00 2001
From: localthomas <81923579+localthomas@users.noreply.github.com>
Date: Fri, 21 Jan 2022 14:52:32 +0100
Subject: [PATCH] added frame sync search for MP3 reading

The first MP3 frame behind metadata blocks is found by searching for frame sync bits.
This skips junk bytes between any metadata blocks and the first MP3 frame.
---
 src/mp3/read.rs          |  33 +++++++++++++++++++++++----------
 tests/files/assets/b.mp3 | Bin 0 -> 1458 bytes
 tests/files/mpeg.rs      |  25 ++++++++++++++++++++++++-
 3 files changed, 47 insertions(+), 11 deletions(-)
 create mode 100644 tests/files/assets/b.mp3

diff --git a/src/mp3/read.rs b/src/mp3/read.rs
index 7d1c8241..e923ec9d 100644
--- a/src/mp3/read.rs
+++ b/src/mp3/read.rs
@@ -1,4 +1,4 @@
-use super::header::{verify_frame_sync, Header, XingHeader};
+use super::header::{search_for_frame_sync, Header, XingHeader};
 use super::{Mp3File, Mp3Properties};
 use crate::ape::constants::APE_PREAMBLE;
 #[cfg(feature = "ape")]
@@ -12,7 +12,7 @@ use crate::id3::{find_id3v1, find_lyrics3v2, ID3FindResults};
 
 use std::io::{Read, Seek, SeekFrom};
 
-use byteorder::ReadBytesExt;
+use byteorder::{BigEndian, ReadBytesExt};
 
 pub(super) fn read_from<R>(
 	reader: &mut R,
@@ -80,17 +80,30 @@ where
 					continue;
 				}
 			},
-			_ if verify_frame_sync([header[0], header[1]]) => {
-				let start = reader.seek(SeekFrom::Current(0))? - 4;
-				let header = Header::read(u32::from_be_bytes(header))?;
+			// metadata blocks might be followed by junk bytes before the first MP3 frame begins
+			_ => {
+				// seek back the length of the temporary header buffer
+				// so that all bytes are included in the search for a frame sync
+				let start_of_search_area =
+					reader.seek(SeekFrom::Current(-1 * header.len() as i64))?;
+				if let Some(first_mp3_frame_start_relative) = search_for_frame_sync(reader)? {
+					let first_mp3_frame_start_absolute =
+						start_of_search_area + first_mp3_frame_start_relative;
 
-				file.first_frame_offset = Some(start);
-				first_frame_header = Some(header);
+					// read the first four bytes of the found frame
+					reader.seek(SeekFrom::Start(first_mp3_frame_start_absolute))?;
+					let header = Header::read(reader.read_u32::<BigEndian>()?)?;
 
-				// We have found the first frame
-				break;
+					file.first_frame_offset = Some(first_mp3_frame_start_absolute);
+					first_frame_header = Some(header);
+
+					// We have found the first frame
+					break;
+				} else {
+					// the search for sync bits was unsuccessful
+					return Err(LoftyError::Mp3("File contains an invalid frame"));
+				}
 			},
-			_ => return Err(LoftyError::Mp3("File contains an invalid frame")),
 		}
 	}
 
diff --git a/tests/files/assets/b.mp3 b/tests/files/assets/b.mp3
new file mode 100644
index 0000000000000000000000000000000000000000..b8fe60bf4cbc3830b1ae59945cc7b49ad7947243
GIT binary patch
literal 1458
zcmeZtF=l1}0w%!_M;|93Lz01k@&CU>h8%_@hEj%H1_g!^hE#@PAj!ZG65whGq@<xr
zihzQdU?~(u!NIOTMe<-pJ`9NrWejNyrVJJgdJJX^MnKY#!2k$>rg?@K0V#-uAd^af
zmZqXu2$UW;pa2B_e+LkBfM;G>K2Q-q5Hm3_un05EBo+K88Gsz+lUSB)YN2NciV`?b
zVP%J~S%f(bd#Az0p^X2(C!{d&e_#NHHUk3(0|T!DFq(j936Kk9nlu1C;N$4)YOH5y
zU=TX;z<~Sx|L;jD4*mEeXyir9VDfQ@qkBnaNlvOlNop~uP{aU<MJ1WVCAg#$bCODP
MaY<sBg(>hK05%tl5dZ)H

literal 0
HcmV?d00001

diff --git a/tests/files/mpeg.rs b/tests/files/mpeg.rs
index 164a6b7e..7972f7e4 100644
--- a/tests/files/mpeg.rs
+++ b/tests/files/mpeg.rs
@@ -1,5 +1,5 @@
 use crate::{set_artist, temp_file, verify_artist};
-use lofty::{FileType, ItemKey, ItemValue, TagItem, TagType};
+use lofty::{Accessor, FileType, ItemKey, ItemValue, TagItem, TagType};
 use std::io::{Seek, SeekFrom, Write};
 
 #[test]
@@ -19,6 +19,29 @@ fn read() {
 	crate::verify_artist!(file, tag, TagType::Ape, "Baz artist", 1);
 }
 
+#[test]
+fn read_with_junk_bytes_between_frames() {
+	// Read a file that includes an ID3v2.3 data block followed by four bytes of junk data (0x20)
+	let file = lofty::read_from_path("tests/files/assets/b.mp3", true).unwrap();
+
+	// note that the file contains ID3v2 and ID3v1 data
+	assert_eq!(file.file_type(), &FileType::MP3);
+
+	let id3v2_tag = &file.tags()[0];
+	assert_eq!(id3v2_tag.artist(), Some("artist test"));
+	assert_eq!(id3v2_tag.album(), Some("album test"));
+	assert_eq!(id3v2_tag.title(), Some("title test"));
+	assert_eq!(
+		id3v2_tag.get_string(&ItemKey::EncoderSettings),
+		Some("Lavf58.62.100")
+	);
+
+	let id3v1_tag = &file.tags()[1];
+	assert_eq!(id3v1_tag.artist(), Some("artist test"));
+	assert_eq!(id3v1_tag.album(), Some("album test"));
+	assert_eq!(id3v1_tag.title(), Some("title test"));
+}
+
 #[test]
 fn write() {
 	let mut file = temp_file!("tests/files/assets/a.mp3");