Skip to content

Conversation

@ynse01
Copy link
Contributor

@ynse01 ynse01 commented Sep 17, 2022

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

Implementation of #87

This PR can be interpreted as attempt 2 of the closed PR #1619. It tries to address the comments mentioned there. This processor ensures that the image remains RGB and no color distortion occurs.

Some design decisions (comments welcome):

  • Not touching the Alpha channel.
  • Using BT.709 luminance, as this is being used in other Normalization processors already. Original PR talks about using LAB.
  • Include the sync channels option, as it was originally in ImageMagick.

Please comment if this is the correct functionality.

I'll address the bulk conversion To-do's in a later PR, as there are more (unrelated) classes that suffer from that issue.

@JimBobSquarePants
Copy link
Member

@ynse01 Thanks for having a crack at this. The code looks great 👍

Do we have output references from ImageMagick we can compare our output with?

@ynse01
Copy link
Contributor Author

ynse01 commented Sep 22, 2022

@ynse01 Thanks for having a crack at this. The code looks great +1

Do we have output references from ImageMagick we can compare our output with?

Good point. Let me try and make an explicit Test for it.

@ynse01
Copy link
Contributor Author

ynse01 commented Sep 24, 2022

The HistogramEqualizationTests seem to be unstable in CI on MacOS. I don't own a Mac, so I can't have a look in any detail.
Please advise.

@JimBobSquarePants
Copy link
Member

I’ll have a look. It’s likely not us. ImagMagick seems to return different results on Mac

@JimBobSquarePants
Copy link
Member

I pulled down the artifacts from the build at the bottom of this page here and compared to a local test run on my Windows machine. (I need to dig my Mac out of a box as I moved out recently)

https://github.com/SixLabors/ImageSharp/actions/runs/3117901063

Interestingly it appears that there is an issue but not with the Mac output. Look at the pixel comparison there.
My output and the reference both suffer from what looks like overflow issues when calculating individual component values.

Mac CI - Left, Windows Local Right

image

Reference Output - Left, Windows Local Right

image

@ynse01
Copy link
Contributor Author

ynse01 commented Sep 25, 2022

Thanks for the directions @JimBobSquarePants.
I found an off-by-one error in the code for the separate channel option and updated that reference image accordingly.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

This looks great to me. Thanks!

Looking forward to a follow up PR to handle the bulk pixel bits.

I need to have a look at the other Histogram equalization methods tbh. As I recall they only work for grayscale images.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants