diff --git a/CHANGELOG.md b/CHANGELOG.md index afeb9f195..e443c318a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lofty/src/mpeg/header.rs b/lofty/src/mpeg/header.rs index f57eacc26..3b8827021 100644 --- a/lofty/src/mpeg/header.rs +++ b/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( - input: &mut R, - pos: &mut u64, - parse_mode: ParsingMode, -) -> Result> +pub(super) fn rev_search_for_frame_header(input: &mut R, pos: &mut u64) -> Result> 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::()?; let size = reader.read_u32::()?; - 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::()?; let frames = reader.read_u32::()?; - 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()); diff --git a/lofty/src/mpeg/properties.rs b/lofty/src/mpeg/properties.rs index c703a94b0..23e6fac28 100644 --- a/lofty/src/mpeg/properties.rs +++ b/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( reader: &mut R, first_frame: (Header, u64), mut last_frame_offset: u64, - xing_header: Option, + vbr_header: Option, 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,7 +174,14 @@ 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))?; @@ -179,7 +189,7 @@ where 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(()) diff --git a/lofty/src/mpeg/read.rs b/lofty/src/mpeg/read.rs index 41ef39d67..6f07de099 100644 --- a/lofty/src/mpeg/read.rs +++ b/lofty/src/mpeg/read.rs @@ -194,7 +194,6 @@ where last_frame_offset, xing_header, file_length, - parse_options.parsing_mode, )?; }