From f24d4ffe43ce929dfcc1759d419e20aca37203c5 Mon Sep 17 00:00:00 2001 From: Lukasz Anforowicz Date: Wed, 15 Nov 2023 00:54:41 +0000 Subject: [PATCH] Require `BufRead` instead of just `Read` for inputs. This commit makes a breaking API change - it changes the `R: Read` constraint (in `Decoder` and `Reader` structs) to the `R: BufRead` constraint. This helps performance by avoiding copying the input data into an additional, intermediate `BufReader` that used to be stored in the (internal) `ReadDecoder::reader` field (after these changes that field is `R` rather than `BufReader`). In particular, some input types (e.g. when decoding from a `&[u8]`) already implement `BufRead` and for such types it is wasteful to introduce additional buffering via `BufReader`. The impact of the change is significant, but relatively small - this means that it mostly shows up in `noncompressed...` benchmarks which magnify the cost of code paths that are not related to `fdeflate` nor `unfilter`. Impact on benchmark runtime looks as follows (run once, and then rerun after compiling before+after with a fresh nightly `rustc`): * kodim02.png: - No change in performance detected (p = 0.08 > 0.05) - [+1.3713% +1.7241% +2.0960%] (p = 0.00 < 0.05) * kodim07.png: - [-1.1466% -0.6693% -0.2705%] (p = 0.00 < 0.05) - No change in performance detected. (p = 0.35 > 0.05) * kodim17.png: - [-2.3062% -1.2878% +0.1746%] (p = 0.05 < 0.05) - [-2.7355% -1.9939% -0.7986%] (p = 0.00 < 0.05) * kodim23.png: - No change in performance detected. (p = 0.51 > 0.05) - [-1.4834% -1.0648% -0.6692%] (p = 0.00 < 0.05) * Lohengrin...png: - [-2.0606% -1.7935% -1.4756%] (p = 0.00 < 0.05) - [-4.2412% -3.6723% -3.0327%] (p = 0.00 < 0.05) * Transparency.png: - [+1.4991% +1.8812% +2.3429%] (p = 0.00 < 0.05) - [-0.7939% -0.5746% -0.3590%] (p = 0.00 < 0.05) * noncompressed-8x8.png: - [-2.2881% -1.4801% -0.4110%] (p = 0.00 < 0.05) - [-7.5687% -7.2013% -6.8838%] (p = 0.00 < 0.05) * noncompressed-128x128.png: - [-12.495% -12.132% -11.760%] (p = 0.00 < 0.05) - [-10.597% -10.230% -9.8399%] (p = 0.00 < 0.05) --- CHANGES.md | 7 ++++++- Cargo.toml | 2 +- examples/show.rs | 4 ++-- src/decoder/mod.rs | 35 ++++++++++++++++++++--------------- src/decoder/stream.rs | 11 +++++++---- src/encoder.rs | 8 ++++---- src/lib.rs | 10 ++++++---- src/text_metadata.rs | 7 ++++--- tests/check_testimages.rs | 10 +++++----- 9 files changed, 55 insertions(+), 39 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 6baffb49..2d2495d9 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,4 +1,9 @@ -## Unreleased +## Unreleased (TODO: breaking changes require increasing semver to 0.18.x) + +* Performance improvements. +* Breaking API changes (required by the performance-related work): + - Changed the generic constraints of `png::Decoder` and `png::Reader` + to require `R: BufRead` (instead of just `R: Read`). ## 0.17.10 diff --git a/Cargo.toml b/Cargo.toml index 5fe02335..9aebad22 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "png" -version = "0.17.10" +version = "0.17.10" # TODO: Increase semver to account for breaking API changes license = "MIT OR Apache-2.0" description = "PNG decoding and encoding library in pure Rust" diff --git a/examples/show.rs b/examples/show.rs index d8ddf75b..4b9d9d79 100644 --- a/examples/show.rs +++ b/examples/show.rs @@ -8,12 +8,12 @@ use glium::{ texture::{ClientFormat, RawImage2d}, BlitTarget, Rect, Surface, }; -use std::{borrow::Cow, env, fs::File, io, path}; +use std::{borrow::Cow, env, fs::File, io, io::BufReader, path}; /// Load the image using `png` fn load_image(path: &path::PathBuf) -> io::Result> { use png::ColorType::*; - let mut decoder = png::Decoder::new(File::open(path)?); + let mut decoder = png::Decoder::new(BufReader::new(File::open(path)?)); decoder.set_transformations(png::Transformations::normalize_to_color8()); let mut reader = decoder.read_info()?; let mut img_data = vec![0; reader.output_buffer_size()]; diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs index 272e4d8c..fecfea08 100644 --- a/src/decoder/mod.rs +++ b/src/decoder/mod.rs @@ -4,7 +4,7 @@ mod zlib; pub use self::stream::{DecodeOptions, Decoded, DecodingError, StreamingDecoder}; use self::stream::{FormatErrorInner, CHUNCK_BUFFER_SIZE}; -use std::io::{BufRead, BufReader, Read}; +use std::io::BufRead; use std::mem; use std::ops::Range; @@ -68,7 +68,7 @@ impl Default for Limits { } /// PNG Decoder -pub struct Decoder { +pub struct Decoder { read_decoder: ReadDecoder, /// Output transformations transform: Transformations, @@ -124,17 +124,17 @@ impl<'data> Row<'data> { } } -impl Decoder { +impl Decoder { /// Create a new decoder configuration with default limits. pub fn new(r: R) -> Decoder { Decoder::new_with_limits(r, Limits::default()) } /// Create a new decoder configuration with custom limits. - pub fn new_with_limits(r: R, limits: Limits) -> Decoder { + pub fn new_with_limits(reader: R, limits: Limits) -> Decoder { Decoder { read_decoder: ReadDecoder { - reader: BufReader::with_capacity(CHUNCK_BUFFER_SIZE, r), + reader, decoder: StreamingDecoder::new(), at_eof: false, }, @@ -144,10 +144,10 @@ impl Decoder { } /// Create a new decoder configuration with custom `DecodeOptions`. - pub fn new_with_options(r: R, decode_options: DecodeOptions) -> Decoder { + pub fn new_with_options(reader: R, decode_options: DecodeOptions) -> Decoder { Decoder { read_decoder: ReadDecoder { - reader: BufReader::with_capacity(CHUNCK_BUFFER_SIZE, r), + reader, decoder: StreamingDecoder::new_with_options(decode_options), at_eof: false, }, @@ -166,17 +166,20 @@ impl Decoder { /// /// ``` /// use std::fs::File; + /// use std::io::BufReader; /// use png::{Decoder, Limits}; /// // This image is 32×32, 1bit per pixel. The reader buffers one row which requires 4 bytes. /// let mut limits = Limits::default(); /// limits.bytes = 3; - /// let mut decoder = Decoder::new_with_limits(File::open("tests/pngsuite/basi0g01.png").unwrap(), limits); + /// let reader = BufReader::new(File::open("tests/pngsuite/basi0g01.png").unwrap()); + /// let mut decoder = Decoder::new_with_limits(reader, limits); /// assert!(decoder.read_info().is_err()); /// /// // This image is 32x32 pixels, so the decoder will allocate less than 10Kib /// let mut limits = Limits::default(); /// limits.bytes = 10*1024; - /// let mut decoder = Decoder::new_with_limits(File::open("tests/pngsuite/basi0g01.png").unwrap(), limits); + /// let reader = BufReader::new(File::open("tests/pngsuite/basi0g01.png").unwrap()); + /// let mut decoder = Decoder::new_with_limits(reader, limits); /// assert!(decoder.read_info().is_ok()); /// ``` pub fn set_limits(&mut self, limits: Limits) { @@ -253,8 +256,10 @@ impl Decoder { /// eg. /// ``` /// use std::fs::File; + /// use std::io::BufReader; /// use png::Decoder; - /// let mut decoder = Decoder::new(File::open("tests/pngsuite/basi0g01.png").unwrap()); + /// let reader = BufReader::new(File::open("tests/pngsuite/basi0g01.png").unwrap()); + /// let mut decoder = Decoder::new(reader); /// decoder.set_ignore_text_chunk(true); /// assert!(decoder.read_info().is_ok()); /// ``` @@ -274,13 +279,13 @@ impl Decoder { } } -struct ReadDecoder { - reader: BufReader, +struct ReadDecoder { + reader: R, decoder: StreamingDecoder, at_eof: bool, } -impl ReadDecoder { +impl ReadDecoder { /// Returns the next decoded chunk. If the chunk is an ImageData chunk, its contents are written /// into image_data. fn decode_next(&mut self, image_data: &mut Vec) -> Result, DecodingError> { @@ -338,7 +343,7 @@ impl ReadDecoder { /// PNG reader (mostly high-level interface) /// /// Provides a high level that iterates over lines or whole images. -pub struct Reader { +pub struct Reader { decoder: ReadDecoder, bpp: BytesPerPixel, subframe: SubframeInfo, @@ -396,7 +401,7 @@ enum SubframeIdx { End, } -impl Reader { +impl Reader { /// Reads all meta data until the next frame data starts. /// Requires IHDR before the IDAT and fcTL before fdAT. fn read_until_image_data(&mut self) -> Result<(), DecodingError> { diff --git a/src/decoder/stream.rs b/src/decoder/stream.rs index 9f4b4dd7..1a86f8d1 100644 --- a/src/decoder/stream.rs +++ b/src/decoder/stream.rs @@ -1380,12 +1380,13 @@ mod tests { use crate::{Decoder, DecodingError}; use byteorder::WriteBytesExt; use std::fs::File; - use std::io::Write; + use std::io::{BufReader, Write}; #[test] fn image_gamma() -> Result<(), ()> { fn trial(path: &str, expected: Option) { - let decoder = crate::Decoder::new(File::open(path).unwrap()); + let decoder = + crate::Decoder::new(BufReader::new(BufReader::new(File::open(path).unwrap()))); let reader = decoder.read_info().unwrap(); let actual: Option = reader.info().source_gamma; assert!(actual == expected); @@ -1426,7 +1427,7 @@ mod tests { #[test] fn image_source_chromaticities() -> Result<(), ()> { fn trial(path: &str, expected: Option) { - let decoder = crate::Decoder::new(File::open(path).unwrap()); + let decoder = crate::Decoder::new(BufReader::new(File::open(path).unwrap())); let reader = decoder.read_info().unwrap(); let actual: Option = reader.info().source_chromaticities; assert!(actual == expected); @@ -1618,7 +1619,9 @@ mod tests { // https://github.com/image-rs/image/issues/1825#issuecomment-1321798639, // but the 2nd iCCP chunk has been altered manually (see the 2nd comment below for more // details). - let decoder = crate::Decoder::new(File::open("tests/bugfixes/issue#1825.png").unwrap()); + let decoder = crate::Decoder::new(BufReader::new( + File::open("tests/bugfixes/issue#1825.png").unwrap(), + )); let reader = decoder.read_info().unwrap(); let icc_profile = reader.info().icc_profile.clone().unwrap().into_owned(); diff --git a/src/encoder.rs b/src/encoder.rs index dbc21c3a..fb9b8716 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -1680,7 +1680,7 @@ mod tests { use rand::{thread_rng, Rng}; use std::fs::File; - use std::io::{Cursor, Write}; + use std::io::{BufReader, Cursor, Write}; use std::{cmp, io}; #[test] @@ -1697,7 +1697,7 @@ mod tests { } eprintln!("{}", path.display()); // Decode image - let decoder = Decoder::new(File::open(path).unwrap()); + let decoder = Decoder::new(BufReader::new(File::open(path).unwrap())); let mut reader = decoder.read_info().unwrap(); let mut buf = vec![0; reader.output_buffer_size()]; let info = reader.next_frame(&mut buf).unwrap(); @@ -1742,7 +1742,7 @@ mod tests { continue; } // Decode image - let decoder = Decoder::new(File::open(path).unwrap()); + let decoder = Decoder::new(BufReader::new(File::open(path).unwrap())); let mut reader = decoder.read_info().unwrap(); let mut buf = vec![0; reader.output_buffer_size()]; let info = reader.next_frame(&mut buf).unwrap(); @@ -1786,7 +1786,7 @@ mod tests { for &bit_depth in &[1u8, 2, 4, 8] { // Do a reference decoding, choose a fitting palette image from pngsuite let path = format!("tests/pngsuite/basn3p0{}.png", bit_depth); - let decoder = Decoder::new(File::open(&path).unwrap()); + let decoder = Decoder::new(BufReader::new(File::open(&path).unwrap())); let mut reader = decoder.read_info().unwrap(); let mut decoded_pixels = vec![0; reader.output_buffer_size()]; diff --git a/src/lib.rs b/src/lib.rs index 26420d99..442f99a3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -5,16 +5,18 @@ //! ## The decoder //! //! The most important types for decoding purposes are [`Decoder`](struct.Decoder.html) and -//! [`Reader`](struct.Reader.html). They both wrap a `std::io::Read`. -//! `Decoder` serves as a builder for `Reader`. Calling `Decoder::read_info` reads from the `Read` until the -//! image data is reached. +//! [`Reader`](struct.Reader.html). They both wrap a `std::io::BufRead`. +//! `Decoder` serves as a builder for `Reader`. Calling `Decoder::read_info` reads from the +//! `BufRead` until the image data is reached. //! //! ### Using the decoder //! ``` //! use std::fs::File; +//! use std::io::BufReader; //! // The decoder is a build for reader and can be used to set various decoding options //! // via `Transformations`. The default output transformation is `Transformations::IDENTITY`. -//! let decoder = png::Decoder::new(File::open("tests/pngsuite/basi0g01.png").unwrap()); +//! let reader = BufReader::new(File::open("tests/pngsuite/basi0g01.png").unwrap()); +//! let decoder = png::Decoder::new(reader); //! let mut reader = decoder.read_info().unwrap(); //! // Allocate the output buffer. //! let mut buf = vec![0; reader.output_buffer_size()]; diff --git a/src/text_metadata.rs b/src/text_metadata.rs index 42f8df32..b0a89250 100644 --- a/src/text_metadata.rs +++ b/src/text_metadata.rs @@ -24,17 +24,18 @@ //! //! ``` //! use std::fs::File; +//! use std::io::BufReader; //! use std::iter::FromIterator; //! use std::path::PathBuf; //! //! // Opening a png file that has a zTXt chunk -//! let decoder = png::Decoder::new( -//! File::open(PathBuf::from_iter([ +//! let decoder = png::Decoder::new(BufReader::new( +//! File::open(PathBuf::from_iter([ //! "tests", //! "text_chunk_examples", //! "ztxt_example.png", //! ])) -//! .unwrap(), +//! .unwrap()), //! ); //! let mut reader = decoder.read_info().unwrap(); //! // If the text chunk is before the image data frames, `reader.info()` already contains the text. diff --git a/tests/check_testimages.rs b/tests/check_testimages.rs index 31c1ef83..5c748cd3 100644 --- a/tests/check_testimages.rs +++ b/tests/check_testimages.rs @@ -46,7 +46,7 @@ where path.push(results_path); let mut ref_results = BTreeMap::new(); let mut failures = vec![]; - for line in BufReader::new(File::open(path).unwrap()).lines() { + for line in BufReader::new(BufReader::new(File::open(path).unwrap())).lines() { let line = line.unwrap(); let parts: Vec<_> = line.split(": ").collect(); if parts.is_empty() { @@ -98,7 +98,7 @@ where #[test] fn render_images() { process_images("results.txt", &TEST_SUITES, |path| { - let mut decoder = png::Decoder::new(File::open(path)?); + let mut decoder = png::Decoder::new(BufReader::new(File::open(path)?)); decoder.set_transformations(png::Transformations::normalize_to_color8()); let mut reader = decoder.read_info()?; let mut img_data = vec![0; reader.output_buffer_size()]; @@ -121,7 +121,7 @@ fn render_images() { #[test] fn render_images_identity() { process_images("results_identity.txt", &TEST_SUITES, |path| { - let decoder = png::Decoder::new(File::open(&path)?); + let decoder = png::Decoder::new(BufReader::new(File::open(&path)?)); let mut reader = decoder.read_info()?; let mut img_data = vec![0; reader.output_buffer_size()]; let info = reader.next_frame(&mut img_data)?; @@ -146,7 +146,7 @@ fn render_images_identity() { #[test] fn render_images_alpha() { process_images("results_alpha.txt", &TEST_SUITES, |path| { - let mut decoder = png::Decoder::new(File::open(&path)?); + let mut decoder = png::Decoder::new(BufReader::new(File::open(&path)?)); decoder.set_transformations(png::Transformations::ALPHA); let mut reader = decoder.read_info()?; let mut img_data = vec![0; reader.output_buffer_size()]; @@ -187,7 +187,7 @@ fn apng_images() { count }; - let mut decoder = png::Decoder::new(File::open(&path)?); + let mut decoder = png::Decoder::new(BufReader::new(File::open(&path)?)); decoder.set_transformations(png::Transformations::normalize_to_color8()); let mut reader = decoder.read_info()?; let mut img_data = vec![0; reader.output_buffer_size()];