From 42aa9ac09481cb04d9e4b8c200e5672248a97c5b Mon Sep 17 00:00:00 2001 From: Rostyslav Bohomaz Date: Wed, 1 Feb 2023 18:05:41 +0200 Subject: [PATCH 1/5] methods for getting pixel using signed coordinates - add `GenericImageView::checked_get_pixel` method with `i64` arguments returning `Option` - add `GenericImageView::saturating_get_pixel` method with `i64` arguments returning nearest pixel in case location is not in bounds --- src/image.rs | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 108 insertions(+), 2 deletions(-) diff --git a/src/image.rs b/src/image.rs index 94d91dec81..84eda0d6b4 100644 --- a/src/image.rs +++ b/src/image.rs @@ -1,5 +1,5 @@ #![allow(clippy::too_many_arguments)] -use std::convert::TryFrom; +use std::convert::{TryFrom, TryInto}; use std::ffi::OsStr; use std::io; use std::io::Read; @@ -7,6 +7,8 @@ use std::ops::{Deref, DerefMut}; use std::path::Path; use std::usize; +use exr::image; + use crate::color::{ColorType, ExtendedColorType}; use crate::error::{ ImageError, ImageFormatHint, ImageResult, LimitError, LimitErrorKind, ParameterError, @@ -921,6 +923,65 @@ pub trait GenericImageView { self.get_pixel(x, y) } + /// Returns the pixel located at (x, y) if the location is [`in_bounds`], + /// otherwise `None` is returned. + /// + /// Location coordinates can be negative. + /// + /// # Safety + /// + /// The [`in_bounds`] check must work as expected. + /// + /// [`in_bounds`]: #method.in_bounds + fn checked_get_pixel(&self, x: i64, y: i64) -> Option { + if x.is_negative() || y.is_negative() { + None + } else { + match (u32::try_from(x), u32::try_from(y)) { + (Ok(x), Ok(y)) => { + if self.in_bounds(x, y) { + Some(unsafe { self.unsafe_get_pixel(x, y) }) + } else { + None + } + }, + _ => None + } + } + } + + /// Returns the pixel located at (x, y) if the location is [`in_bounds`], + /// otherwise the nearest bound pixel is returned. + /// + /// Location coordinates can be negative. + /// + /// [`in_bounds`]: #method.in_bounds + fn saturating_get_pixel(&self, x: i64, y: i64) -> Self::Pixel { + #[inline(always)] + fn convert(value: i64, bound: u32) -> u32 { + value.is_positive() + .then(|| { + match u32::try_from(value) { + Ok(value) => { + if value < bound { + value + } else { + bound - 1 + } + }, + Err(_) => bound, + } + }).unwrap_or(0u32) + } + + unsafe { + self.unsafe_get_pixel( + convert(x, self.width()), + convert(y, self.height()) + ) + } + } + /// Returns an Iterator over the pixels of this image. /// The iterator yields the coordinates of each pixel /// along with their value @@ -1327,7 +1388,7 @@ mod tests { }; use crate::color::Rgba; use crate::math::Rect; - use crate::{GrayImage, ImageBuffer}; + use crate::{GrayImage, ImageBuffer, Luma}; #[test] #[allow(deprecated)] @@ -1447,6 +1508,51 @@ mod tests { source.view(2, 2, 0, 0); } + #[test] + fn test_checked_get_pixel() { + let source = ImageBuffer::from_pixel(3, 3, Rgba([255u8, 0, 0, 255])); + + assert!(source.checked_get_pixel(0, 0).is_some()); + assert!(source.checked_get_pixel(2, 2).is_some()); + assert!(source.checked_get_pixel(0, 2).is_some()); + assert!(source.checked_get_pixel(2, 0).is_some()); + + assert!(source.checked_get_pixel(-1, -1).is_none()); + assert!(source.checked_get_pixel(3, 3).is_none()); + assert!(source.checked_get_pixel(0, 3).is_none()); + assert!(source.checked_get_pixel(3, 0).is_none()); + assert!(source.checked_get_pixel(-1, 0).is_none()); + assert!(source.checked_get_pixel(0, -1).is_none()); + } + + #[test] + fn test_saturating_get_pixel() { + let source: ImageBuffer, Vec> = ImageBuffer::from_vec(3, 3, vec![ + 0u8, 1u8, 2u8, + 3u8, 4u8, 5u8, + 6u8, 7u8, 8u8, + ]).unwrap(); + + for x in 0..source.width() { + for y in 0..source.height() { + assert_eq!(source.saturating_get_pixel(x as i64, y as i64).to_owned(), *source.get_pixel(x, y)); + } + } + + assert_eq!(source.saturating_get_pixel(-1, -1).to_owned(), *source.get_pixel(0, 0)); + assert_eq!(source.saturating_get_pixel(-1, 0).to_owned(), *source.get_pixel(0, 0)); + assert_eq!(source.saturating_get_pixel(0, -1).to_owned(), *source.get_pixel(0, 0)); + assert_eq!(source.saturating_get_pixel(3, 3).to_owned(), *source.get_pixel(2, 2)); + assert_eq!(source.saturating_get_pixel(3, 2).to_owned(), *source.get_pixel(2, 2)); + assert_eq!(source.saturating_get_pixel(2, 3).to_owned(), *source.get_pixel(2, 2)); + assert_eq!(source.saturating_get_pixel(3, -1).to_owned(), *source.get_pixel(2, 0)); + assert_eq!(source.saturating_get_pixel(2, -1).to_owned(), *source.get_pixel(2, 0)); + assert_eq!(source.saturating_get_pixel(3, 0).to_owned(), *source.get_pixel(2, 0)); + assert_eq!(source.saturating_get_pixel(-1, 2).to_owned(), *source.get_pixel(0, 2)); + assert_eq!(source.saturating_get_pixel(-1, 3).to_owned(), *source.get_pixel(0, 2)); + assert_eq!(source.saturating_get_pixel(0, 3).to_owned(), *source.get_pixel(0, 2)); + } + #[test] fn test_copy_sub_image() { let source = ImageBuffer::from_pixel(3, 3, Rgba([255u8, 0, 0, 255])); From 61d4bbe3d99fb4718d0c5208ca97a128a3b57403 Mon Sep 17 00:00:00 2001 From: Rostyslav Bohomaz Date: Wed, 1 Feb 2023 18:17:02 +0200 Subject: [PATCH 2/5] clean up imports after previous commit :\ --- src/image.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/image.rs b/src/image.rs index 84eda0d6b4..6cd63aa8ca 100644 --- a/src/image.rs +++ b/src/image.rs @@ -1,5 +1,5 @@ #![allow(clippy::too_many_arguments)] -use std::convert::{TryFrom, TryInto}; +use std::convert::TryFrom; use std::ffi::OsStr; use std::io; use std::io::Read; @@ -7,8 +7,6 @@ use std::ops::{Deref, DerefMut}; use std::path::Path; use std::usize; -use exr::image; - use crate::color::{ColorType, ExtendedColorType}; use crate::error::{ ImageError, ImageFormatHint, ImageResult, LimitError, LimitErrorKind, ParameterError, @@ -1388,7 +1386,7 @@ mod tests { }; use crate::color::Rgba; use crate::math::Rect; - use crate::{GrayImage, ImageBuffer, Luma}; + use crate::{GrayImage, ImageBuffer}; #[test] #[allow(deprecated)] @@ -1527,7 +1525,7 @@ mod tests { #[test] fn test_saturating_get_pixel() { - let source: ImageBuffer, Vec> = ImageBuffer::from_vec(3, 3, vec![ + let source = GrayImage::from_vec(3, 3, vec![ 0u8, 1u8, 2u8, 3u8, 4u8, 5u8, 6u8, 7u8, 8u8, From 0b34934921c16d6e3b9e2063f65fe87e6e5fd130 Mon Sep 17 00:00:00 2001 From: Rostyslav Bohomaz Date: Wed, 8 Feb 2023 12:56:05 +0200 Subject: [PATCH 3/5] use min max approach for saturating get pixel --- src/image.rs | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/src/image.rs b/src/image.rs index 6cd63aa8ca..16aa10e3ac 100644 --- a/src/image.rs +++ b/src/image.rs @@ -955,29 +955,15 @@ pub trait GenericImageView { /// /// [`in_bounds`]: #method.in_bounds fn saturating_get_pixel(&self, x: i64, y: i64) -> Self::Pixel { - #[inline(always)] - fn convert(value: i64, bound: u32) -> u32 { - value.is_positive() - .then(|| { - match u32::try_from(value) { - Ok(value) => { - if value < bound { - value - } else { - bound - 1 - } - }, - Err(_) => bound, - } - }).unwrap_or(0u32) - } + let (ix, iy, iw, ih) = self.bounds(); - unsafe { - self.unsafe_get_pixel( - convert(x, self.width()), - convert(y, self.height()) - ) - } + let x0 = ix as i64; + let x1 = (ix + iw - 1) as i64; + + let y0 = iy as i64; + let y1 = (iy + ih - 1) as i64; + + unsafe { self.unsafe_get_pixel(x.max(x0).min(x1) as u32, y.max(y0).min(y1) as u32) } } /// Returns an Iterator over the pixels of this image. From 15c1cd29cc933be567722229ff11ab8a90d5871a Mon Sep 17 00:00:00 2001 From: Rostyslav Bohomaz Date: Wed, 8 Feb 2023 12:56:59 +0200 Subject: [PATCH 4/5] rm redundant negative check for checked get pixel --- src/image.rs | 104 ++++++++++++++++++++++++++++++++++----------------- 1 file changed, 69 insertions(+), 35 deletions(-) diff --git a/src/image.rs b/src/image.rs index 16aa10e3ac..5e2e0e9f41 100644 --- a/src/image.rs +++ b/src/image.rs @@ -923,36 +923,28 @@ pub trait GenericImageView { /// Returns the pixel located at (x, y) if the location is [`in_bounds`], /// otherwise `None` is returned. - /// + /// /// Location coordinates can be negative. /// /// # Safety /// /// The [`in_bounds`] check must work as expected. - /// + /// /// [`in_bounds`]: #method.in_bounds fn checked_get_pixel(&self, x: i64, y: i64) -> Option { - if x.is_negative() || y.is_negative() { - None - } else { - match (u32::try_from(x), u32::try_from(y)) { - (Ok(x), Ok(y)) => { - if self.in_bounds(x, y) { - Some(unsafe { self.unsafe_get_pixel(x, y) }) - } else { - None - } - }, - _ => None - } + match (u32::try_from(x), u32::try_from(y)) { + (Ok(x), Ok(y)) => self + .in_bounds(x, y) + .then(|| unsafe { self.unsafe_get_pixel(x, y) }), + _ => None, } } /// Returns the pixel located at (x, y) if the location is [`in_bounds`], /// otherwise the nearest bound pixel is returned. - /// + /// /// Location coordinates can be negative. - /// + /// /// [`in_bounds`]: #method.in_bounds fn saturating_get_pixel(&self, x: i64, y: i64) -> Self::Pixel { let (ix, iy, iw, ih) = self.bounds(); @@ -1511,30 +1503,72 @@ mod tests { #[test] fn test_saturating_get_pixel() { - let source = GrayImage::from_vec(3, 3, vec![ - 0u8, 1u8, 2u8, - 3u8, 4u8, 5u8, - 6u8, 7u8, 8u8, - ]).unwrap(); + #[rustfmt::skip] + let source = { + GrayImage::from_vec(3, 3, vec![ + 0u8, 1u8, 2u8, + 3u8, 4u8, 5u8, + 6u8, 7u8, 8u8 + ]).unwrap() + }; for x in 0..source.width() { for y in 0..source.height() { - assert_eq!(source.saturating_get_pixel(x as i64, y as i64).to_owned(), *source.get_pixel(x, y)); + assert_eq!( + source.saturating_get_pixel(x as i64, y as i64).to_owned(), + *source.get_pixel(x, y) + ); } } - assert_eq!(source.saturating_get_pixel(-1, -1).to_owned(), *source.get_pixel(0, 0)); - assert_eq!(source.saturating_get_pixel(-1, 0).to_owned(), *source.get_pixel(0, 0)); - assert_eq!(source.saturating_get_pixel(0, -1).to_owned(), *source.get_pixel(0, 0)); - assert_eq!(source.saturating_get_pixel(3, 3).to_owned(), *source.get_pixel(2, 2)); - assert_eq!(source.saturating_get_pixel(3, 2).to_owned(), *source.get_pixel(2, 2)); - assert_eq!(source.saturating_get_pixel(2, 3).to_owned(), *source.get_pixel(2, 2)); - assert_eq!(source.saturating_get_pixel(3, -1).to_owned(), *source.get_pixel(2, 0)); - assert_eq!(source.saturating_get_pixel(2, -1).to_owned(), *source.get_pixel(2, 0)); - assert_eq!(source.saturating_get_pixel(3, 0).to_owned(), *source.get_pixel(2, 0)); - assert_eq!(source.saturating_get_pixel(-1, 2).to_owned(), *source.get_pixel(0, 2)); - assert_eq!(source.saturating_get_pixel(-1, 3).to_owned(), *source.get_pixel(0, 2)); - assert_eq!(source.saturating_get_pixel(0, 3).to_owned(), *source.get_pixel(0, 2)); + assert_eq!( + source.saturating_get_pixel(-1, -1).to_owned(), + *source.get_pixel(0, 0) + ); + assert_eq!( + source.saturating_get_pixel(-1, 0).to_owned(), + *source.get_pixel(0, 0) + ); + assert_eq!( + source.saturating_get_pixel(0, -1).to_owned(), + *source.get_pixel(0, 0) + ); + assert_eq!( + source.saturating_get_pixel(3, 3).to_owned(), + *source.get_pixel(2, 2) + ); + assert_eq!( + source.saturating_get_pixel(3, 2).to_owned(), + *source.get_pixel(2, 2) + ); + assert_eq!( + source.saturating_get_pixel(2, 3).to_owned(), + *source.get_pixel(2, 2) + ); + assert_eq!( + source.saturating_get_pixel(3, -1).to_owned(), + *source.get_pixel(2, 0) + ); + assert_eq!( + source.saturating_get_pixel(2, -1).to_owned(), + *source.get_pixel(2, 0) + ); + assert_eq!( + source.saturating_get_pixel(3, 0).to_owned(), + *source.get_pixel(2, 0) + ); + assert_eq!( + source.saturating_get_pixel(-1, 2).to_owned(), + *source.get_pixel(0, 2) + ); + assert_eq!( + source.saturating_get_pixel(-1, 3).to_owned(), + *source.get_pixel(0, 2) + ); + assert_eq!( + source.saturating_get_pixel(0, 3).to_owned(), + *source.get_pixel(0, 2) + ); } #[test] From be2013015ce06d22d5eec488f725e60648d54ef4 Mon Sep 17 00:00:00 2001 From: Rostyslav Bohomaz Date: Wed, 10 May 2023 16:19:42 +0300 Subject: [PATCH 5/5] fix flat bounds method Make it consistent with `ImageBuffer` and `SubImageInner` --- src/flat.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/flat.rs b/src/flat.rs index 4011cb5f73..f6753d2d4e 100644 --- a/src/flat.rs +++ b/src/flat.rs @@ -1389,7 +1389,7 @@ where fn bounds(&self) -> (u32, u32, u32, u32) { let (w, h) = self.dimensions(); - (0, w, 0, h) + (0, 0, w, h) } fn in_bounds(&self, x: u32, y: u32) -> bool {