MPEG: Improve duration calculation

CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+- **MPEG**: Fix durations being slightly off ([issue](https://github.com/Serial-ATA/lofty-rs/issues/412)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/413))
+
 ## [0.20.0] - 2024-06-06
 
 ### Added

lofty/src/mpeg/header.rs

@@ -1,5 +1,4 @@
 use super::constants::{BITRATES, PADDING_SIZES, SAMPLES, SAMPLE_RATES, SIDE_INFORMATION_SIZES};
-use crate::config::ParsingMode;
 use crate::error::Result;
 use crate::macros::decode_err;
 
@@ -50,11 +49,7 @@ where
 // Unlike `search_for_frame_sync`, since this has the `Seek` bound, it will seek the reader
 // back to the start of the header.
 const REV_FRAME_SEARCH_BOUNDS: u64 = 1024;
-pub(super) fn rev_search_for_frame_header<R>(
-	input: &mut R,
-	pos: &mut u64,
-	parse_mode: ParsingMode,
-) -> Result<Option<Header>>
+pub(super) fn rev_search_for_frame_header<R>(input: &mut R, pos: &mut u64) -> Result<Option<Header>>
 where
 	R: Read + Seek,
 {
@@ -70,36 +65,33 @@ where
 	for (i, byte) in buf.iter().rev().enumerate() {
 		frame_sync[1] = frame_sync[0];
 		frame_sync[0] = *byte;
-		if verify_frame_sync(frame_sync) {
-			let relative_frame_start = (search_bounds as usize) - (i + 1);
-			if relative_frame_start + 4 > buf.len() {
-				if parse_mode == ParsingMode::Strict {
-					decode_err!(@BAIL Mpeg, "Expected full frame header (incomplete stream?)")
-				}
+		if !verify_frame_sync(frame_sync) {
+			continue;
+		}
 
-				log::warn!("MPEG: Incomplete frame header, giving up");
-				break;
-			}
+		let relative_frame_start = (search_bounds as usize) - (i + 1);
+		if relative_frame_start + 4 > buf.len() {
+			continue;
+		}
 
-			let header = Header::read(u32::from_be_bytes([
-				frame_sync[0],
-				frame_sync[1],
-				buf[relative_frame_start + 2],
-				buf[relative_frame_start + 3],
-			]));
-
-			// We need to check if the header is actually valid. For
-			// all we know, we could be in some junk (ex. 0xFF_FF_FF_FF).
-			if header.is_none() {
-				continue;
-			}
+		let header = Header::read(u32::from_be_bytes([
+			frame_sync[0],
+			frame_sync[1],
+			buf[relative_frame_start + 2],
+			buf[relative_frame_start + 3],
+		]));
+
+		// We need to check if the header is actually valid. For
+		// all we know, we could be in some junk (ex. 0xFF_FF_FF_FF).
+		if header.is_none() {
+			continue;
+		}
 
-			// Seek to the start of the frame sync
-			*pos += relative_frame_start as u64;
-			input.seek(SeekFrom::Start(*pos))?;
+		// Seek to the start of the frame sync
+		*pos += relative_frame_start as u64;
+		input.seek(SeekFrom::Start(*pos))?;
 
-			return Ok(header);
-		}
+		return Ok(header);
 	}
 
 	Ok(None)
@@ -326,7 +318,16 @@ impl Header {
 	}
 }
 
+#[derive(Copy, Clone)]
+pub(super) enum VbrHeaderType {
+	Xing,
+	Info,
+	Vbri,
+}
+
+#[derive(Copy, Clone)]
 pub(super) struct VbrHeader {
+	pub ty: VbrHeaderType,
 	pub frames: u32,
 	pub size: u32,
 }
@@ -357,7 +358,13 @@ impl VbrHeader {
 				let frames = reader.read_u32::<BigEndian>()?;
 				let size = reader.read_u32::<BigEndian>()?;
 
-				Ok(Some(Self { frames, size }))
+				let ty = match &header {
+					b"Xing" => VbrHeaderType::Xing,
+					b"Info" => VbrHeaderType::Info,
+					_ => unreachable!(),
+				};
+
+				Ok(Some(Self { ty, frames, size }))
 			},
 			b"VBRI" => {
 				if reader_len < 32 {
@@ -373,7 +380,11 @@ impl VbrHeader {
 				let size = reader.read_u32::<BigEndian>()?;
 				let frames = reader.read_u32::<BigEndian>()?;
 
-				Ok(Some(Self { frames, size }))
+				Ok(Some(Self {
+					ty: VbrHeaderType::Vbri,
+					frames,
+					size,
+				}))
 			},
 			_ => Ok(None),
 		}
@@ -386,7 +397,6 @@ impl VbrHeader {
 
 #[cfg(test)]
 mod tests {
-	use crate::config::ParsingMode;
 	use crate::tag::utils::test_utils::read_path;
 
 	use std::io::{Cursor, Read, Seek, SeekFrom};
@@ -410,7 +420,7 @@ mod tests {
 			// We have to start these at the end to do a reverse search, of course :)
 			let mut pos = reader.seek(SeekFrom::End(0)).unwrap();
 
-			let ret = super::rev_search_for_frame_header(reader, &mut pos, ParsingMode::Strict);
+			let ret = super::rev_search_for_frame_header(reader, &mut pos);
 
 			if expected_reader_position.is_some() {
 				assert!(ret.is_ok());

lofty/src/mpeg/properties.rs

@@ -1,5 +1,4 @@
-use super::header::{ChannelMode, Emphasis, Header, Layer, MpegVersion, VbrHeader};
-use crate::config::ParsingMode;
+use super::header::{ChannelMode, Emphasis, Header, Layer, MpegVersion, VbrHeader, VbrHeaderType};
 use crate::error::Result;
 use crate::mpeg::header::rev_search_for_frame_header;
 use crate::properties::{ChannelMask, FileProperties};
@@ -126,9 +125,8 @@ pub(super) fn read_properties<R>(
 	reader: &mut R,
 	first_frame: (Header, u64),
 	mut last_frame_offset: u64,
-	xing_header: Option<VbrHeader>,
+	vbr_header: Option<VbrHeader>,
 	file_length: u64,
-	parse_mode: ParsingMode,
 ) -> Result<()>
 where
 	R: Read + Seek,
@@ -150,15 +148,20 @@ where
 		2
 	};
 
-	if let Some(xing_header) = xing_header {
-		if first_frame_header.sample_rate > 0 && xing_header.is_valid() {
-			let frame_time = (u32::from(first_frame_header.samples) * 1000)
-				.div_round(first_frame_header.sample_rate);
-			let length = u64::from(frame_time) * u64::from(xing_header.frames);
+	if let Some(vbr_header) = vbr_header {
+		if first_frame_header.sample_rate > 0 && vbr_header.is_valid() {
+			log::debug!("MPEG: Valid VBR header; using it to calculate duration");
+
+			let sample_rate = u64::from(first_frame_header.sample_rate);
+			let samples_per_frame = u64::from(first_frame_header.samples);
+
+			let total_frames = u64::from(vbr_header.frames);
+
+			let length = (samples_per_frame * 1000 * total_frames).div_round(sample_rate);
 
 			properties.duration = Duration::from_millis(length);
 			properties.overall_bitrate = ((file_length * 8) / length) as u32;
-			properties.audio_bitrate = ((u64::from(xing_header.size) * 8) / length) as u32;
+			properties.audio_bitrate = ((u64::from(vbr_header.size) * 8) / length) as u32;
 
 			return Ok(());
 		}
@@ -171,15 +174,22 @@ where
 
 	log::warn!("MPEG: Using bitrate to estimate duration");
 
-	properties.audio_bitrate = first_frame_header.bitrate;
+	// http://gabriel.mp3-tech.org/mp3infotag.html:
+	//
+	// "In the Info Tag, the "Xing" identification string (mostly at 0x24) of the header is replaced by "Info" in case of a CBR file."
+	let is_cbr = matches!(vbr_header.map(|h| h.ty), Some(VbrHeaderType::Info));
+	if is_cbr {
+		log::debug!("MPEG: CBR detected");
+		properties.audio_bitrate = first_frame_header.bitrate;
+	}
 
 	// Search for the last frame, starting at the end of the frames
 	reader.seek(SeekFrom::Start(last_frame_offset))?;
 
 	let mut last_frame = None;
 	let mut pos = last_frame_offset;
 	while pos > 0 {
-		match rev_search_for_frame_header(reader, &mut pos, parse_mode) {
+		match rev_search_for_frame_header(reader, &mut pos) {
 			// Found a frame header
 			Ok(Some(header)) => {
 				// Move `last_frame_offset` back to the actual position
@@ -197,14 +207,23 @@ where
 		}
 	}
 
-	if let Some(last_frame_header) = last_frame {
-		let stream_len = last_frame_offset - first_frame_offset + u64::from(last_frame_header.len);
-		let length = (stream_len * 8).div_round(u64::from(properties.audio_bitrate));
+	let Some(last_frame_header) = last_frame else {
+		log::warn!("MPEG: Could not find last frame, properties will be incomplete");
+		return Ok(());
+	};
+
+	let stream_len = (last_frame_offset + u64::from(last_frame_header.len)) - first_frame_offset;
+	if !is_cbr {
+		log::debug!("MPEG: VBR detected");
 
-		if length > 0 {
-			properties.overall_bitrate = ((file_length * 8) / length) as u32;
-			properties.duration = Duration::from_millis(length);
-		}
+		// TODO: Actually handle VBR streams, this still assumes CBR
+		properties.audio_bitrate = first_frame_header.bitrate;
+	}
+
+	let length = (stream_len * 8).div_round(u64::from(properties.audio_bitrate));
+	if length > 0 {
+		properties.overall_bitrate = ((file_length * 8) / length) as u32;
+		properties.duration = Duration::from_millis(length);
 	}
 
 	Ok(())

lofty/src/mpeg/read.rs

@@ -194,7 +194,6 @@ where
 			last_frame_offset,
 			xing_header,
 			file_length,
-			parse_options.parsing_mode,
 		)?;
 	}