-
-
Notifications
You must be signed in to change notification settings - Fork 854
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.
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
Feature: histogram equalization #644
Feature: histogram equalization #644
Conversation
Sync with imagesharp master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea, need a little refinement though. 👍
I wonder if we can incorporate some of the work from this early fork of ImageSharp to build more normalization/equalization methods?
|
||
// build the histogram of the grayscale levels | ||
int luminanceLevels = is16bitPerChannel ? 65536 : 256; | ||
int[] histogram = new int[luminanceLevels]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big allocation here. Use the MemoryAllocator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im using MemoryAllocator now, please check if im using it correctly.
} | ||
|
||
// calculate the cumulative distribution function (which will be the cumulative histogram) | ||
int[] cdf = new int[luminanceLevels]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allocation
} | ||
} | ||
|
||
int[] lut = new int[luminanceLevels]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed this allocation, its not necessary, using the cdf array instead
{ | ||
TPixel sourcePixel = row[x]; | ||
|
||
int luminance = this.GetLuminance(sourcePixel, is16bitPerChannel, ref rgb24, ref rgb48); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We appear to be getting the luminance twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and i see no good way around this.
First i need to access all pixels to calculate the histogram.
After that i need the luminance of each pixel again to remap it to the new value.
|
||
using SixLabors.ImageSharp.PixelFormats; | ||
|
||
namespace SixLabors.ImageSharp.Processing.Contrast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SixLabors.ImageSharp.Processing.Normalization
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have changed that
…essing.Normalization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few things to note
var rgb24 = default(Rgb24); | ||
MemoryAllocator memoryAllocator = configuration.MemoryAllocator; | ||
int numberOfPixels = source.Width * source.Height; | ||
bool is16bitPerChannel = typeof(TPixel) == typeof(Rgb48) || typeof(TPixel) == typeof(Rgba64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we generally don't have ImageProcesses having any logic dependent on the PixelType
do we really need this? do we even need to know that its a 16bit image? normally we would convert the pixel to a Vector4 and process the buffer with no need for branching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am using now vector4, but still i need to determine how many gray levels there are with the pixel type, because i need that to create the histogram.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the code the number of levels looks like its something that the user should be in control of. We should allow it to be passed in to the constructor of the processor but have a default value of 65536 this will give a good high quality default plus allow users to adjust as they desire to tweaking the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is important for the algorithm to know how many possible grey levels there are. If the input image only has 256 possible grey levels, creating a histogram with 65536 would not change the quality of the outcome.
The algorithm just maps each input grey level to a new output grey level with the goal to use all possible grey values equally. For example the picture i have attached to this PR has only grey values of 120 to 200. After the equalization it uses the the complete range from 0 to 256, but the total number of different grey levels will not change after the equalization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it sounds like my suggestion will allow that and will work fine it will just over allocate on smaller pixel sizes but will allow users with higher quality pixel definitions to fully make use of the processor.
Ultimately we can't have processors that have logic dependent on the pixel type. All processors in the core of imagesharp have to have to work consistently no matter which pixel-type is used this includes custom user defined pixel types that might not even map to any sort of bit depth.... they could be using some high precision floating point value for each channel that doesn't map to any of those grayscale depths you are expecting thus we need to allow the user to influence the mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i understand now your reasoning why you do not want pixel type related code in the processor.
The luminance levels is now a parameter of the constructor as you suggested.
// build the histogram of the grayscale levels | ||
int luminanceLevels = is16bitPerChannel ? 65536 : 256; | ||
Span<int> histogram = memoryAllocator.Allocate<int>(luminanceLevels, clear: true).GetSpan(); | ||
for (int y = 0; y < source.Height; y++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be done with a single loop of
var pixels = source.GetPixelSpan();
for (int i = 0; i < pixels.Length; i++){
int luminance = this.GetLuminance(in pixels[i], is16bitPerChannel, ref rgb24, ref rgb48);
histogram[luminance]++;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah nice, i did not know that
|
||
// build the histogram of the grayscale levels | ||
int luminanceLevels = is16bitPerChannel ? 65536 : 256; | ||
Span<int> histogram = memoryAllocator.Allocate<int>(luminanceLevels, clear: true).GetSpan(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be a called with a using statment
using(IBuffer<int> buffer = memoryAllocator.AllocateClean<int>(luminanceLevels))
{
// wrap remaining code in here
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have changed that
// apply the cdf to each pixel of the image | ||
double numberOfPixelsMinusCdfMin = (double)(numberOfPixels - cdfMin); | ||
int luminanceLevelsMinusOne = luminanceLevels - 1; | ||
for (int y = 0; y < source.Height; y++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above, as this doesn't actually depend on the relative position of the pixel then this can be done is a single loop over all the pixels.
Codecov Report
@@ Coverage Diff @@
## master #644 +/- ##
==========================================
+ Coverage 89.39% 89.41% +0.01%
==========================================
Files 896 899 +3
Lines 38337 38414 +77
Branches 2668 2678 +10
==========================================
+ Hits 34272 34348 +76
- Misses 3264 3265 +1
Partials 801 801
Continue to review full report at Codecov.
|
…ngly into Processors namespace
@JimBobSquarePants would think an adaptive histogram equalization could be a useful feature for imagesharp? I think it can produce much better results then the global histogram equalization in some cases. I have hacked together a first rough version in in ImageSharp. Here is an example result where is works pretty good: (note: click on the image to see it in full size, the preview does not look correct) Let me know, if you think that this is something you would like to have for imagesharp, then i would look further into it. |
@brianpopow Definitely! I'd love to get a common set of histogram based operations put together. |
Prerequisites
Description
This PR adds a HistogramEqualization Processor for enhancing the global contrast of an image.
Example image:
After Equalization: