Repository: shssoichiro/oxipng
Pull request #498 (2023-04-15 14:08:31)

Make dependency on `image` optional
After PR https://github.com/shssoichiro/oxipng/pull/481 was merged, the `image` dependency became unused when building with debug assertions disabled, as it is only used to implement output sanity checks when such assertions are enabled.

The `image` crate transitively pulls a significant amount of dependencies, so it's useful for OxiPNG users to get rid of them when not needed:

![image crate dependency tree](https://user-images.githubusercontent.com/7822554/232229434-9a1bcd47-f9bf-4199-add9-a8b7e97412b8.png)

[Cargo does not allow specifying dependencies that are only pulled when debug assertions are
enabled](https://github.com/rust-lang/cargo/issues/7634), so the next best way to give users some flexibility is to gate those debug assertions behind a feature flag.

These changes add a `sanity-checks` feature flag that controls whether the `image` crate and the related sanity checks are compiled in. This feature is enabled by default to keep debug builds useful to catch problems during development.

andrews05: Nice! I'll note that unused dependencies are omitted on build anyway (at least that appeared to be the case for me), which is why I didn't try resolve the issue any further, but this could be helpful regardless.

Patch:
diff --git a/.github/workflows/oxipng.yml b/.github/workflows/oxipng.yml
index dfdb48bf..495efd22 100644
--- a/.github/workflows/oxipng.yml
+++ b/.github/workflows/oxipng.yml
@@ -78,7 +78,7 @@ jobs:
           token: ${{ secrets.GITHUB_TOKEN }}
           args: -- -D warnings
       - name: Run tests
-        run: cargo test
+        run: cargo test --features sanity-checks
       - name: Build benchmarks
         if: matrix.toolchain == 'nightly'
         run: cargo bench --no-run
diff --git a/Cargo.toml b/Cargo.toml
index 8f4a214d..443f81d1 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -22,6 +22,10 @@ name = "oxipng"
 path = "src/main.rs"
 required-features = ["binary"]
 
+[[bench]]
+name = "zopfli"
+required-features = ["zopfli"]
+
 [dependencies]
 zopfli = { version = "0.7.1", optional = true }
 rgb = "0.8.33"
@@ -53,6 +57,7 @@ optional = true
 version = "2.1.0"
 
 [dependencies.image]
+optional = true
 default-features = false
 features = ["png"]
 version = "0.24.3"
@@ -65,6 +70,7 @@ binary = ["clap", "wild", "stderrlog"]
 default = ["binary", "filetime", "parallel", "zopfli"]
 parallel = ["rayon", "indexmap/rayon", "crossbeam-channel"]
 freestanding = ["libdeflater/freestanding"]
+sanity-checks = ["image"]
 
 [lib]
 name = "oxipng"
diff --git a/src/lib.rs b/src/lib.rs
index 614246ce..0f5b637f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -31,12 +31,11 @@ use crate::evaluate::Evaluator;
 use crate::png::PngData;
 use crate::png::PngImage;
 use crate::reduction::*;
-use image::{DynamicImage, GenericImageView, ImageFormat, Pixel};
-use log::{debug, error, info, warn};
+use log::{debug, info, warn};
 use rayon::prelude::*;
 use std::fmt;
 use std::fs::{copy, File, Metadata};
-use std::io::{stdin, stdout, BufWriter, Cursor, Read, Write};
+use std::io::{stdin, stdout, BufWriter, Read, Write};
 use std::path::{Path, PathBuf};
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Arc;
@@ -659,7 +658,8 @@ fn optimize_png(
         );
     }
 
-    debug_assert!(validate_output(&output, original_data));
+    #[cfg(feature = "sanity-checks")]
+    debug_assert!(sanity_checks::validate_output(&output, original_data));
 
     Ok(output)
 }
@@ -1044,46 +1044,54 @@ fn copy_times(input_path_meta: &Metadata, out_path: &Path) -> PngResult<()> {
     })
 }
 
