From 44818a854bdf401b49a4a8309b9706eaaef63709 Mon Sep 17 00:00:00 2001 From: Josh Holmer Date: Wed, 22 Feb 2017 15:45:46 -0500 Subject: [PATCH] Fix two bugs in reducing palette and release version 0.14.2 --- CHANGELOG.md | 4 ++ Cargo.lock | 2 +- Cargo.toml | 4 +- src/png.rs | 82 ++++++++++++++++++++++++--------------- tests/files/issue-56.png | Bin 0 -> 503 bytes tests/filters.rs | 4 +- tests/flags.rs | 40 +++++++++---------- tests/interlaced.rs | 4 +- tests/reduction.rs | 16 ++++---- tests/regression.rs | 51 ++++++++++++------------ 10 files changed, 116 insertions(+), 91 deletions(-) create mode 100644 tests/files/issue-56.png diff --git a/CHANGELOG.md b/CHANGELOG.md index b4abd33a..3f4e028c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +**Version 0.14.2** + - Fix a bug when reducing palette in images with bit depth less than 8 + - Fix a bug when reducing palette in images with transparency + **Version 0.14.1** - Remove zlib dependency and switch entirely to miniz, since zlib 1.2.11 was not working with oxipng. This costs some performance, but is better than having a broken application. diff --git a/Cargo.lock b/Cargo.lock index 79bc82f8..0ce0b147 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,6 +1,6 @@ [root] name = "oxipng" -version = "0.14.1" +version = "0.14.2" dependencies = [ "bit-vec 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)", "byteorder 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/Cargo.toml b/Cargo.toml index e1ee8f7b..846d4255 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,7 +8,7 @@ homepage = "https://github.com/shssoichiro/oxipng" license = "MIT" name = "oxipng" repository = "https://github.com/shssoichiro/oxipng" -version = "0.14.1" +version = "0.14.2" [lib] name = "oxipng" @@ -55,4 +55,4 @@ nightly-binary = ["binary", "nightly", "regex/simd-accel"] dev = ["nightly-binary", "clippy"] [profile.dev] -opt-level = 2 \ No newline at end of file +opt-level = 2 diff --git a/src/png.rs b/src/png.rs index a7d69e8d..39dd57c6 100644 --- a/src/png.rs +++ b/src/png.rs @@ -465,9 +465,29 @@ impl PngData { return false; } - // A palette with RGB slices - let palette = self.palette.clone().unwrap(); - let mut indexed_palette: Vec<&[u8]> = palette.chunks(3).collect(); + // A palette with RGB or RGBA slices + let palette = if let Some(ref trns) = self.transparency_palette { + self.palette + .clone() + .unwrap() + .chunks(3) + .zip(trns.iter()) + .flat_map(|(pixel, trns)| { + let mut pixel = pixel.to_owned(); + pixel.push(*trns); + pixel + }) + .collect() + } else { + self.palette.clone().unwrap() + }; + let mut indexed_palette: Vec<&[u8]> = + palette.chunks(if self.transparency_palette.is_some() { + 4 + } else { + 3 + }) + .collect(); // A map of old indexes to new ones, for any moved let mut index_map: HashMap = HashMap::new(); @@ -589,49 +609,49 @@ impl PngData { } BitDepth::Four => { for byte in &line.data { - let upper = *byte >> 4; + let upper = *byte & 0b11110000; let lower = *byte & 0b00001111; let mut new_byte = 0u8; - if let Some(new_idx) = index_map.get(&upper) { - new_byte &= *new_idx << 4; + new_byte |= if let Some(new_idx) = index_map.get(&upper) { + *new_idx << 4 } else { - new_byte &= upper << 4; - } - if let Some(new_idx) = index_map.get(&lower) { - new_byte &= *new_idx; + upper << 4 + }; + new_byte |= if let Some(new_idx) = index_map.get(&lower) { + *new_idx } else { - new_byte &= lower; - } + lower + }; new_data.push(new_byte); } } BitDepth::Two => { for byte in &line.data { - let one = *byte >> 6; - let two = (*byte >> 4) & 0b00000011; - let three = (*byte >> 2) & 0b00000011; + let one = *byte & 0b11000000; + let two = *byte & 0b00110000; + let three = *byte & 0b00001100; let four = *byte & 0b00000011; let mut new_byte = 0u8; - if let Some(new_idx) = index_map.get(&one) { - new_byte &= *new_idx << 6; + new_byte |= if let Some(new_idx) = index_map.get(&one) { + *new_idx << 6 } else { - new_byte &= one << 6; - } - if let Some(new_idx) = index_map.get(&two) { - new_byte &= *new_idx << 4; + one << 6 + }; + new_byte |= if let Some(new_idx) = index_map.get(&two) { + *new_idx << 4 } else { - new_byte &= two << 4; - } - if let Some(new_idx) = index_map.get(&three) { - new_byte &= *new_idx << 2; + two << 4 + }; + new_byte |= if let Some(new_idx) = index_map.get(&three) { + *new_idx << 2 } else { - new_byte &= three << 2; - } - if let Some(new_idx) = index_map.get(&four) { - new_byte &= *new_idx; + three << 2 + }; + new_byte |= if let Some(new_idx) = index_map.get(&four) { + *new_idx } else { - new_byte &= four; - } + four + }; new_data.push(new_byte); } } diff --git a/tests/files/issue-56.png b/tests/files/issue-56.png new file mode 100644 index 0000000000000000000000000000000000000000..052d0cf11325ebebbec7ce63d488dfa0c5459702 GIT binary patch literal 503 zcmeAS@N?(olHy`uVBq!ia0y~yV0-|?87#~|ky=SpVIUGI4(;BxXv+pNz=zonU)voTJYYXh{RA?+K@%+>m>*sZEbCqK Z!De1=H*d-5I6F}EdAj>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } diff --git a/tests/flags.rs b/tests/flags.rs index 2ebac551..1ce3ad3f 100644 --- a/tests/flags.rs +++ b/tests/flags.rs @@ -58,8 +58,8 @@ fn test_it_converts(input: &Path, let new_png = image::open(output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -115,8 +115,8 @@ fn strip_headers_list() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -156,8 +156,8 @@ fn strip_headers_safe() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -197,8 +197,8 @@ fn strip_headers_all() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -238,8 +238,8 @@ fn strip_headers_none() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -275,8 +275,8 @@ fn interlacing_0_to_1() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -312,8 +312,8 @@ fn interlacing_1_to_0() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -353,8 +353,8 @@ fn interlacing_0_to_1_small_files() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -394,8 +394,8 @@ fn interlacing_1_to_0_small_files() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -434,8 +434,8 @@ fn interlaced_0_to_1_other_filter_mode() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } diff --git a/tests/interlaced.rs b/tests/interlaced.rs index ed8677ba..bd3b8d0d 100644 --- a/tests/interlaced.rs +++ b/tests/interlaced.rs @@ -57,8 +57,8 @@ fn test_it_converts(input: &Path, let new_png = image::open(output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } diff --git a/tests/reduction.rs b/tests/reduction.rs index 992c1e19..079a61c4 100644 --- a/tests/reduction.rs +++ b/tests/reduction.rs @@ -57,8 +57,8 @@ fn test_it_converts(input: &Path, let new_png = image::open(output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -1268,8 +1268,8 @@ fn palette_should_be_reduced_with_dupes() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -1308,8 +1308,8 @@ fn palette_should_be_reduced_with_unused() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -1348,8 +1348,8 @@ fn palette_should_be_reduced_with_both() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } diff --git a/tests/regression.rs b/tests/regression.rs index 9b03b296..998bd418 100644 --- a/tests/regression.rs +++ b/tests/regression.rs @@ -56,8 +56,8 @@ fn test_it_converts(input: &Path, let new_png = image::open(output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -65,9 +65,7 @@ fn test_it_converts(input: &Path, #[test] fn issue_29() { let input = PathBuf::from("tests/files/issue-29.png"); - let mut opts = get_opts(&input); - opts.filter = HashSet::new(); - opts.filter.insert(0); + let opts = get_opts(&input); let output = opts.out_file.clone(); test_it_converts(&input, @@ -114,8 +112,8 @@ fn issue_42() { let new_png = image::open(&output).unwrap(); // Conversion should be lossless - assert_eq!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>(), - new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); + assert!(old_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>() == + new_png.pixels().map(|x| x.2.channels().to_owned()).collect::>>()); remove_file(output).ok(); } @@ -123,9 +121,7 @@ fn issue_42() { #[test] fn issue_52_01() { let input = PathBuf::from("tests/files/issue-52-01.png"); - let mut opts = get_opts(&input); - opts.filter = HashSet::new(); - opts.filter.insert(0); + let opts = get_opts(&input); let output = opts.out_file.clone(); test_it_converts(&input, @@ -140,9 +136,7 @@ fn issue_52_01() { #[test] fn issue_52_02() { let input = PathBuf::from("tests/files/issue-52-02.png"); - let mut opts = get_opts(&input); - opts.filter = HashSet::new(); - opts.filter.insert(0); + let opts = get_opts(&input); let output = opts.out_file.clone(); test_it_converts(&input, @@ -157,9 +151,7 @@ fn issue_52_02() { #[test] fn issue_52_03() { let input = PathBuf::from("tests/files/issue-52-03.png"); - let mut opts = get_opts(&input); - opts.filter = HashSet::new(); - opts.filter.insert(0); + let opts = get_opts(&input); let output = opts.out_file.clone(); test_it_converts(&input, @@ -174,9 +166,7 @@ fn issue_52_03() { #[test] fn issue_52_04() { let input = PathBuf::from("tests/files/issue-52-04.png"); - let mut opts = get_opts(&input); - opts.filter = HashSet::new(); - opts.filter.insert(0); + let opts = get_opts(&input); let output = opts.out_file.clone(); test_it_converts(&input, @@ -191,9 +181,7 @@ fn issue_52_04() { #[test] fn issue_52_05() { let input = PathBuf::from("tests/files/issue-52-05.png"); - let mut opts = get_opts(&input); - opts.filter = HashSet::new(); - opts.filter.insert(0); + let opts = get_opts(&input); let output = opts.out_file.clone(); test_it_converts(&input, @@ -208,9 +196,7 @@ fn issue_52_05() { #[test] fn issue_52_06() { let input = PathBuf::from("tests/files/issue-52-06.png"); - let mut opts = get_opts(&input); - opts.filter = HashSet::new(); - opts.filter.insert(0); + let opts = get_opts(&input); let output = opts.out_file.clone(); test_it_converts(&input, @@ -221,3 +207,18 @@ fn issue_52_06() { ColorType::Indexed, BitDepth::Two); } + +#[test] +fn issue_56() { + let input = PathBuf::from("tests/files/issue-56.png"); + let opts = get_opts(&input); + let output = opts.out_file.clone(); + + test_it_converts(&input, + &output, + &opts, + ColorType::Indexed, + BitDepth::Four, + ColorType::Indexed, + BitDepth::Four); +}