ogg_pager: Fix writing of perfectly divisible packets

lofty/Cargo.toml

@@ -22,7 +22,7 @@ lofty_attr = "0.11.0"
 # Debug logging
 log           = "0.4.22"
 # OGG Vorbis/Opus
-ogg_pager     = "0.6.1"
+ogg_pager     = { path = "../ogg_pager" }
 # Key maps
 paste         = "1.0.15"
 

lofty/src/tag/mod.rs

@@ -776,11 +776,11 @@ mod tests {
 
 		assert!(cmd_output.status.success());
 
-		let stderr = String::from_utf8(cmd_output.stderr).unwrap();
+		let stdout = String::from_utf8(cmd_output.stdout).unwrap();
 
-		assert!(!stderr.contains("CRC mismatch!"));
+		assert!(!stdout.contains("CRC mismatch!"));
 		assert!(
-			!stderr.contains("Header processing failed: Invalid data found when processing input")
+			!stdout.contains("Header processing failed: Invalid data found when processing input")
 		);
 	}
 
@@ -817,11 +817,10 @@ mod tests {
 			.output()
 			.unwrap();
 
-		assert!(cmd_output.status.success());
-
-		let stderr = String::from_utf8(cmd_output.stderr).unwrap();
+		let stdout = String::from_utf8(cmd_output.stdout).unwrap();
 
-		assert!(!stderr.contains("WARNING:"));
+		assert!(cmd_output.status.success(), "{stdout}");
+		assert!(!stdout.contains("WARNING:"));
 	}
 
 	#[test_log::test]

ogg_pager/CHANGELOG.md