-/// Validate that the output png data still matches the original image
-fn validate_output(output: &[u8], original_data: &[u8]) -> bool {
-    let (old_png, new_png) = rayon::join(
-        || load_png_image_from_memory(original_data),
-        || load_png_image_from_memory(output),
-    );
+#[cfg(feature = "sanity-checks")]
+mod sanity_checks {
+    use super::*;
+    use image::{DynamicImage, GenericImageView, ImageFormat, Pixel};
+    use log::error;
+    use std::io::Cursor;
+
+    /// Validate that the output png data still matches the original image
+    pub(super) fn validate_output(output: &[u8], original_data: &[u8]) -> bool {
+        let (old_png, new_png) = rayon::join(
+            || load_png_image_from_memory(original_data),
+            || load_png_image_from_memory(output),
+        );
 
-    match (new_png, old_png) {
-        (Err(new_err), _) => {
-            error!("Failed to read output image for validation: {}", new_err);
-            false
-        }
-        (_, Err(old_err)) => {
-            // The original image might be invalid if, for example, there is a CRC error,
-            // and we set fix_errors to true. In that case, all we can do is check that the
-            // new image is decodable.
-            warn!("Failed to read input image for validation: {}", old_err);
-            true
+        match (new_png, old_png) {
+            (Err(new_err), _) => {
+                error!("Failed to read output image for validation: {}", new_err);
+                false
+            }
+            (_, Err(old_err)) => {
+                // The original image might be invalid if, for example, there is a CRC error,
+                // and we set fix_errors to true. In that case, all we can do is check that the
+                // new image is decodable.
+                warn!("Failed to read input image for validation: {}", old_err);
+                true
+            }
+            (Ok(new_png), Ok(old_png)) => images_equal(&old_png, &new_png),
         }
-        (Ok(new_png), Ok(old_png)) => images_equal(&old_png, &new_png),
     }
-}
 
-/// Loads a PNG image from memory to a [DynamicImage]
-fn load_png_image_from_memory(png_data: &[u8]) -> Result<DynamicImage, image::ImageError> {
-    let mut reader = image::io::Reader::new(Cursor::new(png_data));
-    reader.set_format(ImageFormat::Png);
-    reader.no_limits();
-    reader.decode()
-}
+    /// Loads a PNG image from memory to a [DynamicImage]
+    fn load_png_image_from_memory(png_data: &[u8]) -> Result<DynamicImage, image::ImageError> {
+        let mut reader = image::io::Reader::new(Cursor::new(png_data));
+        reader.set_format(ImageFormat::Png);
+        reader.no_limits();
+        reader.decode()
+    }
 
-/// Compares images pixel by pixel for equivalent content
-fn images_equal(old_png: &DynamicImage, new_png: &DynamicImage) -> bool {
-    let a = old_png.pixels().filter(|x| {
-        let p = x.2.channels();
-        !(p.len() == 4 && p[3] == 0)
-    });
-    let b = new_png.pixels().filter(|x| {
-        let p = x.2.channels();
-        !(p.len() == 4 && p[3] == 0)
-    });
-    a.eq(b)
+    /// Compares images pixel by pixel for equivalent content
+    fn images_equal(old_png: &DynamicImage, new_png: &DynamicImage) -> bool {
+        let a = old_png.pixels().filter(|x| {
+            let p = x.2.channels();
+            !(p.len() == 4 && p[3] == 0)
+        });
+        let b = new_png.pixels().filter(|x| {
+            let p = x.2.channels();
+            !(p.len() == 4 && p[3] == 0)
+        });
+        a.eq(b)
+    }
 }
diff --git a/src/rayon.rs b/src/rayon.rs
index 768db04c..ae1914c2 100644
--- a/src/rayon.rs
+++ b/src/rayon.rs
@@ -52,6 +52,7 @@ where
 
 impl<I: Iterator> ParallelIterator for I {}
 
+#[allow(dead_code)]
 pub fn join<A, B>(a: impl FnOnce() -> A, b: impl FnOnce() -> B) -> (A, B) {
     (a(), b())
 }


Test patch:
diff --git a/tests/flags.rs b/tests/flags.rs
index 38db85b7..fc7ace9e 100644
--- a/tests/flags.rs
+++ b/tests/flags.rs
@@ -27,6 +27,7 @@ fn get_opts(input: &Path) -> (OutFile, oxipng::Options) {
 }
 
 /// Add callback to allow checks before the output file is deleted again
+#[allow(clippy::too_many_arguments)]
 fn test_it_converts_callbacks<CBPRE, CBPOST>(
     input: PathBuf,
     output: &OutFile,
@@ -38,8 +39,8 @@ fn test_it_converts_callbacks<CBPRE, CBPOST>(
     mut callback_pre: CBPRE,
     mut callback_post: CBPOST,
 ) where
-    CBPOST: FnMut(&Path) -> (),
-    CBPRE: FnMut(&Path) -> (),
+    CBPOST: FnMut(&Path),
+    CBPRE: FnMut(&Path),
 {
     let png = PngData::new(&input, opts.fix_errors).unwrap();
 
@@ -48,14 +49,14 @@ fn test_it_converts_callbacks<CBPRE, CBPOST>(
 
     callback_pre(&input);
 
-    match oxipng::optimize(&InFile::Path(input), &output, &opts) {
+    match oxipng::optimize(&InFile::Path(input), output, opts) {
         Ok(_) => (),
         Err(x) => panic!("{}", x),
     };
     let output = output.path().unwrap();
     assert!(output.exists());
 
-    callback_post(&output);
+    callback_post(output);
 
     let png = match PngData::new(output, opts.fix_errors) {
         Ok(x) => x,
diff --git a/tests/reduction.rs b/tests/reduction.rs
index f3afdb88..d88cc58c 100644
--- a/tests/reduction.rs
+++ b/tests/reduction.rs
@@ -834,7 +834,7 @@ fn small_files() {
     let png = match PngData::new(output, opts.fix_errors) {
         Ok(x) => x,
         Err(x) => {
-            remove_file(&output).ok();
+            remove_file(output).ok();
             panic!("{}", x)
         }
     };
@@ -866,7 +866,7 @@ fn palette_should_be_reduced_with_dupes() {
     let png = match PngData::new(output, opts.fix_errors) {
         Ok(x) => x,
         Err(x) => {
-            remove_file(&output).ok();
+            remove_file(output).ok();
             panic!("{}", x)
         }
     };
@@ -899,7 +899,7 @@ fn palette_should_be_reduced_with_unused() {
     let png = match PngData::new(output, opts.fix_errors) {
         Ok(x) => x,
         Err(x) => {
-            remove_file(&output).ok();
+            remove_file(output).ok();
             panic!("{}", x)
         }
     };
@@ -932,7 +932,7 @@ fn palette_should_be_reduced_with_both() {
     let png = match PngData::new(output, opts.fix_errors) {
         Ok(x) => x,
         Err(x) => {
-            remove_file(&output).ok();
+            remove_file(output).ok();
             panic!("{}", x)
         }
     };