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
#416 (comment)
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 #417

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
 

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"

benches/decoder.rs

@@ -89,8 +89,7 @@ fn bench_next_row(g: &mut BenchmarkGroup<WallTime>, data: Vec<u8>, 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();
             }
         })
     });

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<R: Read> {
     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<R: Read> Decoder<R> {
     /// Create a new decoder configuration with default limits.
     pub fn new(r: R) -> Decoder<R> {
@@ -230,7 +200,6 @@ impl<R: Read> Decoder<R> {
             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<R: Read> {
     /// Function that can transform decompressed, unfiltered rows into final output.
     /// See the `transform.rs` module for more details.
     transform_fn: Option<TransformFn>,
-    /// 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<u8>,
 }
 
 /// The subframe specific information.
@@ -538,18 +503,14 @@ impl<R: Read> Reader<R> {
         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<R: Read> Reader<R> {
         Ok(output_info)
     }
 
-    /// Returns the next processed row of the image
-    pub fn next_row(&mut self) -> Result<Option<Row>, 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<Option<InterlacedRow>, 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<Option<InterlaceInfo>, DecodingError> {
         let (rowlen, interlace) = match self.next_pass() {
             Some((rowlen, interlace)) => (rowlen, interlace),
             None => return Ok(None),
@@ -605,19 +564,18 @@ impl<R: Read> Reader<R> {
             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