Skip to content

Use GetPixelRowSpan to access pixel data in Histogram equalization #1431

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

Merged
merged 4 commits into from
Nov 16, 2020
Merged

Use GetPixelRowSpan to access pixel data in Histogram equalization #1431

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7040ca7
Use GetPixelRowSpan to access pixels, fixes #1429
brianpopow Nov 16, 2020
2021a42
Use GetPixelRowSpan to access pixel data in global hist equalization
brianpopow Nov 16, 2020
6b106da
Use ref TPixel pixel = ref rowSpan[dx];
brianpopow Nov 16, 2020
9bed14f
Add histogram equalization benchmark
brianpopow Nov 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 30 additions & 40 deletions ...arp/Processing/Processors/Normalization/AdaptiveHistogramEqualizationProcessor{TPixel}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,42 +86,39 @@ protected override void OnFrameApply(ImageFrame<TPixel> source)
new Rectangle(0, 0, sourceWidth, tileYStartPositions.Count),
in operation);

ref TPixel pixelsBase = ref source.GetPixelReference(0, 0);

// Fix left column
ProcessBorderColumn(ref pixelsBase, cdfData, 0, sourceWidth, sourceHeight, this.Tiles, tileHeight, xStart: 0, xEnd: halfTileWidth, luminanceLevels);
ProcessBorderColumn(source, cdfData, 0, sourceHeight, this.Tiles, tileHeight, xStart: 0, xEnd: halfTileWidth, luminanceLevels);

// Fix right column
int rightBorderStartX = ((this.Tiles - 1) * tileWidth) + halfTileWidth;
ProcessBorderColumn(ref pixelsBase, cdfData, this.Tiles - 1, sourceWidth, sourceHeight, this.Tiles, tileHeight, xStart: rightBorderStartX, xEnd: sourceWidth, luminanceLevels);
ProcessBorderColumn(source, cdfData, this.Tiles - 1, sourceHeight, this.Tiles, tileHeight, xStart: rightBorderStartX, xEnd: sourceWidth, luminanceLevels);

// Fix top row
ProcessBorderRow(ref pixelsBase, cdfData, 0, sourceWidth, this.Tiles, tileWidth, yStart: 0, yEnd: halfTileHeight, luminanceLevels);
ProcessBorderRow(source, cdfData, 0, sourceWidth, this.Tiles, tileWidth, yStart: 0, yEnd: halfTileHeight, luminanceLevels);

// Fix bottom row
int bottomBorderStartY = ((this.Tiles - 1) * tileHeight) + halfTileHeight;
ProcessBorderRow(ref pixelsBase, cdfData, this.Tiles - 1, sourceWidth, this.Tiles, tileWidth, yStart: bottomBorderStartY, yEnd: sourceHeight, luminanceLevels);
ProcessBorderRow(source, cdfData, this.Tiles - 1, sourceWidth, this.Tiles, tileWidth, yStart: bottomBorderStartY, yEnd: sourceHeight, luminanceLevels);

// Left top corner
ProcessCornerTile(ref pixelsBase, cdfData, sourceWidth, 0, 0, xStart: 0, xEnd: halfTileWidth, yStart: 0, yEnd: halfTileHeight, luminanceLevels);
ProcessCornerTile(source, cdfData, 0, 0, xStart: 0, xEnd: halfTileWidth, yStart: 0, yEnd: halfTileHeight, luminanceLevels);

// Left bottom corner
ProcessCornerTile(ref pixelsBase, cdfData, sourceWidth, 0, this.Tiles - 1, xStart: 0, xEnd: halfTileWidth, yStart: bottomBorderStartY, yEnd: sourceHeight, luminanceLevels);
ProcessCornerTile(source, cdfData, 0, this.Tiles - 1, xStart: 0, xEnd: halfTileWidth, yStart: bottomBorderStartY, yEnd: sourceHeight, luminanceLevels);