@@ -6,6 +6,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.7.0] - 2024-10-26
+
+### Fixed
+- Writing a packet whose size is perfectly divisible by 255 would make the second to last segment have a size of 0, rather than 255 ([issue](https://github.com/Serial-ATA/lofty-rs/issues/469)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/475))
+
+### Removed
+- `Page::extend()` ([PR](https://github.com/Serial-ATA/lofty-rs/pull/475))
+- `segment_table()` ([PR](https://github.com/Serial-ATA/lofty-rs/pull/475))
+
 ## [0.6.1] - 2024-04-21
 
 ### Fixed

ogg_pager/src/lib.rs

@@ -136,51 +136,6 @@ impl Page {
 		self.header.checksum = crc::crc32(&self.as_bytes());
 	}
 
-	/// Extends the Page's content, returning another Page if too much data was provided
-	///
-	/// This will do nothing if `content` is greater than the max page size. In this case,
-	/// [`paginate()`] should be used.
-	pub fn extend(&mut self, content: &[u8]) -> Option<Page> {
-		let self_len = self.content.len();
-		let content_len = content.len();
-
-		if self_len + content_len <= MAX_CONTENT_SIZE {
-			self.content.extend(content.iter());
-			self.end += content_len as u64;
-
-			return None;
-		}
-
-		if content_len <= MAX_CONTENT_SIZE {
-			let remaining = 65025 - self_len;
-
-			self.content.extend(content[0..remaining].iter());
-			self.header.header_type_flag = 0;
-			self.header.abgp = 1_u64.wrapping_neg(); // -1 in two's complement indicates that no packets finish on this page
-			self.end += remaining as u64;
-
-			let mut p = Page {
-				content: content[remaining..].to_vec(),
-				header: PageHeader {
-					start: self.end,
-					header_type_flag: 1,
-					abgp: 0,
-					stream_serial: self.header.stream_serial,
-					sequence_number: self.header.sequence_number + 1,
-					segments: segment_table(remaining),
-					checksum: 0,
-				},
-				end: self.header().start + content.len() as u64,
-			};
-
-			p.gen_crc();
-
-			return Some(p);
-		}
-
-		None
-	}
-
 	/// Returns the page's content
 	pub fn content(&self) -> &[u8] {
 		self.content.as_slice()
@@ -191,41 +146,33 @@ impl Page {
 	pub fn take_content(self) -> Vec<u8> {
 		self.content
 	}
-
-	/// Returns the page's segment table
-	#[must_use]
-	pub fn segment_table(&self) -> Vec<u8> {
-		segment_table(self.content.len())
-	}
 }
 
-/// Creates a segment table based on the length
-#[must_use]
-pub fn segment_table(length: usize) -> Vec<u8> {
-	if length == 0 {
-		return vec![1, 0];
-	}
+#[cfg(test)]
+mod tests {
+	use crate::{paginate, Page, PageHeader};
+	use std::io::Cursor;
 
-	let last_len = (length % 255) as u8;
-	let needed = (length / 255) + 1;
+	pub fn segment_table(length: usize) -> Vec<u8> {
+		if length == 0 {
+			return vec![1, 0];
+		}
 
-	let mut segments = Vec::with_capacity(needed);
+		let last_len = (length % 255) as u8;
+		let needed = (length / 255) + 1;
 
-	for i in 0..needed {
-		if i + 1 < needed {
-			segments.push(255);
-		} else {
-			segments.push(last_len);
-		}
-	}
+		let mut segments = Vec::with_capacity(needed);
 
-	segments
-}
+		for i in 0..needed {
+			if i + 1 < needed {
+				segments.push(255);
+			} else {
+				segments.push(last_len);
+			}
+		}
 
-#[cfg(test)]
-mod tests {
-	use crate::{paginate, segment_table, Page, PageHeader};
-	use std::io::Cursor;
+		segments
+	}
 
 	#[test]
 	fn opus_ident_header() {
@@ -288,6 +235,86 @@ mod tests {
 			} else {
 				assert_eq!(header.header_type_flag, 1);
 			}
+
+			if i + 1 == len {
+				let segments = &header.segments[..header.segments.len()];
+				for s in &segments[..segments.len() - 1] {
+					assert_eq!(*s, 255);
+				}
+
+				assert_eq!(segments.last(), Some(&171));
+			} else {
+				assert_eq!(header.segments, vec![255; super::MAX_WRITTEN_SEGMENT_COUNT]);
+			}
+		}
+	}
+
+	#[test]
+	fn paginate_large_perfectly_divisible() {
+		// Create 20 max-size pages.
+		// This means we will need to make another page with a single zero segment table.
+		const PAGES_TO_WRITE: usize = 20;
+		const PACKET_SIZE: usize = (super::MAX_WRITTEN_SEGMENT_COUNT * 255) * PAGES_TO_WRITE;
+
+		let packet = vec![0; PACKET_SIZE];
+
+		let pages = paginate([packet.as_slice()], 1234, 0, 0).unwrap();
+
+		let len = pages.len();
+		assert_eq!(len, PAGES_TO_WRITE + 1);
+
+		for (i, page) in pages.iter().enumerate() {
+			if i + 1 == len {
+				break;
+			}
+
+			assert!(page.header.segments.iter().all(|c| *c == 255));
 		}
+
+		let last = pages.last().unwrap();
+		assert_eq!(last.header.segments.len(), 1);
+		assert_eq!(*last.header.segments.first().unwrap(), 0);
+
+		let mut total_size = 0;
+		for page in pages {
+			total_size += page
+				.header
+				.segments
+				.iter()
+				.map(|&b| usize::from(b))
+				.sum::<usize>();
+		}
+
+		assert_eq!(total_size, PACKET_SIZE);
+	}
+
+	#[test]
+	fn paginate_perfectly_divisible_terminate() {
+		// Our segment table will have 17 max-size segments, it should be terminated with a 0 to
+		// indicate the end of our packet.
+		const SEGMENTS: usize = 17;
+		const PACKET_SIZE: usize = SEGMENTS * 255;
+
+		let packet = vec![0; PACKET_SIZE];
+
+		let pages = paginate([packet.as_slice()], 1234, 0, 0).unwrap();
+
+		let len = pages.len();
+		assert_eq!(len, 1);
+
+		let page = &pages[0];
+
+		// + 1 for the terminating 0
+		assert_eq!(page.header.segments.len(), SEGMENTS + 1);
+
+		let correct_number_of_segments = page
+			.header
+			.segments
+			.iter()
+			.take(SEGMENTS)
+			.all(|&b| b == 255);
+		assert!(correct_number_of_segments);
+
+		assert_eq!(*page.header.segments.last().unwrap(), 0);
 	}
 }

ogg_pager/src/paginate.rs

@@ -16,7 +16,7 @@ struct PaginateContext {
 	idx: usize,
 	remaining_page_size: usize,
 	current_packet_len: usize,
-	last_segment_size: u8,
+	last_segment_size: Option<u8>,
 }
 
 impl PaginateContext {
@@ -29,14 +29,14 @@ impl PaginateContext {
 			flags: PaginateContextFlags {
 				first_page: true,
 				fresh_packet: true,
-				packet_spans_multiple_pages: false,
 				packet_finished_on_page: false,
+				need_nil_page: false,
 			},
 			pos: 0,
 			idx: 0,
 			remaining_page_size: MAX_WRITTEN_CONTENT_SIZE,
 			current_packet_len: 0,
-			last_segment_size: 0,
+			last_segment_size: None,
 		}
 	}
 
@@ -45,7 +45,7 @@ impl PaginateContext {
 		self.pos = 0;
 
 		self.current_packet_len = packet.len();
-		self.last_segment_size = (packet.len() % 255) as u8;
+		self.last_segment_size = Some((packet.len() % 255) as u8);
 	}
 
 	fn flush_page(&mut self, content: &mut Vec<u8>) {
@@ -61,13 +61,7 @@ impl PaginateContext {
 					_ => 0,
 				}
 			},
-			abgp: if self.flags.packet_finished_on_page {
-				self.abgp
-			} else {
-				// A special value of '-1' (in two's complement) indicates that no packets
-				// finish on this page.
-				1_u64.wrapping_neg()
-			},
+			abgp: 0,
 			stream_serial: self.stream_serial,
 			sequence_number: self.idx as u32,
 			segments: Vec::new(),
@@ -79,27 +73,36 @@ impl PaginateContext {
 		let content_len = content.len();
 		self.pos += content_len as u64;
 
+		// Determine how many *full* segments our page content takes up.
+		// Anything < 255 will be covered by `last_segment_size`
+		let full_segments_occupied = content_len / 255;
+
 		// Moving on to a new packet
 		debug_assert!(self.pos <= self.current_packet_len as u64);
-		if self.pos == self.current_packet_len as u64 {
-			self.flags.packet_spans_multiple_pages = false;
+		let at_packet_end = self.pos == self.current_packet_len as u64;
+		if at_packet_end && full_segments_occupied == MAX_WRITTEN_SEGMENT_COUNT {
+			// See comment on `PaginateContextFlags.need_nil_page`
+			self.flags.need_nil_page = true;
+			self.flags.packet_finished_on_page = false;
 		}
 
-		// We need to determine how many segments our page content takes up.
-		// If it takes up the remainder of the segment table for the entire packet,
-		// we'll just consume it as is.
-		let segments_occupied = if content_len >= 255 {
-			content_len.div_ceil(255)
+		if self.flags.packet_finished_on_page {
+			header.abgp = self.abgp;
 		} else {
-			1
-		};
+			// A special value of '-1' (in two's complement) indicates that no packets
+			// finish on this page.
+			header.abgp = 1_u64.wrapping_neg()
+		}
 
-		debug_assert!(segments_occupied <= MAX_WRITTEN_SEGMENT_COUNT);
-		if self.flags.packet_spans_multiple_pages {
-			header.segments = vec![255; segments_occupied];
-		} else {
-			header.segments = vec![255; segments_occupied - 1];
-			header.segments.push(self.last_segment_size);
+		debug_assert!(full_segments_occupied <= MAX_WRITTEN_SEGMENT_COUNT);
+		header.segments = vec![255; full_segments_occupied];
+
+		if full_segments_occupied != MAX_WRITTEN_SEGMENT_COUNT {
+			// End of the packet
+			let last_segment_size = self
+				.last_segment_size
+				.expect("fresh packet should be indicated at this point");
+			header.segments.push(last_segment_size);
 		}
 
 		self.pages.push(Page {
@@ -117,8 +120,16 @@ impl PaginateContext {
 struct PaginateContextFlags {
 	first_page: bool,
 	fresh_packet: bool,
-	packet_spans_multiple_pages: bool,
 	packet_finished_on_page: bool,
+	// A 'nil' page just means it is zero-length. This is used when our packet is perfectly
+	// divisible by `255 * MAX_SEGMENT_COUNT`. We need a zero-sized segment to mark the end of our
+	// packet across page boundaries.
+	//
+	// Very rare circumstance, but still possible.
+	//
+	// From <https://xiph.org/ogg/doc/framing.html>:
+	// "Note also that a 'nil' (zero length) packet is not an error; it consists of nothing more than a lacing value of zero in the header."
+	need_nil_page: bool,
 }
 
 /// Create pages from a list of packets
@@ -162,6 +173,13 @@ fn paginate_packet(ctx: &mut PaginateContext, packet: &[u8]) -> Result<()> {
 	let mut page_content = Vec::with_capacity(MAX_WRITTEN_CONTENT_SIZE);
 	let mut packet = packet;
 	loop {
+		// See comment on `PaginateContextFlags.need_nil_page`
+		if ctx.flags.need_nil_page {
+			assert!(packet.is_empty());
+			ctx.flags.packet_finished_on_page = true;
+			ctx.flush_page(&mut page_content);
+		}
+
 		if packet.is_empty() {
 			break;
 		}
@@ -175,8 +193,6 @@ fn paginate_packet(ctx: &mut PaginateContext, packet: &[u8]) -> Result<()> {
 
 		if bytes_read <= MAX_WRITTEN_CONTENT_SIZE && packet.is_empty() {
 			ctx.flags.packet_finished_on_page = true;
-		} else {
-			ctx.flags.packet_spans_multiple_pages = true;
 		}
 
 		if ctx.remaining_page_size == 0 || packet.is_empty() {