From 462e982784cd1bc67fbbbe9f98b9f7e0e2438498 Mon Sep 17 00:00:00 2001 From: andrews05 Date: Mon, 25 Sep 2023 22:15:13 +1300 Subject: [PATCH 1/4] Move file-specific options under OutFile (#529) This PR is addressing #220. It's not super important but it's a breaking change, so if it's something we want to do then I thought I should get it in now before the next release. - [x] pretend can become another variant of OutFile, probably OutFile::None, as that's what it essentially is - just another output destination and not a separate option - [x] ~~backup and~~ preserve_attrs should become properties of OutFile::Path variant (so that it would contain Path { path, ~~backup,~~ preserve_attrs }) as they don't have any effect on any other output and so semantically belong there best Closes #220 --- src/lib.rs | 63 +++++++++++++++++++++++++++----------------- src/main.rs | 29 +++++++++++--------- tests/filters.rs | 5 +--- tests/flags.rs | 11 ++++---- tests/interlaced.rs | 5 +--- tests/interlacing.rs | 5 +--- tests/lib.rs | 6 ++--- tests/reduction.rs | 5 +--- tests/regression.rs | 7 ++--- tests/strategies.rs | 5 +--- 10 files changed, 71 insertions(+), 70 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index bde4deed..c0c53e4a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -76,15 +76,36 @@ pub mod internal_tests { #[derive(Clone, Debug)] pub enum OutFile { - /// Path(None) means same as input - Path(Option), + /// Don't actually write any output, just calculate the best results. + None, + /// Write output to a file. + /// + /// * `path`: Path to write the output file. `None` means same as input. + /// * `preserve_attrs`: Ensure the output file has the same permissions & timestamps as the input file. + Path { + path: Option, + preserve_attrs: bool, + }, + /// Write to standard output. StdOut, } impl OutFile { + /// Construct a new `OutFile` with the given path. + /// + /// This is a convenience method for `OutFile::Path { path: Some(path), preserve_attrs: false }`. + pub fn from_path(path: PathBuf) -> Self { + OutFile::Path { + path: Some(path), + preserve_attrs: false, + } + } + pub fn path(&self) -> Option<&Path> { match *self { - OutFile::Path(Some(ref p)) => Some(p.as_path()), + OutFile::Path { + path: Some(ref p), .. + } => Some(p.as_path()), _ => None, } } @@ -138,18 +159,10 @@ pub struct Options { /// /// Default: `false` pub check: bool, - /// Don't actually write any output, just calculate the best results. - /// - /// Default: `false` - pub pretend: bool, /// Write to output even if there was no improvement in compression. /// /// Default: `false` pub force: bool, - /// Ensure the output file has the same permissions as the input file. - /// - /// Default: `false` - pub preserve_attrs: bool, /// Which RowFilters to try on the file /// /// Default: `None,Sub,Entropy,Bigrams` @@ -296,10 +309,8 @@ impl Default for Options { Options { backup: false, check: false, - pretend: false, fix_errors: false, force: false, - preserve_attrs: false, filter: indexset! {RowFilter::None, RowFilter::Sub, RowFilter::Entropy, RowFilter::Bigrams}, interlace: Some(Interlacing::None), optimize_alpha: false, @@ -416,7 +427,13 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<( let opt_metadata_preserved; let in_data = match *input { InFile::Path(ref input_path) => { - if opts.preserve_attrs { + if matches!( + output, + OutFile::Path { + preserve_attrs: true, + .. + } + ) { opt_metadata_preserved = input_path .metadata() .map_err(|err| { @@ -458,8 +475,8 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<( if is_fully_optimized(in_data.len(), optimized_output.len(), opts) { match (output, input) { // if p is None, it also means same as the input path - (OutFile::Path(ref p), InFile::Path(ref input_path)) - if p.as_ref().map_or(true, |p| p == input_path) => + (OutFile::Path { path, .. }, InFile::Path(ref input_path)) + if path.as_ref().map_or(true, |p| p == input_path) => { info!("{}: Could not optimize further, no change written", input); return Ok(()); @@ -484,20 +501,18 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<( ) }; - if opts.pretend { - info!("{}: Running in pretend mode, no output", savings); - return Ok(()); - } - match (output, input) { - (&OutFile::StdOut, _) | (&OutFile::Path(None), &InFile::StdIn) => { + (OutFile::None, _) => { + info!("{}: Running in pretend mode, no output", savings); + } + (&OutFile::StdOut, _) | (&OutFile::Path { path: None, .. }, &InFile::StdIn) => { let mut buffer = BufWriter::new(stdout()); buffer .write_all(&optimized_output) .map_err(|e| PngError::new(&format!("Unable to write to stdout: {}", e)))?; } - (OutFile::Path(ref output_path), _) => { - let output_path = output_path + (OutFile::Path { path, .. }, _) => { + let output_path = path .as_ref() .map(|p| p.as_path()) .unwrap_or_else(|| input.path().unwrap()); diff --git a/src/main.rs b/src/main.rs index 12abcbbb..79568f01 100644 --- a/src/main.rs +++ b/src/main.rs @@ -376,12 +376,16 @@ fn collect_files( } continue; }; - let out_file = if let Some(ref out_dir) = *out_dir { - let out_path = Some(out_dir.join(input.file_name().unwrap())); - OutFile::Path(out_path) - } else { - (*out_file).clone() - }; + let out_file = + if let (Some(out_dir), &OutFile::Path { preserve_attrs, .. }) = (out_dir, out_file) { + let path = Some(out_dir.join(input.file_name().unwrap())); + OutFile::Path { + path, + preserve_attrs, + } + } else { + (*out_file).clone() + }; let in_file = if using_stdin { InFile::StdIn } else { @@ -459,10 +463,15 @@ fn parse_opts_into_struct( None }; - let out_file = if matches.get_flag("stdout") { + let out_file = if matches.get_flag("pretend") { + OutFile::None + } else if matches.get_flag("stdout") { OutFile::StdOut } else { - OutFile::Path(matches.get_one::("output_file").cloned()) + OutFile::Path { + path: matches.get_one::("output_file").cloned(), + preserve_attrs: matches.get_flag("preserve"), + } }; opts.optimize_alpha = matches.get_flag("alpha"); @@ -482,10 +491,6 @@ fn parse_opts_into_struct( opts.check = matches.get_flag("check"); - opts.pretend = matches.get_flag("pretend"); - - opts.preserve_attrs = matches.get_flag("preserve"); - opts.bit_depth_reduction = !matches.get_flag("no-bit-reduction"); opts.color_type_reduction = !matches.get_flag("no-color-reduction"); diff --git a/tests/filters.rs b/tests/filters.rs index 17e79b81..5e2f0931 100644 --- a/tests/filters.rs +++ b/tests/filters.rs @@ -20,10 +20,7 @@ fn get_opts(input: &Path) -> (OutFile, oxipng::Options) { filter.insert(RowFilter::None); options.filter = filter; - ( - OutFile::Path(Some(input.with_extension("out.png"))), - options, - ) + (OutFile::from_path(input.with_extension("out.png")), options) } fn test_it_converts( diff --git a/tests/flags.rs b/tests/flags.rs index 8b256953..c244df17 100644 --- a/tests/flags.rs +++ b/tests/flags.rs @@ -26,10 +26,7 @@ fn get_opts(input: &Path) -> (OutFile, oxipng::Options) { filter.insert(RowFilter::None); options.filter = filter; - ( - OutFile::Path(Some(input.with_extension("out.png"))), - options, - ) + (OutFile::from_path(input.with_extension("out.png")), options) } /// Add callback to allow checks before the output file is deleted again @@ -518,8 +515,10 @@ fn preserve_attrs() { #[cfg(feature = "filetime")] let mtime_canon = RefCell::new(filetime::FileTime::from_unix_time(0, 0)); - let (output, mut opts) = get_opts(&input); - opts.preserve_attrs = true; + let (mut output, opts) = get_opts(&input); + if let OutFile::Path { preserve_attrs, .. } = &mut output { + *preserve_attrs = true; + } #[cfg(feature = "filetime")] let callback_pre = |path_in: &Path| { diff --git a/tests/interlaced.rs b/tests/interlaced.rs index efcea5f1..8d7cd6ea 100644 --- a/tests/interlaced.rs +++ b/tests/interlaced.rs @@ -22,10 +22,7 @@ fn get_opts(input: &Path) -> (OutFile, oxipng::Options) { filter.insert(RowFilter::None); options.filter = filter; - ( - OutFile::Path(Some(input.with_extension("out.png"))), - options, - ) + (OutFile::from_path(input.with_extension("out.png")), options) } fn test_it_converts( diff --git a/tests/interlacing.rs b/tests/interlacing.rs index 2e971979..541a0963 100644 --- a/tests/interlacing.rs +++ b/tests/interlacing.rs @@ -17,10 +17,7 @@ fn get_opts(input: &Path) -> (OutFile, oxipng::Options) { filter.insert(RowFilter::None); options.filter = filter; - ( - OutFile::Path(Some(input.with_extension("out.png"))), - options, - ) + (OutFile::from_path(input.with_extension("out.png")), options) } fn test_it_converts( diff --git a/tests/lib.rs b/tests/lib.rs index 3c27f8be..91068f4b 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -37,7 +37,7 @@ fn optimize_from_memory_apng() { fn optimize() { let result = oxipng::optimize( &"tests/files/fully_optimized.png".into(), - &OutFile::Path(None), + &OutFile::None, &Options::default(), ); assert!(result.is_ok()); @@ -47,7 +47,7 @@ fn optimize() { fn optimize_corrupted() { let result = oxipng::optimize( &"tests/files/corrupted_header.png".into(), - &OutFile::Path(None), + &OutFile::None, &Options::default(), ); assert!(result.is_err()); @@ -57,7 +57,7 @@ fn optimize_corrupted() { fn optimize_apng() { let result = oxipng::optimize( &"tests/files/apng_file.png".into(), - &OutFile::Path(None), + &OutFile::None, &Options::from_preset(0), ); assert!(result.is_ok()); diff --git a/tests/reduction.rs b/tests/reduction.rs index 07cbd20b..fca6bfc7 100644 --- a/tests/reduction.rs +++ b/tests/reduction.rs @@ -21,10 +21,7 @@ fn get_opts(input: &Path) -> (OutFile, oxipng::Options) { filter.insert(RowFilter::None); options.filter = filter; - ( - OutFile::Path(Some(input.with_extension("out.png"))), - options, - ) + (OutFile::from_path(input.with_extension("out.png")), options) } fn test_it_converts( diff --git a/tests/regression.rs b/tests/regression.rs index 99a9938b..dbd20c3f 100644 --- a/tests/regression.rs +++ b/tests/regression.rs @@ -20,10 +20,7 @@ fn get_opts(input: &Path) -> (OutFile, oxipng::Options) { filter.insert(RowFilter::None); options.filter = filter; - ( - OutFile::Path(Some(input.with_extension("out.png"))), - options, - ) + (OutFile::from_path(input.with_extension("out.png")), options) } fn test_it_converts( @@ -295,7 +292,7 @@ fn issue_92_filter_5() { let input = "tests/files/issue-92.png"; let (_, mut opts) = get_opts(Path::new(input)); opts.filter = [RowFilter::MinSum].iter().cloned().collect(); - let output = OutFile::Path(Some(Path::new(input).with_extension("-f5-out.png"))); + let output = OutFile::from_path(Path::new(input).with_extension("-f5-out.png")); test_it_converts( input, diff --git a/tests/strategies.rs b/tests/strategies.rs index 8b4b9478..fb8f3cfb 100644 --- a/tests/strategies.rs +++ b/tests/strategies.rs @@ -19,10 +19,7 @@ fn get_opts(input: &Path) -> (OutFile, oxipng::Options) { filter.insert(RowFilter::None); options.filter = filter; - ( - OutFile::Path(Some(input.with_extension("out.png"))), - options, - ) + (OutFile::from_path(input.with_extension("out.png")), options) } fn test_it_converts( From b6a238c67aa626aa7547b68b79a6e4d1b3879aac Mon Sep 17 00:00:00 2001 From: LuckyTurtleDev Date: Mon, 25 Sep 2023 22:28:22 +0200 Subject: [PATCH 2/4] skip non png files, if `--recursive` is used (#548) Fix #547 --- src/main.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/main.rs b/src/main.rs index 79568f01..a83e4464 100644 --- a/src/main.rs +++ b/src/main.rs @@ -25,6 +25,7 @@ use oxipng::RowFilter; use oxipng::StripChunks; use oxipng::{InFile, OutFile}; use rayon::prelude::*; +use std::ffi::OsString; use std::fs::DirBuilder; use std::io::Write; #[cfg(feature = "zopfli")] @@ -64,7 +65,7 @@ fn main() { ) .arg( Arg::new("recursive") - .help("Recurse into subdirectories") + .help("Recurse into subdirectories and optimize all *.png/*.apng files") .short('r') .long("recursive") .action(ArgAction::SetTrue), @@ -353,10 +354,10 @@ fn collect_files( out_dir: &Option, out_file: &OutFile, recursive: bool, - allow_stdin: bool, + top_level: bool, //explicitly specify files ) -> Vec<(InFile, OutFile)> { let mut in_out_pairs = Vec::new(); - let allow_stdin = allow_stdin && files.len() == 1; + let allow_stdin = top_level && files.len() == 1; for input in files { let using_stdin = allow_stdin && input.to_str().map_or(false, |p| p == "-"); if !using_stdin && input.is_dir() { @@ -389,6 +390,14 @@ fn collect_files( let in_file = if using_stdin { InFile::StdIn } else { + // Skip non png files if not given on top level + if !top_level && { + let extension = input.extension().map(|f| f.to_ascii_lowercase()); + extension != Some(OsString::from("png")) + && extension != Some(OsString::from("apng")) + } { + continue; + } InFile::Path(input) }; in_out_pairs.push((in_file, out_file)); From fa47c82617c8840c5d2297d3b86d8731ee9ec82b Mon Sep 17 00:00:00 2001 From: Winterhuman <86165318+Winterhuman@users.noreply.github.com> Date: Mon, 25 Sep 2023 20:43:38 +0000 Subject: [PATCH 3/4] Add `--timeout` notice (#557) Closes: https://github.com/shssoichiro/oxipng/issues/556 Adds a statement saying that `--timeout` isn't as useful for compression algorithms which use fewer and slower rounds, which OxiPNG tends to use nowadays. --------- Co-authored-by: andrews05 --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index a83e4464..db1eabe2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -257,7 +257,7 @@ fn main() { ) .arg( Arg::new("timeout") - .help("Maximum amount of time, in seconds, to spend on optimizations") + .help("Maximum amount of time, in seconds, to spend on optimizations (currently of limited use due to the shift away from zlib)") .value_name("secs") .long("timeout") .value_parser(value_parser!(u64)), From 25d0685bdff3939705766c5ec958db7d134ace78 Mon Sep 17 00:00:00 2001 From: andrews05 Date: Tue, 26 Sep 2023 20:52:51 +1300 Subject: [PATCH 4/4] Remove `backup` and `check` options (#546) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tidy up the API by removing a couple of options we don't really need. Backup was discussed in #542 Check was discussed in #439 @shssoichiro Just say if you prefer to keep either of these 🙂 --- src/lib.rs | 33 +-------------------------------- src/main.rs | 17 ++++++----------- 2 files changed, 7 insertions(+), 43 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index c0c53e4a..2530c310 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -34,7 +34,7 @@ use log::{debug, info, trace, warn}; use rayon::prelude::*; use std::borrow::Cow; use std::fmt; -use std::fs::{copy, File, Metadata}; +use std::fs::{File, Metadata}; use std::io::{stdin, stdout, BufWriter, Read, Write}; use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, Ordering}; @@ -147,18 +147,10 @@ pub type PngResult = Result; #[derive(Clone, Debug)] /// Options controlling the output of the `optimize` function pub struct Options { - /// Whether the input file should be backed up before writing the output. - /// - /// Default: `false` - pub backup: bool, /// Attempt to fix errors when decoding the input file rather than returning an `Err`. /// /// Default: `false` pub fix_errors: bool, - /// Don't actually run any optimizations, just parse the PNG file. - /// - /// Default: `false` - pub check: bool, /// Write to output even if there was no improvement in compression. /// /// Default: `false` @@ -307,8 +299,6 @@ impl Default for Options { fn default() -> Options { // Default settings based on -o 2 from the CLI interface Options { - backup: false, - check: false, fix_errors: false, force: false, filter: indexset! {RowFilter::None, RowFilter::Sub, RowFilter::Entropy, RowFilter::Bigrams}, @@ -462,11 +452,6 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<( let mut png = PngData::from_slice(&in_data, opts)?; - if opts.check { - info!("Running in check mode, not optimizing"); - return Ok(()); - } - // Run the optimizer on the decoded PNG. let mut optimized_output = optimize_png(&mut png, &in_data, opts, deadline)?; @@ -516,9 +501,6 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<( .as_ref() .map(|p| p.as_path()) .unwrap_or_else(|| input.path().unwrap()); - if opts.backup { - perform_backup(output_path)?; - } let out_file = File::create(output_path).map_err(|err| { PngError::new(&format!( "Unable to write to file {}: {}", @@ -998,19 +980,6 @@ fn is_fully_optimized(original_size: usize, optimized_size: usize, opts: &Option original_size <= optimized_size && !opts.force } -fn perform_backup(input_path: &Path) -> PngResult<()> { - let backup_file = input_path.with_extension(format!( - "bak.{}", - input_path.extension().unwrap().to_str().unwrap() - )); - copy(input_path, &backup_file).map(|_| ()).map_err(|_| { - PngError::new(&format!( - "Unable to write to backup file at {}", - backup_file.display() - )) - }) -} - #[cfg(not(unix))] fn copy_permissions(metadata_input: &Metadata, out_file: &File) -> PngResult<()> { let readonly_input = metadata_input.permissions().readonly(); diff --git a/src/main.rs b/src/main.rs index db1eabe2..94c0cfc6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -61,6 +61,7 @@ fn main() { .help("Back up modified files") .short('b') .long("backup") + .hide(true) .action(ArgAction::SetTrue), ) .arg( @@ -103,13 +104,6 @@ fn main() { .long("preserve") .action(ArgAction::SetTrue), ) - .arg( - Arg::new("check") - .help("Do not run any optimization passes") - .short('c') - .long("check") - .action(ArgAction::SetTrue), - ) .arg( Arg::new("pretend") .help("Do not write any files, only calculate compression gains") @@ -299,6 +293,11 @@ Heuristic filter selection strategies: ) .get_matches_from(std::env::args()); + if matches.get_flag("backup") { + eprintln!("The --backup flag is no longer supported. Please use --out or --dir to preserve your existing files."); + exit(1) + } + let (out_file, out_dir, opts) = match parse_opts_into_struct(&matches) { Ok(x) => x, Err(x) => { @@ -492,14 +491,10 @@ fn parse_opts_into_struct( opts.fast_evaluation = matches.get_flag("fast"); } - opts.backup = matches.get_flag("backup"); - opts.force = matches.get_flag("force"); opts.fix_errors = matches.get_flag("fix"); - opts.check = matches.get_flag("check"); - opts.bit_depth_reduction = !matches.get_flag("no-bit-reduction"); opts.color_type_reduction = !matches.get_flag("no-color-reduction");