Skip to content
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

Jump to bottom

Fix AdaptiveThresholdProcessor throws IndexOutOfRangeException #2221

Merged
merged 7 commits into from
Sep 2, 2022
Merged

Fix AdaptiveThresholdProcessor throws IndexOutOfRangeException #2221

merged 7 commits into from
Sep 2, 2022

Conversation

brianpopow
Copy link
Collaborator

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This PR fixes issue #2217.

The PR is still marked as WIP, because I am not sure the output is correct. It's all black. I assume that's not how it should look like? I am not familiar with the algorithm.

Input:
187073251-5d23775b-f4b9-459b-a686-e2556401a1f7

Output:
output

gfoidl
gfoidl reviewed Sep 1, 2022
View reviewed changes
src/ImageSharp/Processing/Processors/Binarization/AdaptiveThresholdProcessor{TPixel}.cs Outdated
var x2 = Math.Min(x - this.startX + this.clusterSize + 1, this.bounds.Width - 1);
var y1 = Math.Max(y - this.startY - this.clusterSize + 1, 0);
var y2 = Math.Min(y - this.startY + this.clusterSize + 1, this.bounds.Height - 1);
int x1 = Math.Min(Math.Max(x - this.startX - this.clusterSize + 1, 0), maxX);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could use Math.Clamp?

@JimBobSquarePants
Copy link
Member

I am not familiar with the algorithm.

Unfortunately me neither. It was added here. I'll read the attached paper.

One thing I have noticed having a quick look at the code is that it does not use our standardized Integral Image extension method added here. I'll pull it down and have a fiddle.

@JimBobSquarePants
Copy link
Member

Looks like I have this working.

AdaptiveThreshold_WithRectangle_Works_Rgba32_Bradley02
AdaptiveThreshold_Works_Rgba32_Bradley01
AdaptiveThreshold_Works_Rgba32_Bradley02
AdaptiveThreshold_Works_Rgba32_ducky
Issue_2217_AdaptiveThreshold_DoesNotThrowIndexOutOfRangeException_Rgba32_Issue_2217_AdaptiveThresholdProcessor

@brianpopow
Copy link
Collaborator Author

Looks like I have this working.

Nice, good job in solving this issue! I think this is now ready to be merged.

@brianpopow brianpopow changed the title WIP: Fix AdaptiveThresholdProcessor throws IndexOutOfRangeException Fix AdaptiveThresholdProcessor throws IndexOutOfRangeException Sep 2, 2022
@JimBobSquarePants JimBobSquarePants merged commit 6e065fa into main Sep 2, 2022
@JimBobSquarePants JimBobSquarePants deleted the bp/Issue2217 branch September 2, 2022 11:03
@JimBobSquarePants JimBobSquarePants added this to the 3.0.0 milestone Sep 2, 2022
@brianpopow brianpopow mentioned this pull request Sep 2, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gfoidl gfoidl gfoidl left review comments

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@brianpopow @JimBobSquarePants @gfoidl