From 29e90cc4ec83ddbbe2286967b12224935c742771 Mon Sep 17 00:00:00 2001 From: Lukasz Anforowicz Date: Tue, 16 Jul 2024 21:05:12 +0000 Subject: [PATCH] Remove `Reader::scrach_buffer` field + resulting breaking API changes. This commit is desirable, because it avoids copying of image data across intermediate buffers. This commit was motivated by the data gathered in https://github.com/image-rs/image-png/discussions/416#discussioncomment-7436871 The commit results in the followin performance gains seen in the recently introduced `row-by-row/128x128-4k-idat` benchmark: - time: [-18.401% -17.807% -17.192%] (p = 0.00 < 0.05) - time: [-9.4276% -8.8789% -8.2860%] (p = 0.00 < 0.05) - time: [-12.389% -11.780% -11.181%] (p = 0.00 < 0.05) Fixes https://github.com/image-rs/image-png/issues/417 --- CHANGES.md | 7 +++- Cargo.toml | 2 +- benches/decoder.rs | 3 +- src/decoder/mod.rs | 84 ++++++++++++---------------------------------- 4 files changed, 29 insertions(+), 67 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 65e6c449..4b587285 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,4 +1,9 @@ -## Unreleased +## 0.18.0 + +* Breaking API changes (motivated by performance gains in some scenarios): + - Removing the `Row` and `InterlacedRow` structs + - Removing the `Reader::next_interlaced_row` method + - Changing the signature of the `Reader::next_row` method ## 0.17.13 diff --git a/Cargo.toml b/Cargo.toml index 5849279d..0f498b41 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "png" -version = "0.17.13" +version = "0.18.0" license = "MIT OR Apache-2.0" description = "PNG decoding and encoding library in pure Rust" diff --git a/benches/decoder.rs b/benches/decoder.rs index c47089d7..d190b5d7 100644 --- a/benches/decoder.rs +++ b/benches/decoder.rs @@ -89,8 +89,7 @@ fn bench_next_row(g: &mut BenchmarkGroup, data: Vec, name: &str) { let mut reader = create_reader(data.as_slice()); for output_row in image.chunks_exact_mut(bytes_per_row) { - let decoded_row = reader.next_row().unwrap().unwrap(); - output_row.copy_from_slice(decoded_row.data()); + reader.next_row(output_row).unwrap().unwrap(); } }) }); diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs index 2a189649..db3f6c66 100644 --- a/src/decoder/mod.rs +++ b/src/decoder/mod.rs @@ -7,7 +7,6 @@ use self::stream::{FormatErrorInner, CHUNK_BUFFER_SIZE}; use self::transform::{create_transform_fn, TransformFn}; use std::io::{BufRead, BufReader, Read}; -use std::mem; use std::ops::Range; use crate::adam7; @@ -87,25 +86,8 @@ pub struct Decoder { transform: Transformations, } -/// A row of data with interlace information attached. -#[derive(Clone, Copy, Debug)] -pub struct InterlacedRow<'data> { - data: &'data [u8], - interlace: InterlaceInfo, -} - -impl<'data> InterlacedRow<'data> { - pub fn data(&self) -> &'data [u8] { - self.data - } - - pub fn interlace(&self) -> InterlaceInfo { - self.interlace - } -} - /// PNG (2003) specifies two interlace modes, but reserves future extensions. -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum InterlaceInfo { /// the null method means no interlacing Null, @@ -123,18 +105,6 @@ pub enum InterlaceInfo { Adam7 { pass: u8, line: u32, width: u32 }, } -/// A row of data without interlace information. -#[derive(Clone, Copy, Debug)] -pub struct Row<'data> { - data: &'data [u8], -} - -impl<'data> Row<'data> { - pub fn data(&self) -> &'data [u8] { - self.data - } -} - impl Decoder { /// Create a new decoder configuration with default limits. pub fn new(r: R) -> Decoder { @@ -230,7 +200,6 @@ impl Decoder { current_start: 0, transform: self.transform, transform_fn: None, - scratch_buffer: Vec::new(), }; // Check if the decoding buffer of a single raw line has a valid size. @@ -389,10 +358,6 @@ pub struct Reader { /// Function that can transform decompressed, unfiltered rows into final output. /// See the `transform.rs` module for more details. transform_fn: Option, - /// This buffer is only used so that `next_row` and `next_interlaced_row` can return reference - /// to a byte slice. In a future version of this library, this buffer will be removed and - /// `next_row` and `next_interlaced_row` will write directly into a user provided output buffer. - scratch_buffer: Vec, } /// The subframe specific information. @@ -538,18 +503,14 @@ impl Reader { self.prev_start = 0; let width = self.info().width; if self.info().interlaced { - while let Some(InterlacedRow { - data: row, - interlace, - .. - }) = self.next_interlaced_row()? - { + let mut row = vec![0; self.output_line_size(width)]; + while let Some(interlace) = self.next_row(&mut row)? { let (line, pass) = match interlace { InterlaceInfo::Adam7 { line, pass, .. } => (line, pass), InterlaceInfo::Null => unreachable!("expected interlace information"), }; let samples = color_type.samples() as u8; - adam7::expand_pass(buf, width, row, pass, line, samples * (bit_depth as u8)); + adam7::expand_pass(buf, width, &row, pass, line, samples * (bit_depth as u8)); } } else { for row in buf @@ -586,14 +547,12 @@ impl Reader { Ok(output_info) } - /// Returns the next processed row of the image - pub fn next_row(&mut self) -> Result, DecodingError> { - self.next_interlaced_row() - .map(|v| v.map(|v| Row { data: v.data })) - } - - /// Returns the next processed row of the image - pub fn next_interlaced_row(&mut self) -> Result, DecodingError> { + /// Returns the next processed row of the image. + /// + /// A `ParameterError` will be returned if `row` doesn't have enough space. For initial + /// interlaced rows a smaller buffer may work, but providing a buffer that can hold + /// `self.output_line_size(self.info().width` bytes should always avoid that error. + pub fn next_row(&mut self, row: &mut [u8]) -> Result, DecodingError> { let (rowlen, interlace) = match self.next_pass() { Some((rowlen, interlace)) => (rowlen, interlace), None => return Ok(None), @@ -605,19 +564,18 @@ impl Reader { self.subframe.width }; let output_line_size = self.output_line_size(width); + if row.len() < output_line_size { + return Err(DecodingError::Parameter( + ParameterErrorKind::ImageBufferSize { + expected: output_line_size, + actual: row.len(), + } + .into(), + )); + } - // TODO: change the interface of `next_interlaced_row` to take an output buffer instead of - // making us return a reference to a buffer that we own. - let mut output_buffer = mem::take(&mut self.scratch_buffer); - output_buffer.resize(output_line_size, 0u8); - let ret = self.next_interlaced_row_impl(rowlen, &mut output_buffer); - self.scratch_buffer = output_buffer; - ret?; - - Ok(Some(InterlacedRow { - data: &self.scratch_buffer[..output_line_size], - interlace, - })) + self.next_interlaced_row_impl(rowlen, &mut row[..output_line_size])?; + Ok(Some(interlace)) } /// Read the rest of the image and chunks and finish up, including text chunks or others