Use GetPixelRowSpan to access pixel data in Histogram equalization #1431

brianpopow · 2020-11-16T14:51:27Z

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 matches 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 changes the histogram equalization to use GetPixelRowSpan to access pixel data to fix an Access Violation issue for very large images reported in #1429.

JimBobSquarePants · 2020-11-16T14:55:32Z

...ageSharp/Processing/Processors/Normalization/GlobalHistogramEqualizationProcessor{TPixel}.cs

                {
                    // TODO: We should bulk convert here.
-                    ref TPixel pixel = ref Unsafe.Add(ref pixelBase, x);
+                    TPixel pixel = pixelRow[x];


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

JimBobSquarePants · 2020-11-16T14:59:48Z

...eSharp/Processing/Processors/Normalization/AdaptiveHistogramEqualizationProcessor{TPixel}.cs

        {
            for (int dy = yStart; dy < yEnd; dy++)
            {
-                int dyOffSet = dy * sourceWidth;


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.

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.

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

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

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

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.*.

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.

codecov · 2020-11-16T15:15:28Z

Codecov Report

Merging #1431 (9bed14f) into master (dea0737) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1431      +/-   ##
==========================================
- Coverage   83.08%   83.07%   -0.01%     
==========================================
  Files         707      707              
  Lines       31834    31831       -3     
  Branches     3590     3590              
==========================================
- Hits        26449    26445       -4     
- Misses       4668     4669       +1     
  Partials      717      717

Flag	Coverage Δ
unittests	`83.07% <100.00%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
.../AdaptiveHistogramEqualizationProcessor{TPixel}.cs	`99.55% <100.00%> (-0.01%)`	⬇️
...on/GlobalHistogramEqualizationProcessor{TPixel}.cs	`96.42% <100.00%> (ø)`
src/ImageSharp/ImageFrame{TPixel}.cs	`85.98% <0.00%> (-0.94%)`	⬇️

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 dea0737...9bed14f. Read the comment docs.

brianpopow · 2020-11-16T15:52:01Z

after the changes of this PR:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.630 (2004/?/20H1)
Intel Core i7-6700K CPU 4.00GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.100
  [Host]     : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
  Job-ZJWYUQ : .NET Framework 4.8 (4.8.4250.0), X64 RyuJIT
  Job-PDUVYP : .NET Core 2.1.23 (CoreCLR 4.6.29321.03, CoreFX 4.6.29321.01), X64 RyuJIT
  Job-ZXDATS : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT

IterationCount=3  LaunchCount=1  WarmupCount=3

|                                               Method |        Job |       Runtime |      Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------------------------------------------- |----------- |-------------- |----------:|----------:|----------:|------:|------:|------:|----------:|
|                      'Global Histogram Equalization' | Job-YJFVOZ |    .NET 4.7.2 |  2.992 ms | 0.5856 ms | 0.0321 ms |     - |     - |     - |   5.22 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-YJFVOZ |    .NET 4.7.2 | 38.121 ms | 2.7466 ms | 0.1505 ms |     - |     - |     - |   6.77 KB |
|                      'Global Histogram Equalization' | Job-RDLXEE | .NET Core 2.1 |  2.299 ms | 0.6028 ms | 0.0330 ms |     - |     - |     - |   5.15 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-RDLXEE | .NET Core 2.1 | 18.264 ms | 2.2435 ms | 0.1230 ms |     - |     - |     - |   5.95 KB |
|                      'Global Histogram Equalization' | Job-VSRZSV | .NET Core 3.1 |  2.268 ms | 1.1815 ms | 0.0648 ms |     - |     - |     - |    6.9 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-VSRZSV | .NET Core 3.1 | 15.433 ms | 1.9711 ms | 0.1080 ms |     - |     - |     - |   5.95 KB |

before that:

|                                               Method |        Job |       Runtime |      Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------------------------------------------- |----------- |-------------- |----------:|----------:|----------:|------:|------:|------:|----------:|
|                      'Global Histogram Equalization' | Job-XFMRZJ |    .NET 4.7.2 |  3.061 ms | 1.3141 ms | 0.0720 ms |     - |     - |     - |   5.16 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-XFMRZJ |    .NET 4.7.2 | 48.401 ms | 6.5210 ms | 0.3574 ms |     - |     - |     - |    8.8 KB |
|                      'Global Histogram Equalization' | Job-WTJMNW | .NET Core 2.1 |  2.315 ms | 0.9744 ms | 0.0534 ms |     - |     - |     - |   5.13 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-WTJMNW | .NET Core 2.1 | 37.697 ms | 2.5760 ms | 0.1412 ms |     - |     - |     - |   5.95 KB |
|                      'Global Histogram Equalization' | Job-AVGHFY | .NET Core 3.1 |  2.287 ms | 0.6242 ms | 0.0342 ms |     - |     - |     - |   6.82 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-AVGHFY | .NET Core 3.1 | 16.601 ms | 3.7207 ms | 0.2039 ms |     - |     - |     - |   5.93 KB |

~~netcore 3.1 is almost the same as before, but net471 and core 2.1 are slower now.~~

JimBobSquarePants · 2020-11-16T16:00:54Z

...eSharp/Processing/Processors/Normalization/AdaptiveHistogramEqualizationProcessor{TPixel}.cs

+                    Span<TPixel> rowSpan = source.GetPixelRowSpan(dy);
                    int tileX = 0;
                    int xLimit = Math.Min(x + tileWidth, sourceWidth - 1);
                    for (int dx = x; dx < xLimit; dx++)


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.

antonfirsov · 2020-11-16T16:11:23Z

@brianpopow I guess before/after are the other way around in #1431 (comment)?

Any explanation for the significant difference for Framework VS Core? I would be surprised if it's only about indexers.

brianpopow · 2020-11-16T16:24:22Z

@brianpopow I guess before/after are the other way around in #1431 (comment)?

Any explanation for the significant difference for Framework VS Core? I would be surprised if it's only about indexers.

Give me a moment i will do the benchmark again. I really think i made a mistake.

brianpopow · 2020-11-16T17:26:15Z

@brianpopow I guess before/after are the other way around in #1431 (comment)?

Any explanation for the significant difference for Framework VS Core? I would be surprised if it's only about indexers.

@antonfirsov @JimBobSquarePants: I have run the benchmark now many times, but with the same results as above. The current master branch is actually slower than the change in this PR. I could not believe my eyes, maybe some else wants to double check this.

After the change of this PR:

|                                               Method |        Job |       Runtime |      Mean |    Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------------------------------------------- |----------- |-------------- |----------:|---------:|----------:|------:|------:|------:|----------:|
|                      'Global Histogram Equalization' | Job-KEXZPE |    .NET 4.7.2 |  3.028 ms | 1.898 ms | 0.1040 ms |     - |     - |     - |   5.16 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-KEXZPE |    .NET 4.7.2 | 38.288 ms | 3.331 ms | 0.1826 ms |     - |     - |     - |   7.43 KB |
|                      'Global Histogram Equalization' | Job-GHKMRJ | .NET Core 2.1 |  2.313 ms | 1.010 ms | 0.0553 ms |     - |     - |     - |   5.14 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-GHKMRJ | .NET Core 2.1 | 18.522 ms | 4.412 ms | 0.2419 ms |     - |     - |     - |   5.94 KB |
|                      'Global Histogram Equalization' | Job-DUVFTK | .NET Core 3.1 |  2.350 ms | 2.915 ms | 0.1598 ms |     - |     - |     - |    6.9 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-DUVFTK | .NET Core 3.1 | 15.641 ms | 4.962 ms | 0.2720 ms |     - |     - |     - |   5.91 KB |

Current master branch:

|                                               Method |        Job |       Runtime |      Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------------------------------------------- |----------- |-------------- |----------:|----------:|----------:|------:|------:|------:|----------:|
|                      'Global Histogram Equalization' | Job-WDJRLD |    .NET 4.7.2 |  3.008 ms | 0.5885 ms | 0.0323 ms |     - |     - |     - |   5.19 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-WDJRLD |    .NET 4.7.2 | 49.309 ms | 4.5882 ms | 0.2515 ms |     - |     - |     - |   7.88 KB |
|                      'Global Histogram Equalization' | Job-TYBBMO | .NET Core 2.1 |  2.249 ms | 0.7262 ms | 0.0398 ms |     - |     - |     - |   5.14 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-TYBBMO | .NET Core 2.1 | 37.419 ms | 2.8973 ms | 0.1588 ms |     - |     - |     - |   5.96 KB |
|                      'Global Histogram Equalization' | Job-OCZNHU | .NET Core 3.1 |  2.281 ms | 0.8250 ms | 0.0452 ms |     - |     - |     - |   6.89 KB |
| 'AdaptiveHistogramEqualization (Tile interpolation)' | Job-OCZNHU | .NET Core 3.1 | 16.639 ms | 4.2431 ms | 0.2326 ms |     - |     - |     - |   5.88 KB |

I cannot really tell where the large difference between the framework and core comes from.

JimBobSquarePants · 2020-11-16T17:28:37Z

Haha... I read it the same as Anton. I say we merge this then!

antonfirsov · 2020-11-16T18:04:07Z

Can we add a test based on what we know about the root cause? (Maybe doesn't need the 20k x 10k size.)

brianpopow · 2020-11-16T18:10:24Z

Can we add a test based on what we know about the root cause? (Maybe doesn't need the 20k x 10k size.)

It seems it has to be large to trigger this. 15k x 10k was the "smallest" image where i have seen this. This seems too large for a unit test in my opinion. @antonfirsov what do you think?

antonfirsov

We can add such a test with RemoteExecutor, but it will probably run for seconds, and we probably don't want the additional load on our CI, in that case the PR is fine as is.

Use GetPixelRowSpan to access pixel data in Histogram equalization

brianpopow added 2 commits November 16, 2020 14:56

Use GetPixelRowSpan to access pixels, fixes #1429

7040ca7

Use GetPixelRowSpan to access pixel data in global hist equalization

2021a42

brianpopow added the bug label Nov 16, 2020

brianpopow linked an issue Nov 16, 2020 that may be closed by this pull request

AccessViolationException when applying adaptive histogram to large images #1429

Closed

4 tasks

JimBobSquarePants reviewed Nov 16, 2020

View reviewed changes

Use ref TPixel pixel = ref rowSpan[dx];

6b106da

Add histogram equalization benchmark

9bed14f

JimBobSquarePants reviewed Nov 16, 2020

View reviewed changes

JimBobSquarePants approved these changes Nov 16, 2020

View reviewed changes

antonfirsov approved these changes Nov 16, 2020

View reviewed changes

brianpopow merged commit f77801e into master Nov 16, 2020

brianpopow deleted the bp/fix1429 branch November 16, 2020 18:18

JimBobSquarePants pushed a commit that referenced this pull request Mar 13, 2021

Merge pull request #1431 from SixLabors/bp/fix1429

63d5b70

Use GetPixelRowSpan to access pixel data in Histogram equalization

dependabot bot mentioned this pull request Sep 23, 2025

Bump the nuget group with 5 updates norschel/enterJSWebSecurity2025-Demo1#8

Open

dependabot bot mentioned this pull request Oct 19, 2025

Bump the nuget group with 1 update ewdlop/bepuphysics2Fork#1

Open

Uh oh!

Use GetPixelRowSpan to access pixel data in Histogram equalization #1431

Use GetPixelRowSpan to access pixel data in Histogram equalization #1431

Uh oh!

Conversation

brianpopow commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brianpopow commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Nov 16, 2020

Uh oh!

brianpopow commented Nov 16, 2020

Uh oh!

brianpopow commented Nov 16, 2020

Uh oh!

JimBobSquarePants commented Nov 16, 2020

Uh oh!

antonfirsov commented Nov 16, 2020

Uh oh!

brianpopow commented Nov 16, 2020

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brianpopow commented Nov 16, 2020 •

edited

Loading

JimBobSquarePants Nov 16, 2020 •

edited

Loading

codecov bot commented Nov 16, 2020 •

edited

Loading

brianpopow commented Nov 16, 2020 •

edited

Loading