From d063119d9c520a9bc273efa81beb7d951c3dee9d Mon Sep 17 00:00:00 2001 From: Kornel Date: Tue, 26 Mar 2024 15:09:58 +0000 Subject: [PATCH 1/8] Avoid split_at panic in ScanLines --- src/png/scan_lines.rs | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/png/scan_lines.rs b/src/png/scan_lines.rs index 0c31dd4b..e865963c 100644 --- a/src/png/scan_lines.rs +++ b/src/png/scan_lines.rs @@ -24,20 +24,24 @@ impl<'a> Iterator for ScanLines<'a> { type Item = ScanLine<'a>; #[inline] fn next(&mut self) -> Option { - self.iter.next().map(|(len, pass, num_pixels)| { - let (data, rest) = self.raw_data.split_at(len); - self.raw_data = rest; - let (&filter, data) = if self.has_filter { - data.split_first().unwrap() - } else { - (&0, data) - }; - ScanLine { - filter, - data, - pass, - num_pixels, - } + let (len, pass, num_pixels) = self.iter.next()?; + debug_assert!(self.raw_data.len() >= len); + debug_assert!(!self.has_filter || len > 1); + if self.raw_data.len() < len { + return None; + } + let (data, rest) = self.raw_data.split_at(len); + self.raw_data = rest; + let (&filter, data) = if self.has_filter { + data.split_first().unwrap() + } else { + (&0, data) + }; + Some(ScanLine { + filter, + data, + pass, + num_pixels, }) } } From 36de54318d9cd9b8cbf6e01749a15cd32de078be Mon Sep 17 00:00:00 2001 From: Kornel Date: Tue, 26 Mar 2024 15:10:26 +0000 Subject: [PATCH 2/8] Faster, garbage-resilient most_popular_edge_color --- src/reduction/palette.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/reduction/palette.rs b/src/reduction/palette.rs index aa903559..127c3cfe 100644 --- a/src/reduction/palette.rs +++ b/src/reduction/palette.rs @@ -183,12 +183,21 @@ pub fn sorted_palette_battiato(png: &PngImage) -> Option { // Find the most popular color on the image edges (the pixels neighboring the filter bytes) fn most_popular_edge_color(num_colors: usize, png: &PngImage) -> usize { - let mut counts = vec![0; num_colors]; + let mut counts = [0u32; 256]; for line in png.scan_lines(false) { - counts[line.data[0] as usize] += 1; - counts[line.data[line.data.len() - 1] as usize] += 1; + if let &[first, .., last] = line.data { + counts[first as usize] += 1; + counts[last as usize] += 1; + } } - counts.iter().enumerate().max_by_key(|(_, &v)| v).unwrap().0 + counts + .iter() + .copied() + .take(num_colors) + .enumerate() + .max_by_key(|&(_, v)| v) + .unwrap_or_default() + .0 } // Calculate co-occurences matrix From a3590df20cb25f088e5765e82f2df0ca0f740ecd Mon Sep 17 00:00:00 2001 From: Kornel Date: Tue, 26 Mar 2024 15:23:16 +0000 Subject: [PATCH 3/8] Optimize co-occurence matrix --- src/reduction/palette.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/reduction/palette.rs b/src/reduction/palette.rs index 127c3cfe..e7619bc4 100644 --- a/src/reduction/palette.rs +++ b/src/reduction/palette.rs @@ -201,14 +201,18 @@ fn most_popular_edge_color(num_colors: usize, png: &PngImage) -> usize { } // Calculate co-occurences matrix -fn co_occurrence_matrix(num_colors: usize, png: &PngImage) -> Vec> { - let mut matrix = vec![vec![0; num_colors]; num_colors]; +fn co_occurrence_matrix(num_colors: usize, png: &PngImage) -> Vec> { + let mut matrix = vec![vec![0u32; num_colors]; num_colors]; let mut prev: Option = None; + let mut prev_val = None; for line in png.scan_lines(false) { for i in 0..line.data.len() { let val = line.data[i] as usize; - if i > 0 { - matrix[line.data[i - 1] as usize][val] += 1; + if val > num_colors { + continue; + } + if let Some(prev_val) = prev_val.replace(val) { + matrix[prev_val][val] += 1; } if let Some(prev) = &prev { matrix[prev.data[i] as usize][val] += 1; @@ -220,7 +224,7 @@ fn co_occurrence_matrix(num_colors: usize, png: &PngImage) -> Vec> { } // Calculate edge list sorted by weight -fn weighted_edges(matrix: &[Vec]) -> Vec<(usize, usize)> { +fn weighted_edges(matrix: &[Vec]) -> Vec<(usize, usize)> { let mut edges = Vec::new(); for i in 0..matrix.len() { for j in 0..i { From eae53362b224089cd651361336fa707adf113bd3 Mon Sep 17 00:00:00 2001 From: andrews05 Date: Fri, 29 Mar 2024 09:00:03 +1300 Subject: [PATCH 4/8] Add comment regarding avoiding panics --- src/png/scan_lines.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/png/scan_lines.rs b/src/png/scan_lines.rs index e865963c..080c211e 100644 --- a/src/png/scan_lines.rs +++ b/src/png/scan_lines.rs @@ -27,6 +27,8 @@ impl<'a> Iterator for ScanLines<'a> { let (len, pass, num_pixels) = self.iter.next()?; debug_assert!(self.raw_data.len() >= len); debug_assert!(!self.has_filter || len > 1); + // The data length should always be correct here but this check assures + // the compiler that it doesn't need to account for a potential panic if self.raw_data.len() < len { return None; } From a30ee05ca862e5596913e1bfb57d42e143e5d8c6 Mon Sep 17 00:00:00 2001 From: Andrew Date: Thu, 28 Mar 2024 18:17:15 +1300 Subject: [PATCH 5/8] Add test for image with truncated palette --- ...palette_should_be_reduced_with_missing.png | Bin 0 -> 138 bytes tests/reduction.rs | 37 ++++++++++++++++++ 2 files changed, 37 insertions(+) create mode 100644 tests/files/palette_should_be_reduced_with_missing.png diff --git a/tests/files/palette_should_be_reduced_with_missing.png b/tests/files/palette_should_be_reduced_with_missing.png new file mode 100644 index 0000000000000000000000000000000000000000..fd6d61b8be64b0a25f2ec01d506a89470208c4f6 GIT binary patch literal 138 zcmeAS@N?(olHy`uVBq!ia0vp^0U*r53?z4+XPOVB*aCb)TswdIJ8t-P`^1cYKoNUS z7srr@*0*OAg%}igST-K|FW<{jXuQ@T?eDU0&$&2Qm>Lf(7wsyBu$EacH##T?+-P_# c36{Cd$e}5_QG0*XYM@aJp00i_>zopr0LbqvZ2$lO literal 0 HcmV?d00001 diff --git a/tests/reduction.rs b/tests/reduction.rs index 221e320d..7a6292c4 100644 --- a/tests/reduction.rs +++ b/tests/reduction.rs @@ -1049,6 +1049,43 @@ fn palette_should_be_reduced_with_both() { remove_file(output).ok(); } +#[test] +fn palette_should_be_reduced_with_missing() { + let input = PathBuf::from("tests/files/palette_should_be_reduced_with_missing.png"); + let (output, opts) = get_opts(&input); + + let png = PngData::new(&input, &opts).unwrap(); + + assert_eq!(png.raw.ihdr.color_type.png_header_code(), INDEXED); + assert_eq!(png.raw.ihdr.bit_depth, BitDepth::Eight); + if let ColorType::Indexed { palette } = &png.raw.ihdr.color_type { + assert_eq!(palette.len(), 2); + } + + match oxipng::optimize(&InFile::Path(input), &output, &opts) { + Ok(_) => (), + Err(x) => panic!("{}", x), + }; + let output = output.path().unwrap(); + assert!(output.exists()); + + let png = match PngData::new(output, &opts) { + Ok(x) => x, + Err(x) => { + remove_file(output).ok(); + panic!("{}", x) + } + }; + + assert_eq!(png.raw.ihdr.color_type.png_header_code(), INDEXED); + assert_eq!(png.raw.ihdr.bit_depth, BitDepth::Two); + if let ColorType::Indexed { palette } = &png.raw.ihdr.color_type { + assert_eq!(palette.len(), 3); + } + + remove_file(output).ok(); +} + #[test] fn rgba_16_reduce_alpha() { test_it_converts( From 1ddab42edb1c244b57ecb307e2fe54741a56aa0a Mon Sep 17 00:00:00 2001 From: Andrew Date: Thu, 28 Mar 2024 18:20:27 +1300 Subject: [PATCH 6/8] Always fix palette even if it would be larger --- src/reduction/palette.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/reduction/palette.rs b/src/reduction/palette.rs index e7619bc4..150e2526 100644 --- a/src/reduction/palette.rs +++ b/src/reduction/palette.rs @@ -43,8 +43,9 @@ pub fn reduced_palette(png: &PngImage, optimize_alpha: bool) -> Option let data = if did_change { // Reassign data bytes to new indices png.data.iter().map(|b| byte_map[*b as usize]).collect() - } else if condensed.len() < palette.len() { - // Data is unchanged but palette will be truncated + } else if condensed.len() != palette.len() { + // Data is unchanged but palette is different size + // Note the new palette could potentially be larger if the original had a missing entry png.data.clone() } else { // Nothing has changed From fca76a7afbd27091aed2c16e40258cc90fece0bf Mon Sep 17 00:00:00 2001 From: Andrew Date: Thu, 28 Mar 2024 18:21:10 +1300 Subject: [PATCH 7/8] Always run palette reduction even if there's only one entry --- src/reduction/palette.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/reduction/palette.rs b/src/reduction/palette.rs index 150e2526..ce5459e3 100644 --- a/src/reduction/palette.rs +++ b/src/reduction/palette.rs @@ -14,9 +14,8 @@ pub fn reduced_palette(png: &PngImage, optimize_alpha: bool) -> Option if png.ihdr.bit_depth != BitDepth::Eight { return None; } - let palette = match &png.ihdr.color_type { - ColorType::Indexed { palette } if palette.len() > 1 => palette, - _ => return None, + let ColorType::Indexed { palette } = &png.ihdr.color_type else { + return None; }; let mut used = [false; 256]; From db7da039faea0a2c214b7eaacaba77f5a012edd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez?= Date: Thu, 28 Mar 2024 12:46:53 +0100 Subject: [PATCH 8/8] Use GitHub macOS ARM runners for building ARM macOS binaries GitHub introduced free macOS ARM runners on January, and my experience using them in other projects to improve CI times and be able to actually run tests on Apple Silicon Macs has been positive. Let's use them in OxiPNG to hopefully speed up CI a bit, and finally be able to run the test suite on AArch64 macOS. --- .github/workflows/oxipng.yml | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/.github/workflows/oxipng.yml b/.github/workflows/oxipng.yml index 4c4d10af..3361fcb7 100644 --- a/.github/workflows/oxipng.yml +++ b/.github/workflows/oxipng.yml @@ -49,9 +49,9 @@ jobs: - target: i686-pc-windows-msvc os: windows-latest - target: x86_64-apple-darwin - os: macos-latest + os: macos-12 - target: aarch64-apple-darwin - os: macos-latest + os: macos-14 # ARM64 runner env: CARGO_BUILD_TARGET: ${{ matrix.target }} @@ -122,19 +122,11 @@ jobs: reporter: github-check fail_on_error: true - # There aren't good user-mode ARM64 emulators we can use on x64 macOS hosts. - # QEMU doesn't have any plans to add such support due to a lack of kernel - # syscall stability guarantees: https://gitlab.com/qemu-project/qemu/-/issues/1682 - name: Run tests - if: matrix.target != 'aarch64-apple-darwin' run: | cargo nextest run --release --features sanity-checks cargo test --doc --release --features sanity-checks - - name: Build tests (ARM64 macOS only) - if: matrix.target == 'aarch64-apple-darwin' - run: cargo test --release --features sanity-checks --no-run - - name: Build benchmarks run: cargo bench --no-run