diff --git a/ogg_pager/CHANGELOG.md b/ogg_pager/CHANGELOG.md index 56b01ca5..5b174ee9 100644 --- a/ogg_pager/CHANGELOG.md +++ b/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 diff --git a/ogg_pager/src/paginate.rs b/ogg_pager/src/paginate.rs index 5664e44d..532df2a7 100644 --- a/ogg_pager/src/paginate.rs +++ b/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, } 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) { @@ -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 : + // "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() {