From 7892e3b8c7980394d1abf6f5b7117070c6a88431 Mon Sep 17 00:00:00 2001 From: Kornel Date: Wed, 18 Dec 2024 16:25:57 +0000 Subject: [PATCH 1/5] Parallelize roundtrip tests --- src/encoder.rs | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/encoder.rs b/src/encoder.rs index c143fa04..cbbebd69 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -1737,9 +1737,18 @@ mod tests { use std::io::Cursor; #[test] - fn roundtrip() { + fn roundtrip1() { + roundtrip_inner(); + } + + #[test] + fn roundtrip2() { + roundtrip_inner(); + } + + fn roundtrip_inner() { // More loops = more random testing, but also more test wait time - for _ in 0..10 { + for _ in 0..5 { for path in glob::glob("tests/pngsuite/*.png") .unwrap() .map(|r| r.unwrap()) @@ -1783,9 +1792,18 @@ mod tests { } #[test] - fn roundtrip_stream() { + fn roundtrip_stream1() { + roundtrip_stream_inner(); + } + + #[test] + fn roundtrip_stream2() { + roundtrip_stream_inner(); + } + + fn roundtrip_stream_inner() { // More loops = more random testing, but also more test wait time - for _ in 0..10 { + for _ in 0..5 { for path in glob::glob("tests/pngsuite/*.png") .unwrap() .map(|r| r.unwrap()) From be425a3e663436146c5552cfa0164e0798be4cd6 Mon Sep 17 00:00:00 2001 From: Kornel Date: Wed, 18 Dec 2024 16:36:14 +0000 Subject: [PATCH 2/5] Move Info::encode #418 --- src/common.rs | 87 +----------------------------- src/encoder.rs | 144 +++++++++++++++++++++++++++++++++++++------------ 2 files changed, 111 insertions(+), 120 deletions(-) diff --git a/src/common.rs b/src/common.rs index cb56e6e7..ad6f59a1 100644 --- a/src/common.rs +++ b/src/common.rs @@ -1,5 +1,5 @@ //! Common types shared between the encoder and decoder -use crate::text_metadata::{EncodableTextChunk, ITXtChunk, TEXtChunk, ZTXtChunk}; +use crate::text_metadata::{ITXtChunk, TEXtChunk, ZTXtChunk}; use crate::{chunk, encoder}; use io::Write; use std::{borrow::Cow, convert::TryFrom, fmt, io}; @@ -735,91 +735,6 @@ impl Info<'_> { self.srgb = Some(rendering_intent); self.icc_profile = None; } - - /// Encode this header to the writer. - /// - /// Note that this does _not_ include the PNG signature, it starts with the IHDR chunk and then - /// includes other chunks that were added to the header. - #[deprecated(note = "Use Encoder+Writer instead")] - pub fn encode(&self, mut w: W) -> encoder::Result<()> { - // Encode the IHDR chunk - let mut data = [0; 13]; - data[..4].copy_from_slice(&self.width.to_be_bytes()); - data[4..8].copy_from_slice(&self.height.to_be_bytes()); - data[8] = self.bit_depth as u8; - data[9] = self.color_type as u8; - data[12] = self.interlaced as u8; - encoder::write_chunk(&mut w, chunk::IHDR, &data)?; - - // Encode the pHYs chunk - if let Some(pd) = self.pixel_dims { - let mut phys_data = [0; 9]; - phys_data[0..4].copy_from_slice(&pd.xppu.to_be_bytes()); - phys_data[4..8].copy_from_slice(&pd.yppu.to_be_bytes()); - match pd.unit { - Unit::Meter => phys_data[8] = 1, - Unit::Unspecified => phys_data[8] = 0, - } - encoder::write_chunk(&mut w, chunk::pHYs, &phys_data)?; - } - - // If specified, the sRGB information overrides the source gamma and chromaticities. - if let Some(srgb) = &self.srgb { - srgb.encode(&mut w)?; - - // gAMA and cHRM are optional, for backwards compatibility - let srgb_gamma = crate::srgb::substitute_gamma(); - if Some(srgb_gamma) == self.source_gamma { - srgb_gamma.encode_gama(&mut w)? - } - let srgb_chromaticities = crate::srgb::substitute_chromaticities(); - if Some(srgb_chromaticities) == self.source_chromaticities { - srgb_chromaticities.encode(&mut w)?; - } - } else { - if let Some(gma) = self.source_gamma { - gma.encode_gama(&mut w)? - } - if let Some(chrms) = self.source_chromaticities { - chrms.encode(&mut w)?; - } - if let Some(iccp) = &self.icc_profile { - encoder::write_iccp_chunk(&mut w, "_", iccp)? - } - } - - if let Some(exif) = &self.exif_metadata { - encoder::write_chunk(&mut w, chunk::eXIf, exif)?; - } - - if let Some(actl) = self.animation_control { - actl.encode(&mut w)?; - } - - // The position of the PLTE chunk is important, it must come before the tRNS chunk and after - // many of the other metadata chunks. - if let Some(p) = &self.palette { - encoder::write_chunk(&mut w, chunk::PLTE, p)?; - }; - - if let Some(t) = &self.trns { - encoder::write_chunk(&mut w, chunk::tRNS, t)?; - } - - for text_chunk in &self.uncompressed_latin1_text { - text_chunk.encode(&mut w)?; - } - - for text_chunk in &self.compressed_latin1_text { - text_chunk.encode(&mut w)?; - } - - for text_chunk in &self.utf8_text { - text_chunk.encode(&mut w)?; - } - - Ok(()) - } } impl BytesPerPixel { diff --git a/src/encoder.rs b/src/encoder.rs index cbbebd69..a52c2b89 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -9,7 +9,7 @@ use flate2::write::ZlibEncoder; use crate::chunk::{self, ChunkType}; use crate::common::{ AnimationControl, BitDepth, BlendOp, BytesPerPixel, ColorType, Compression, DisposeOp, - FrameControl, Info, ParameterError, ParameterErrorKind, PixelDimensions, ScaledFloat, + FrameControl, Info, ParameterError, ParameterErrorKind, PixelDimensions, ScaledFloat, Unit, }; use crate::filter::{filter, AdaptiveFilterType, FilterType}; use crate::text_metadata::{ @@ -589,13 +589,94 @@ impl Writer { )); } - self.w.write_all(&[137, 80, 78, 71, 13, 10, 26, 10])?; // PNG signature - #[allow(deprecated)] - info.encode(&mut self.w)?; + self.encode_header(info)?; Ok(self) } + /// Encode PNG signature, IHDR, and then chunks that were added to the `Info` + fn encode_header(&mut self, info: &Info<'_>) -> Result<()> { + self.w.write_all(&[137, 80, 78, 71, 13, 10, 26, 10])?; // PNG signature + + // Encode the IHDR chunk + let mut data = [0; 13]; + data[..4].copy_from_slice(&info.width.to_be_bytes()); + data[4..8].copy_from_slice(&info.height.to_be_bytes()); + data[8] = info.bit_depth as u8; + data[9] = info.color_type as u8; + data[12] = info.interlaced as u8; + self.write_chunk(chunk::IHDR, &data)?; + + // Encode the pHYs chunk + if let Some(pd) = info.pixel_dims { + let mut phys_data = [0; 9]; + phys_data[0..4].copy_from_slice(&pd.xppu.to_be_bytes()); + phys_data[4..8].copy_from_slice(&pd.yppu.to_be_bytes()); + match pd.unit { + Unit::Meter => phys_data[8] = 1, + Unit::Unspecified => phys_data[8] = 0, + } + self.write_chunk(chunk::pHYs, &phys_data)?; + } + + // If specified, the sRGB information overrides the source gamma and chromaticities. + if let Some(srgb) = &info.srgb { + srgb.encode(&mut self.w)?; + + // gAMA and cHRM are optional, for backwards compatibility + let srgb_gamma = crate::srgb::substitute_gamma(); + if Some(srgb_gamma) == info.source_gamma { + srgb_gamma.encode_gama(&mut self.w)? + } + let srgb_chromaticities = crate::srgb::substitute_chromaticities(); + if Some(srgb_chromaticities) == info.source_chromaticities { + srgb_chromaticities.encode(&mut self.w)?; + } + } else { + if let Some(gma) = info.source_gamma { + gma.encode_gama(&mut self.w)? + } + if let Some(chrms) = info.source_chromaticities { + chrms.encode(&mut self.w)?; + } + if let Some(iccp) = &info.icc_profile { + self.write_iccp_chunk("_", iccp)? + } + } + + if let Some(exif) = &info.exif_metadata { + self.write_chunk(chunk::eXIf, exif)?; + } + + if let Some(actl) = info.animation_control { + actl.encode(&mut self.w)?; + } + + // The position of the PLTE chunk is important, it must come before the tRNS chunk and after + // many of the other metadata chunks. + if let Some(p) = &info.palette { + self.write_chunk(chunk::PLTE, p)?; + }; + + if let Some(t) = &info.trns { + self.write_chunk(chunk::tRNS, t)?; + } + + for text_chunk in &info.uncompressed_latin1_text { + self.write_text_chunk(text_chunk)?; + } + + for text_chunk in &info.compressed_latin1_text { + self.write_text_chunk(text_chunk)?; + } + + for text_chunk in &info.utf8_text { + self.write_text_chunk(text_chunk)?; + } + + Ok(()) + } + /// Write a raw chunk of PNG data. /// /// The chunk will have its CRC calculated and correctly. The data is not filtered in any way, @@ -615,6 +696,31 @@ impl Writer { text_chunk.encode(&mut self.w) } + fn write_iccp_chunk(&mut self, profile_name: &str, icc_profile: &[u8]) -> Result<()> { + let profile_name = encode_iso_8859_1(profile_name)?; + if profile_name.len() < 1 || profile_name.len() > 79 { + return Err(TextEncodingError::InvalidKeywordSize.into()); + } + + let estimated_compressed_size = icc_profile.len() * 3 / 4; + let chunk_size = profile_name + .len() + .checked_add(2) // string NUL + compression type. Checked add optimizes out later Vec reallocations. + .and_then(|s| s.checked_add(estimated_compressed_size)) + .ok_or(EncodingError::LimitsExceeded)?; + + let mut data = Vec::new(); + data.try_reserve_exact(chunk_size) + .map_err(|_| EncodingError::LimitsExceeded)?; + + data.extend(profile_name.into_iter().chain([0, 0])); + + let mut encoder = ZlibEncoder::new(data, flate2::Compression::default()); + encoder.write_all(icc_profile)?; + + self.write_chunk(chunk::iCCP, &encoder.finish()?) + } + /// Check if we should allow writing another image. fn validate_new_image(&self) -> Result<()> { if !self.options.validate_sequence { @@ -1055,36 +1161,6 @@ impl Drop for Writer { } } -// This should be moved to Writer after `Info::encoding` is gone -pub(crate) fn write_iccp_chunk( - w: &mut W, - profile_name: &str, - icc_profile: &[u8], -) -> Result<()> { - let profile_name = encode_iso_8859_1(profile_name)?; - if profile_name.len() < 1 || profile_name.len() > 79 { - return Err(TextEncodingError::InvalidKeywordSize.into()); - } - - let estimated_compressed_size = icc_profile.len() * 3 / 4; - let chunk_size = profile_name - .len() - .checked_add(2) // string NUL + compression type. Checked add optimizes out later Vec reallocations. - .and_then(|s| s.checked_add(estimated_compressed_size)) - .ok_or(EncodingError::LimitsExceeded)?; - - let mut data = Vec::new(); - data.try_reserve_exact(chunk_size) - .map_err(|_| EncodingError::LimitsExceeded)?; - - data.extend(profile_name.into_iter().chain([0, 0])); - - let mut encoder = ZlibEncoder::new(data, flate2::Compression::default()); - encoder.write_all(icc_profile)?; - - write_chunk(w, chunk::iCCP, &encoder.finish()?) -} - enum ChunkOutput<'a, W: Write> { Borrowed(&'a mut Writer), Owned(Writer), From 57f3ab17c1d13cd54f8049a0f08ad0c973b05ca9 Mon Sep 17 00:00:00 2001 From: Kornel Date: Wed, 18 Dec 2024 17:48:39 +0000 Subject: [PATCH 3/5] Separate PartialInfo from Info --- src/common.rs | 14 ++++++++++++-- src/encoder.rs | 29 ++++++++--------------------- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/src/common.rs b/src/common.rs index ad6f59a1..2b523905 100644 --- a/src/common.rs +++ b/src/common.rs @@ -77,6 +77,16 @@ impl ColorType { || self == ColorType::Rgba)) || (bit_depth == BitDepth::Sixteen && self == ColorType::Indexed) } + + pub(crate) fn bits_per_pixel(&self, bit_depth: BitDepth) -> usize { + self.samples() * bit_depth as usize + } + + pub(crate) fn bytes_per_pixel(&self, bit_depth: BitDepth) -> usize { + // If adjusting this for expansion or other transformation passes, remember to keep the old + // implementation for bpp_in_prediction, which is internal to the png specification. + self.samples() * ((bit_depth as usize + 7) >> 3) + } } /// Bit depth of the PNG file. @@ -683,14 +693,14 @@ impl Info<'_> { /// Returns the number of bits per pixel. pub fn bits_per_pixel(&self) -> usize { - self.color_type.samples() * self.bit_depth as usize + self.color_type.bits_per_pixel(self.bit_depth) } /// Returns the number of bytes per pixel. pub fn bytes_per_pixel(&self) -> usize { // If adjusting this for expansion or other transformation passes, remember to keep the old // implementation for bpp_in_prediction, which is internal to the png specification. - self.color_type.samples() * ((self.bit_depth as usize + 7) >> 3) + self.color_type.bytes_per_pixel(self.bit_depth) } /// Return the number of bytes for this pixel used in prediction. diff --git a/src/encoder.rs b/src/encoder.rs index a52c2b89..226686d8 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -514,33 +514,20 @@ impl PartialInfo { } fn bpp_in_prediction(&self) -> BytesPerPixel { - // Passthrough - self.to_info().bpp_in_prediction() + BytesPerPixel::from_usize(self.bytes_per_pixel()) + } + + fn bytes_per_pixel(&self) -> usize { + self.color_type.bytes_per_pixel(self.bit_depth) } fn raw_row_length(&self) -> usize { - // Passthrough - self.to_info().raw_row_length() + self.raw_row_length_from_width(self.width) } fn raw_row_length_from_width(&self, width: u32) -> usize { - // Passthrough - self.to_info().raw_row_length_from_width(width) - } - - /// Converts this partial info to an owned Info struct, - /// setting missing values to their defaults - fn to_info(&self) -> Info<'static> { - Info { - width: self.width, - height: self.height, - bit_depth: self.bit_depth, - color_type: self.color_type, - frame_control: self.frame_control, - animation_control: self.animation_control, - compression: self.compression, - ..Default::default() - } + self.color_type + .raw_row_length_from_width(self.bit_depth, width) } } From 6d5d64f18aec61be0954ade5637bd301985ab344 Mon Sep 17 00:00:00 2001 From: Kornel Date: Wed, 18 Dec 2024 18:23:42 +0000 Subject: [PATCH 4/5] Compression belongs to Options --- src/common.rs | 4 ---- src/encoder.rs | 11 +++++------ 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/common.rs b/src/common.rs index 2b523905..18b80884 100644 --- a/src/common.rs +++ b/src/common.rs @@ -597,7 +597,6 @@ pub struct Info<'a> { pub frame_control: Option, pub animation_control: Option, - pub compression: Compression, /// Gamma of the source system. /// Set by both `gAMA` as well as to a replacement by `sRGB` chunk. pub source_gamma: Option, @@ -643,9 +642,6 @@ impl Default for Info<'_> { pixel_dims: None, frame_control: None, animation_control: None, - // Default to `deflate::Compression::Fast` and `filter::FilterType::Sub` - // to maintain backward compatible output. - compression: Compression::Fast, source_gamma: None, source_chromaticities: None, srgb: None, diff --git a/src/encoder.rs b/src/encoder.rs index 226686d8..0f687131 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -158,6 +158,7 @@ struct Options { adaptive_filter: AdaptiveFilterType, sep_def_img: bool, validate_sequence: bool, + compression: Compression, } impl<'a, W: Write> Encoder<'a, W> { @@ -313,7 +314,7 @@ impl<'a, W: Write> Encoder<'a, W> { /// Accepts a `Compression` or any type that can transform into a `Compression`. Notably `deflate::Compression` and /// `deflate::CompressionOptions` which "just work". pub fn set_compression(&mut self, compression: Compression) { - self.info.compression = compression; + self.options.compression = compression; } /// Set the used filter type. @@ -495,7 +496,6 @@ struct PartialInfo { color_type: ColorType, frame_control: Option, animation_control: Option, - compression: Compression, has_palette: bool, } @@ -508,7 +508,6 @@ impl PartialInfo { color_type: info.color_type, frame_control: info.frame_control, animation_control: info.animation_control, - compression: info.compression, has_palette: info.palette.is_some(), } } @@ -787,7 +786,7 @@ impl Writer { let filter_method = self.options.filter; let adaptive_method = self.options.adaptive_filter; - let zlib_encoded = match self.info.compression { + let zlib_encoded = match self.options.compression { Compression::Fast => { let mut compressor = fdeflate::Compressor::new(std::io::Cursor::new(Vec::new()))?; @@ -832,7 +831,7 @@ impl Writer { _ => { let mut current = vec![0; in_len]; - let mut zlib = ZlibEncoder::new(Vec::new(), self.info.compression.to_options()); + let mut zlib = ZlibEncoder::new(Vec::new(), self.options.compression.to_options()); for line in data.chunks(in_len) { let filter_type = filter( filter_method, @@ -1417,13 +1416,13 @@ impl<'a, W: Write> StreamWriter<'a, W> { width, height, frame_control: fctl, - compression, .. } = writer.info; let bpp = writer.info.bpp_in_prediction(); let in_len = writer.info.raw_row_length() - 1; let filter = writer.options.filter; + let compression = writer.options.compression; let adaptive_filter = writer.options.adaptive_filter; let prev_buf = vec![0; in_len]; let curr_buf = vec![0; in_len]; From ff6372d504f824539103f8d9286696d2016a11ca Mon Sep 17 00:00:00 2001 From: Kornel Date: Wed, 18 Dec 2024 18:09:24 +0000 Subject: [PATCH 5/5] Don't use source_ fields for output --- src/common.rs | 18 ++++++++++++++++++ src/decoder/stream.rs | 16 ++-------------- src/encoder.rs | 2 +- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/common.rs b/src/common.rs index 18b80884..9751e4e8 100644 --- a/src/common.rs +++ b/src/common.rs @@ -731,6 +731,24 @@ impl Info<'_> { .raw_row_length_from_width(self.bit_depth, width) } + /// Gamma dependent on sRGB chunk + pub fn gamma(&self) -> Option { + if self.srgb.is_some() { + Some(crate::srgb::substitute_gamma()) + } else { + self.gama_chunk + } + } + + /// Chromaticities dependent on sRGB chunk + pub fn chromaticities(&self) -> Option { + if self.srgb.is_some() { + Some(crate::srgb::substitute_chromaticities()) + } else { + self.chrm_chunk + } + } + /// Mark the image data as conforming to the SRGB color space with the specified rendering intent. /// /// Any ICC profiles will be ignored. diff --git a/src/decoder/stream.rs b/src/decoder/stream.rs index 88044b74..a2296783 100644 --- a/src/decoder/stream.rs +++ b/src/decoder/stream.rs @@ -1315,11 +1315,6 @@ impl StreamingDecoder { }; info.chrm_chunk = Some(source_chromaticities); - // Ignore chromaticities if sRGB profile is used. - if info.srgb.is_none() { - info.source_chromaticities = Some(source_chromaticities); - } - Ok(Decoded::Nothing) } } @@ -1340,11 +1335,6 @@ impl StreamingDecoder { let source_gamma = ScaledFloat::from_scaled(source_gamma); info.gama_chunk = Some(source_gamma); - // Ignore chromaticities if sRGB profile is used. - if info.srgb.is_none() { - info.source_gamma = Some(source_gamma); - } - Ok(Decoded::Nothing) } } @@ -1368,8 +1358,6 @@ impl StreamingDecoder { // Set srgb and override source gamma and chromaticities. info.srgb = Some(rendering_intent); - info.source_gamma = Some(crate::srgb::substitute_gamma()); - info.source_chromaticities = Some(crate::srgb::substitute_chromaticities()); Ok(Decoded::Nothing) } } @@ -1846,7 +1834,7 @@ mod tests { fn trial(path: &str, expected: Option) { let decoder = crate::Decoder::new(File::open(path).unwrap()); let reader = decoder.read_info().unwrap(); - let actual: Option = reader.info().source_gamma; + let actual: Option = reader.info().gamma(); assert!(actual == expected); } trial("tests/pngsuite/f00n0g08.png", None); @@ -1887,7 +1875,7 @@ mod tests { fn trial(path: &str, expected: Option) { let decoder = crate::Decoder::new(File::open(path).unwrap()); let reader = decoder.read_info().unwrap(); - let actual: Option = reader.info().source_chromaticities; + let actual: Option = reader.info().chromaticities(); assert!(actual == expected); } trial( diff --git a/src/encoder.rs b/src/encoder.rs index 0f687131..dc3bfc5b 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -2183,7 +2183,7 @@ mod tests { let decoder = crate::Decoder::new(Cursor::new(buffer)); let mut reader = decoder.read_info()?; assert_eq!( - reader.info().source_gamma, + reader.info().gamma(), gamma, "Deviation with gamma {:?}", gamma