Clamp abyss policy for filter3x3#2874
Conversation
197g
left a comment
There was a problem hiding this comment.
In direct comparison to #3055 , I like clamp-abyss much better as an explainable policy for this operation. That also fits with internalizing complexity instead of a weirder interface. There's imageproc for all the customization you may want. Dynamically sized kernel are instead reweighted with outside pixels not counting to the total but I think that'd be least intuitive behavior for the most common use of a 3x3 kernel in particular.
| let x0 = x as isize + a; | ||
| let y0 = y as isize + b; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
isizeis suspicious here after the fix. The only use is with typeu32inget_pixeland we have ful control over the type oftapswhich is a local. It seems rather wasteful to supposeisizefor 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
CI is super flaky for some reason. Third time a job failed due to a network error today. |
This adds the clamp/replicate abyss policy to
filter3x3. This is consistent withblurandfast_blur.Why is this necessary?
Previously,
filter3x3would leave a 1-pixel border around the filtered image. This made the function a lot less useful. This PR fixes this defect by applying the filter to all pixels of the image.As the TODO says, this function is currently slower than it has to be. This can be addressed in future PRs. I focused on making it correct for now.
Closes: #3026