Skip to content

Clamp abyss policy for filter3x3 #2874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
RunDevelopment wants to merge 5 commits into
base: main
Choose a base branch
from
+31 −12
Open

Clamp abyss policy for filter3x3 #2874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
502883e
Clamp abyss policy for `filter3x3`
RunDevelopment Mar 18, 2026
5ac2263
Merge branch 'main' into clamp-filter3x3
RunDevelopment Jun 10, 2026
448d8d6
Merge branch 'main' into clamp-filter3x3
RunDevelopment Jun 26, 2026
8c6969e
Unsigned tippy taps
RunDevelopment Jun 26, 2026
cc0b53b
Fix index overflow with assert
RunDevelopment Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 27 additions & 12 deletions src/imageops/sample.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,34 +887,47 @@ where

/// Perform a 3x3 box filter on the supplied image.
///
/// # Arguments:
/// # Arguments
///
/// * `image` - source image.
/// * `kernel` - is an array of the filter weights of length 9.
///
/// # Notes
///
/// This method typically assumes that the input is scene-linear light.
/// If it is not, color distortion may occur.
///
/// This operations uses the clamp/replicate abyss policy. I.e. `aaa|abcdef|fff`.
pub fn filter3x3<I, P, S>(image: &I, kernel: &[f32; 9]) -> ImageBuffer<P, Vec<S>>
where
I: GenericImageView<Pixel = P>,
P: Pixel<Subpixel = S>,
S: Primitive,
{
// The kernel's input positions relative to the current pixel.
let taps: &[(isize, isize)] = &[
(-1, -1),
(0, -1),
(1, -1),
(-1, 0),
// Each coordinate -1, 0, 1 is offset by 1 to make them unsigned.
let taps: &[(u32, u32)] = &[
(0, 0),
(1, 0),
(-1, 1),
(2, 0),
(0, 1),
(1, 1),
(2, 1),
(0, 2),
(1, 2),
(2, 2),
];

let (width, height) = image.dimensions();
let mut out = image.buffer_like();
if width == 0 || height == 0 {
return out;
}

assert!(
width < u32::MAX && height < u32::MAX,
"Image dimensions too large"
);

let max = S::DEFAULT_MAX_VALUE;
let max: f32 = NumCast::from(max).unwrap();
Expand All @@ -924,18 +937,20 @@ where
sum => 1.0 / sum,
};

for y in 1..height - 1 {
for x in 1..width - 1 {
for y in 0..height {
for x in 0..width {
let mut t = [0.0; MAX_CHANNEL];

// TODO: There is no need to recalculate the kernel for each pixel.
// Only a subtract and addition is needed for pixels after the first
// in each row.
for (&k, &(a, b)) in kernel.iter().zip(taps.iter()) {
let x0 = x as isize + a;
let y0 = y as isize + b;
Comment on lines -935 to -936

@197g 197g Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding slowness, do you mean it would be better to handle the near-border in a separate loop? If so, do you want to do that in a follow-up PR or did you intend that comment to be a blocker? I'm fine with either.

In an case, the cast to isize is suspicious here after the fix. The only use is with type u32 in get_pixel and we have ful control over the type of taps which is a local. It seems rather wasteful to suppose isize for what is a tri-state bool, no?

@RunDevelopment RunDevelopment Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding slowness,

Definitely follow-up PR. This PR changes behavior, so I can't just add a benchmark and say "it's 2x faster now," since the before and after don't do the same thing.

In an case, the cast to isize is suspicious here after the fix. The only use is with type u32 in get_pixel and we have ful control over the type of taps which is a local. It seems rather wasteful to suppose isize for what is a tri-state bool, no?

It's not any more sus than before IMO. But we could change tabs to go 0,1,2 instead of -1,0,1 and then subtract 1. I.e. (x + a).saturating_sub(1).min(width - 1). This makes the clamping less explicit, but it's probably fine.

@197g 197g Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this exposes an overflow case when x = u32::MAX (previous >= i32::MAX on 32-bit targets). It's good to handle / fail that the same regardless of platform.

@RunDevelopment RunDevelopment Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, x = u32::MAX isn't possible because x < width, but x = u32::MAX - 1 overflows so your point stands.

I think I'll just add an assert width < u32::MAX. Same for height. Not supporting images that large is probably fine.

Alternatively, we could also handle x=0 separately. This would also turn the saturating_sub(1) into a simple - 1 and allow us to write x - 1 + a (underflow and overflow impossible). But I don't want to write that code, because unrolling the nested loops in that way is going to be annoying.

// `x + a` won't overflow since x∈[0,u32::MAX-2] (because x < width < u32::MAX)
// and a∈[0,2]. Similar for `y + b`.
let x0 = (x + a).saturating_sub(1).min(width - 1);
let y0 = (y + b).saturating_sub(1).min(height - 1);

let p = image.get_pixel(x0 as u32, y0 as u32);
let p = image.get_pixel(x0, y0);

for (tc, &c) in t.iter_mut().zip(p.channels()) {
*tc += <f32 as NumCast>::from(c).unwrap() * k;
Expand Down
4 changes: 4 additions & 0 deletions src/images/dynimage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1284,9 +1284,13 @@ impl DynamicImage {
///
/// * `kernel` - array contains filter.
///
/// # Notes
///
/// This method typically assumes that the input is scene-linear light. It operates on pixel
/// channel values directly without taking into account color space data. If it is not, color
/// distortion may occur.
///
/// This operations uses the clamp/replicate abyss policy. I.e. `aaa|abcdef|fff`.
#[must_use]
pub fn filter3x3(&self, kernel: &[f32; 9]) -> DynamicImage {
dynamic_map!(*self, ref p => imageops::filter3x3(p, kernel))
Expand Down
Loading