Skip to content

Conversation

@CoenraadS
Copy link

@CoenraadS CoenraadS commented Apr 30, 2021

Closes #87

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

Add AutoLevel Normalization (only works for L8, L16 types)

Test Image generated from following command (ImageMagick)

cd ImageSharp\tests\Images\Input\Jpg\baseline
magick 640px-Unequalized_Hawkes_Bay_NZ.jpg -auto-level 640px-Unequalized_Hawkes_Bay_NZ.png

@CLAassistant
Copy link

CLAassistant commented Apr 30, 2021

CLA assistant check
All committers have signed the CLA.

Rectangle sourceRectangle)
: base(configuration, source, sourceRectangle)
{
// TODO I don't know how to get channel bit depth for non-grayscale types
Copy link
Author

Choose a reason for hiding this comment

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

I didn't know what to do here

@codecov
Copy link

codecov bot commented May 1, 2021

Codecov Report

Merging #1619 (a237912) into master (9baba86) will increase coverage by 0.01%.
The diff coverage is 93.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1619      +/-   ##
==========================================
+ Coverage   84.30%   84.31%   +0.01%     
==========================================
  Files         816      818       +2     
  Lines       35919    35977      +58     
  Branches     4185     4189       +4     
==========================================
+ Hits        30280    30334      +54     
- Misses       4813     4815       +2     
- Partials      826      828       +2     
Flag Coverage Δ
unittests 84.31% <93.10%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...essors/Normalization/AutoLevelProcessor{TPixel}.cs 92.85% <92.85%> (ø)
...s/Normalization/HistogramEqualizationExtensions.cs 66.66% <100.00%> (+16.66%) ⬆️
...ing/Processors/Normalization/AutoLevelProcessor.cs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9baba86...a237912. Read the comment docs.

@CoenraadS CoenraadS changed the title Add AutoLevel Histogram Processor Add AutoLevel Processor May 2, 2021
: base(configuration, source, sourceRectangle)
{
// TODO I don't know how to get channel bit depth for non-grayscale types
if (!(typeof(TPixel) == typeof(L16) || typeof(TPixel) == typeof(L8)))
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be ok to take the PixelType.BitsPerPixel and divide it by Unsafe.SizeOf<T>(). That will give you a per-pixel-component average (since all our pixel formats are blittable) which should be close enough.

Copy link
Author

@CoenraadS CoenraadS May 3, 2021

Choose a reason for hiding this comment

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

If I would try that with L16

Unsafe.SizeOf<L16>() would return 2
BitsPerPixel would return 16

The result is half of what it needs to be? (L16 is only 1 component?)

Or did you mean it's an operation only for non-luminescence types?

Copy link
Member

Choose a reason for hiding this comment

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

Ah right yep, my bad We need a ChannelCount property on PixelTypeInfo.

@antonfirsov This would be a breaking change (which we can probably allow for 1.1 release) but do you have any ideas how we could introduce it in a non breaking way?

Copy link
Member

Choose a reason for hiding this comment

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

A hack you could do just now is something similar to what we do in the png encoder, I.E use a switch for known types and a default for unknown.

return typeof(TPixel) switch
{
Type t when t == typeof(A8) => PngColorType.GrayscaleWithAlpha,
Type t when t == typeof(Argb32) => PngColorType.RgbWithAlpha,
Type t when t == typeof(Bgr24) => PngColorType.Rgb,
Type t when t == typeof(Bgra32) => PngColorType.RgbWithAlpha,
Type t when t == typeof(L8) => PngColorType.Grayscale,
Type t when t == typeof(L16) => PngColorType.Grayscale,
Type t when t == typeof(La16) => PngColorType.GrayscaleWithAlpha,
Type t when t == typeof(La32) => PngColorType.GrayscaleWithAlpha,
Type t when t == typeof(Rgb24) => PngColorType.Rgb,
Type t when t == typeof(Rgba32) => PngColorType.RgbWithAlpha,
Type t when t == typeof(Rgb48) => PngColorType.Rgb,
Type t when t == typeof(Rgba64) => PngColorType.RgbWithAlpha,
Type t when t == typeof(RgbaVector) => PngColorType.RgbWithAlpha,
_ => PngColorType.RgbWithAlpha

@JimBobSquarePants JimBobSquarePants added this to the 1.1.0 milestone May 8, 2021
@CoenraadS
Copy link
Author

Looking through the ImageMagick implementation, they also support autolevel on the color channels (I thought it would just create a grayscale image but its not), but I must admit I'm not to sure how to implement that, nor do I have a use for it. Would full color support be required for this to be merged?

@brianpopow
Copy link
Collaborator

Looking through the ImageMagick implementation, they also support autolevel on the color channels (I thought it would just create a grayscale image but its not), but I must admit I'm not to sure how to implement that, nor do I have a use for it. Would full color support be required for this to be merged?

@CoenraadS: in my opinion, its ok if its only works on grayscale image. This is also the case for the other HistogramEqualization processors. Maybe add a note in the Processor Summary text, that it only works with gray images, so the user know what to expect.

@JimBobSquarePants
Copy link
Member

What's the science behind working for all color channels? Might it be a case of using the resultant normalized luminance as some sort of multiplier against each pixel? (luminance/bitdepth/bitdepth) * pixel.

I'm not convinced greyscale only is an adequate implementation for the base library (I dearly wish our existing Histogram equalization operations supported full color) simply because every use case for automated equalization I've ever seen is in the application against full color photos in an album.

@brianpopow
Copy link
Collaborator

brianpopow commented May 11, 2021

What's the science behind working for all color channels? Might it be a case of using the resultant normalized luminance as some sort of multiplier against each pixel? (luminance/bitdepth/bitdepth) * pixel.

I'm not convinced greyscale only is an adequate implementation for the base library (I dearly wish our existing Histogram equalization operations supported full color) simply because every use case for automated equalization I've ever seen is in the application against full color photos in an album.

Im not convinced that applying the histogram equalization to each individual color channel of an RGB image makes much sense.
OpenCV for example does not support it for the adaptive histogram equalization either CLAHE.

One approach which sounds reasonable to me is converting the image from the RGB color space to the LAB color space and then apply the histogram equalization to the L channel. Then convert it back to RGB.
I had once a Proof of concept done for this when i was working on CLAHE for ImageSharp, but i was not fully convinced with the results and its like 2 years ago (cant find the source code anymore).

In my opinion applying the equalization to all color channels should not be the scope of this PR. Maybe its worth looking into how ImageMagic does it, but that could be something for a future PR.

edit: I have checked the AutoLevel source code of ImageMagick and they do apply it to each channel separately. So maybe I was wrong in saying it does not make sense to do it this way. (But its stated in the comment of the source code, that applying the histogram stretch individually will cause a color distortion.) I still believe the method i mentioned above would be a better approach.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: autolevel (like ImageMagick)

4 participants