// Right top corner
ProcessCornerTile(ref pixelsBase, cdfData, sourceWidth, this.Tiles - 1, 0, xStart: rightBorderStartX, xEnd: sourceWidth, yStart: 0, yEnd: halfTileHeight, luminanceLevels);
ProcessCornerTile(source, cdfData, this.Tiles - 1, 0, xStart: rightBorderStartX, xEnd: sourceWidth, yStart: 0, yEnd: halfTileHeight, luminanceLevels);

// Right bottom corner
ProcessCornerTile(ref pixelsBase, cdfData, sourceWidth, this.Tiles - 1, this.Tiles - 1, xStart: rightBorderStartX, xEnd: sourceWidth, yStart: bottomBorderStartY, yEnd: sourceHeight, luminanceLevels);
ProcessCornerTile(source, cdfData, this.Tiles - 1, this.Tiles - 1, xStart: rightBorderStartX, xEnd: sourceWidth, yStart: bottomBorderStartY, yEnd: sourceHeight, luminanceLevels);
}
}

/// <summary>
/// Processes the part of a corner tile which was previously left out. It consists of 1 / 4 of a tile and does not need interpolation.
/// </summary>
/// <param name="pixelsBase">The output pixels base reference.</param>
/// <param name="source">The source image.</param>
/// <param name="cdfData">The lookup table to remap the grey values.</param>
/// <param name="sourceWidth">The source image width.</param>
/// <param name="cdfX">The x-position in the CDF lookup map.</param>
/// <param name="cdfY">The y-position in the CDF lookup map.</param>
/// <param name="xStart">X start position.</param>
Expand All @@ -133,9 +130,8 @@ protected override void OnFrameApply(ImageFrame<TPixel> source)
/// or 65536 for 16-bit grayscale images.
/// </param>
private static void ProcessCornerTile(
ref TPixel pixelsBase,
ImageFrame<TPixel> source,
CdfTileData cdfData,
int sourceWidth,
int cdfX,
int cdfY,
int xStart,
Expand All @@ -146,23 +142,22 @@ private static void ProcessCornerTile(
{
for (int dy = yStart; dy < yEnd; dy++)
{
int dyOffSet = dy * sourceWidth;
Copy link
Member

@JimBobSquarePants JimBobSquarePants Nov 16, 2020

Choose a reason for hiding this comment

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

It seems to me like int dyOffSet = dy * sourceWidth; (and the same below) were the actual source of the issue and we could have mitigated the issue with rows whilekeeping Unsafe.Add.

I'd like to see a before/after benchmark if possible.

Copy link
Member

@antonfirsov antonfirsov Nov 16, 2020

Choose a reason for hiding this comment

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

while keeping Unsafe.Add

Do we really want it? What I meant in #1429 (comment) is a policy against Unsafe.* regardless of whether it can be justified in a particular use-case.

These processors are the spice in the library, it's not the core functionality where we really need the 1-2 extra percents got by micro-optimization. I'd rather go low-risk here. It's much better to get IndexOutOfRangException than an access violation which is a security-critical bug.

Copy link
Member

@JimBobSquarePants JimBobSquarePants Nov 16, 2020

Choose a reason for hiding this comment

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

I'd like to see benchmarks before I make a decision.

Copy link
Member

@JimBobSquarePants JimBobSquarePants Nov 16, 2020

Choose a reason for hiding this comment

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

I do agree we should be a lot more careful with Unsafe though. I'm guilty of reaching for it far too often.

Copy link
Member

@JimBobSquarePants JimBobSquarePants Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Think I've spotted a way to keep perf for all in a safe manner. https://github.com/SixLabors/ImageSharp/pull/1431/files#r524378700

Copy link
Member

@antonfirsov antonfirsov Nov 16, 2020

Choose a reason for hiding this comment

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

Try a microbenchmark iterating through a span. You shall see no difference or almost no difference if the loop is defined as for (int i = 0; i < span.Length; i++).

Since real life code does much more than that, benefits shall be negligible or nonexistent. I barely see such code in BCL, I believe the only visible benefit is on .NET Framework, or maybe older .NET Core.

If there is no proof for visible benefits, I would strongly prefer to see a guideline point against Unsafe.*.

Copy link
Member

@JimBobSquarePants JimBobSquarePants Nov 16, 2020

Choose a reason for hiding this comment

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

That's why I suggested the slice outside the loop in this instance so we can use .Length. There's little change in Core 3.1 with the current approach but Core 2.1 and NET 472 are suffering according to the benchmarks.

If there is no proof for visible benefits, I would strongly prefer to see a guideline point against Unsafe.*.

Yep. Totally with you. I know of several places in the library where it would be smart to revert to safer code.

Span<TPixel> rowSpan = source.GetPixelRowSpan(dy);
for (int dx = xStart; dx < xEnd; dx++)
{
ref TPixel pixel = ref Unsafe.Add(ref pixelsBase, dyOffSet + dx);
TPixel pixel = rowSpan[dx];
float luminanceEqualized = cdfData.RemapGreyValue(cdfX, cdfY, GetLuminance(pixel, luminanceLevels));
pixel.FromVector4(new Vector4(luminanceEqualized, luminanceEqualized, luminanceEqualized, pixel.ToVector4().W));
rowSpan[dx].FromVector4(new Vector4(luminanceEqualized, luminanceEqualized, luminanceEqualized, pixel.ToVector4().W));
}
}
}

/// <summary>
/// Processes a border column of the image which is half the size of the tile width.
/// </summary>
/// <param name="pixelBase">The output pixels reference.</param>
/// <param name="source">The source image.</param>
/// <param name="cdfData">The pre-computed lookup tables to remap the grey values for each tiles.</param>
/// <param name="cdfX">The X index of the lookup table to use.</param>
/// <param name="sourceWidth">The source image width.</param>
/// <param name="sourceHeight">The source image height.</param>
/// <param name="tileCount">The number of vertical tiles.</param>
/// <param name="tileHeight">The height of a tile.</param>
Expand All @@ -173,10 +168,9 @@ private static void ProcessCornerTile(
/// or 65536 for 16-bit grayscale images.
/// </param>
private static void ProcessBorderColumn(
ref TPixel pixelBase,
ImageFrame<TPixel> source,
CdfTileData cdfData,
int cdfX,
int sourceWidth,
int sourceHeight,
int tileCount,
int tileHeight,
Expand All @@ -194,12 +188,12 @@ private static void ProcessBorderColumn(
int tileY = 0;
for (int dy = y; dy < yLimit; dy++)
{
int dyOffSet = dy * sourceWidth;
Span<TPixel> rowSpan = source.GetPixelRowSpan(dy);
for (int dx = xStart; dx < xEnd; dx++)
{
ref TPixel pixel = ref Unsafe.Add(ref pixelBase, dyOffSet + dx);
TPixel pixel = rowSpan[dx];
float luminanceEqualized = InterpolateBetweenTwoTiles(pixel, cdfData, cdfX, cdfY, cdfX, cdfY + 1, tileY, tileHeight, luminanceLevels);
pixel.FromVector4(new Vector4(luminanceEqualized, luminanceEqualized, luminanceEqualized, pixel.ToVector4().W));
rowSpan[dx].FromVector4(new Vector4(luminanceEqualized, luminanceEqualized, luminanceEqualized, pixel.ToVector4().W));
}

tileY++;
Expand All @@ -213,7 +207,7 @@ private static void ProcessBorderColumn(
/// <summary>
/// Processes a border row of the image which is half of the size of the tile height.
/// </summary>
/// <param name="pixelBase">The output pixels base reference.</param>
/// <param name="source">The source image.</param>
/// <param name="cdfData">The pre-computed lookup tables to remap the grey values for each tiles.</param>
/// <param name="cdfY">The Y index of the lookup table to use.</param>
/// <param name="sourceWidth">The source image width.</param>
Expand All @@ -226,7 +220,7 @@ private static void ProcessBorderColumn(
/// or 65536 for 16-bit grayscale images.
/// </param>
private static void ProcessBorderRow(
ref TPixel pixelBase,
ImageFrame<TPixel> source,
CdfTileData cdfData,
int cdfY,
int sourceWidth,
Expand All @@ -244,14 +238,14 @@ private static void ProcessBorderRow(
{
for (int dy = yStart; dy < yEnd; dy++)
{
int dyOffSet = dy * sourceWidth;
Span<TPixel> rowSpan = source.GetPixelRowSpan(dy);
int tileX = 0;
int xLimit = Math.Min(x + tileWidth, sourceWidth - 1);
for (int dx = x; dx < xLimit; dx++)
Copy link
Member

@JimBobSquarePants JimBobSquarePants Nov 16, 2020

Choose a reason for hiding this comment

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

I think if we sliced the rowSpan and used .Length then we should be able avoid all bounds checks and maintain previous performance in a safe manner on all targets.

{
ref TPixel pixel = ref Unsafe.Add(ref pixelBase, dyOffSet + dx);
TPixel pixel = rowSpan[dx];
float luminanceEqualized = InterpolateBetweenTwoTiles(pixel, cdfData, cdfX, cdfY, cdfX + 1, cdfY, tileX, tileWidth, luminanceLevels);
pixel.FromVector4(new Vector4(luminanceEqualized, luminanceEqualized, luminanceEqualized, pixel.ToVector4().W));
rowSpan[dx].FromVector4(new Vector4(luminanceEqualized, luminanceEqualized, luminanceEqualized, pixel.ToVector4().W));
tileX++;
}
}
Expand Down Expand Up @@ -410,8 +404,6 @@ public RowIntervalOperation(
[MethodImpl(InliningOptions.ShortMethod)]
public void Invoke(in RowInterval rows)
{
ref TPixel sourceBase = ref this.source.GetPixelReference(0, 0);

for (int index = rows.Min; index < rows.Max; index++)
{
(int y, int cdfY) tileYStartPosition = this.tileYStartPositions[index];
Expand All @@ -427,11 +419,11 @@ public void Invoke(in RowInterval rows)
int xEnd = Math.Min(x + this.tileWidth, this.sourceWidth);
for (int dy = y; dy < yEnd; dy++)
{
int dyOffSet = dy * this.sourceWidth;
Span<TPixel> rowSpan = this.source.GetPixelRowSpan(dy);
int tileX = 0;
for (int dx = x; dx < xEnd; dx++)
{
ref TPixel pixel = ref Unsafe.Add(ref sourceBase, dyOffSet + dx);
TPixel pixel = rowSpan[dx];
float luminanceEqualized = InterpolateBetweenFourTiles(
pixel,
this.cdfData,
Expand All @@ -445,7 +437,7 @@ public void Invoke(in RowInterval rows)
this.tileHeight,
this.luminanceLevels);

pixel.FromVector4(new Vector4(luminanceEqualized, luminanceEqualized, luminanceEqualized, pixel.ToVector4().W));
rowSpan[dx].FromVector4(new Vector4(luminanceEqualized, luminanceEqualized, luminanceEqualized, pixel.ToVector4().W));
tileX++;
}

Expand Down Expand Up @@ -597,15 +589,13 @@ public RowIntervalOperation(
[MethodImpl(InliningOptions.ShortMethod)]
public void Invoke(in RowInterval rows)
{
ref TPixel sourceBase = ref this.source.GetPixelReference(0, 0);

for (int index = rows.Min; index < rows.Max; index++)
{
int cdfX = 0;
int cdfY = this.tileYStartPositions[index].cdfY;
int y = this.tileYStartPositions[index].y;
int endY = Math.Min(y + this.tileHeight, this.sourceHeight);
ref int cdfMinBase = ref MemoryMarshal.GetReference(this.cdfMinBuffer2D.GetRowSpan(cdfY));
Span<int> cdfMinSpan = this.cdfMinBuffer2D.GetRowSpan(cdfY);

using IMemoryOwner<int> histogramBuffer = this.allocator.Allocate<int>(this.luminanceLevels);
Span<int> histogram = histogramBuffer.GetSpan();
Expand All @@ -620,10 +610,10 @@ public void Invoke(in RowInterval rows)
int xlimit = Math.Min(x + this.tileWidth, this.sourceWidth);
for (int dy = y; dy < endY; dy++)
{
int dyOffset = dy * this.sourceWidth;
Span<TPixel> rowSpan = this.source.GetPixelRowSpan(dy);
for (int dx = x; dx < xlimit; dx++)
{
int luminance = GetLuminance(Unsafe.Add(ref sourceBase, dyOffset + dx), this.luminanceLevels);
int luminance = GetLuminance(rowSpan[dx], this.luminanceLevels);
histogram[luminance]++;
}
}
Expand All @@ -633,7 +623,7 @@ public void Invoke(in RowInterval rows)
this.processor.ClipHistogram(histogram, this.processor.ClipLimit);
}

Unsafe.Add(ref cdfMinBase, cdfX) = this.processor.CalculateCdf(ref cdfBase, ref histogramBase, histogram.Length - 1);
cdfMinSpan[cdfX] += this.processor.CalculateCdf(ref cdfBase, ref histogramBase, histogram.Length - 1);

cdfX++;
}
Expand Down
10 changes: 5 additions & 5 deletions ...Sharp/Processing/Processors/Normalization/GlobalHistogramEqualizationProcessor{TPixel}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,13 +116,13 @@ public GrayscaleLevelsRowOperation(
public void Invoke(int y)
{
ref int histogramBase = ref MemoryMarshal.GetReference(this.histogramBuffer.GetSpan());
ref TPixel pixelBase = ref MemoryMarshal.GetReference(this.source.GetPixelRowSpan(y));
Span<TPixel> pixelRow = this.source.GetPixelRowSpan(y);
int levels = this.luminanceLevels;

for (int x = 0; x < this.bounds.Width; x++)
{
// TODO: We should bulk convert here.
var vector = Unsafe.Add(ref pixelBase, x).ToVector4();
var vector = pixelRow[x].ToVector4();
int luminance = ImageMaths.GetBT709Luminance(ref vector, levels);
Interlocked.Increment(ref Unsafe.Add(ref histogramBase, luminance));
}
Expand Down Expand Up @@ -165,18 +165,18 @@ public CdfApplicationRowOperation(
public void Invoke(int y)
{
ref int cdfBase = ref MemoryMarshal.GetReference(this.cdfBuffer.GetSpan());
ref TPixel pixelBase = ref MemoryMarshal.GetReference(this.source.GetPixelRowSpan(y));
Span<TPixel> pixelRow = this.source.GetPixelRowSpan(y);
int levels = this.luminanceLevels;
float noOfPixelsMinusCdfMin = this.numberOfPixelsMinusCdfMin;

for (int x = 0; x < this.bounds.Width; x++)
{
// TODO: We should bulk convert here.
ref TPixel pixel = ref Unsafe.Add(ref pixelBase, x);
TPixel pixel = pixelRow[x];
Copy link
Member

@JimBobSquarePants JimBobSquarePants Nov 16, 2020

Choose a reason for hiding this comment

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

You can still use ref here and avoid the double access.

var vector = pixel.ToVector4();
int luminance = ImageMaths.GetBT709Luminance(ref vector, levels);
float luminanceEqualized = Unsafe.Add(ref cdfBase, luminance) / noOfPixelsMinusCdfMin;
pixel.FromVector4(new Vector4(luminanceEqualized, luminanceEqualized, luminanceEqualized, vector.W));
pixelRow[x].FromVector4(new Vector4(luminanceEqualized, luminanceEqualized, luminanceEqualized, vector.W));
}
}
}
Expand Down