Skip to content
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

Fix #2518 #2519

Merged
merged 6 commits into from
Sep 2, 2023
Merged

Fix #2518 #2519

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
43c542b
OilPaint benchmark
antonfirsov Aug 25, 2023
6d6833e
fix #2518
antonfirsov Aug 25, 2023
075d05f
Update OilPaintingProcessor{TPixel}.cs
JimBobSquarePants Aug 28, 2023
99771d6
clamp the vector to 0..1 and undo buffer overallocation
antonfirsov Aug 29, 2023
ac0c1f9
throw ImageProcessingException instead of clamping
antonfirsov Sep 1, 2023
918ea1b
Merge branch 'main' into fix-2518
antonfirsov Sep 1, 2023
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
38 changes: 20 additions & 18 deletions src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Buffers;
using System.Numerics;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using SixLabors.ImageSharp.Advanced;
using SixLabors.ImageSharp.Memory;
using SixLabors.ImageSharp.PixelFormats;
Expand Down Expand Up @@ -34,13 +35,14 @@ public OilPaintingProcessor(Configuration configuration, OilPaintingProcessor de
/// <inheritdoc/>
protected override void OnFrameApply(ImageFrame<TPixel> source)
{
int levels = Math.Clamp(this.definition.Levels, 1, 255);
int brushSize = Math.Clamp(this.definition.BrushSize, 1, Math.Min(source.Width, source.Height));

using Buffer2D<TPixel> targetPixels = this.Configuration.MemoryAllocator.Allocate2D<TPixel>(source.Size());

source.CopyTo(targetPixels);

RowIntervalOperation operation = new(this.SourceRectangle, targetPixels, source.PixelBuffer, this.Configuration, brushSize >> 1, this.definition.Levels);
RowIntervalOperation operation = new(this.SourceRectangle, targetPixels, source.PixelBuffer, this.Configuration, brushSize >> 1, levels);
ParallelRowIterator.IterateRowIntervals(
this.Configuration,
this.SourceRectangle,
Expand Down Expand Up @@ -105,18 +107,18 @@ public void Invoke(in RowInterval rows)
Span<Vector4> targetRowVector4Span = targetRowBuffer.Memory.Span;
Span<Vector4> targetRowAreaVector4Span = targetRowVector4Span.Slice(this.bounds.X, this.bounds.Width);

ref float binsRef = ref bins.GetReference();
ref int intensityBinRef = ref Unsafe.As<float, int>(ref binsRef);
ref float redBinRef = ref Unsafe.Add(ref binsRef, (uint)this.levels);
ref float blueBinRef = ref Unsafe.Add(ref redBinRef, (uint)this.levels);
ref float greenBinRef = ref Unsafe.Add(ref blueBinRef, (uint)this.levels);
Span<float> binsSpan = bins.GetSpan();
Span<int> intensityBinsSpan = MemoryMarshal.Cast<float, int>(binsSpan);
Span<float> redBinSpan = binsSpan[this.levels..];
Span<float> blueBinSpan = redBinSpan[this.levels..];
Span<float> greenBinSpan = blueBinSpan[this.levels..];

for (int y = rows.Min; y < rows.Max; y++)
{
Span<TPixel> sourceRowPixelSpan = this.source.DangerousGetRowSpan(y);
Span<TPixel> sourceRowAreaPixelSpan = sourceRowPixelSpan.Slice(this.bounds.X, this.bounds.Width);

PixelOperations<TPixel>.Instance.ToVector4(this.configuration, sourceRowAreaPixelSpan, sourceRowAreaVector4Span);
PixelOperations<TPixel>.Instance.ToVector4(this.configuration, sourceRowAreaPixelSpan, sourceRowAreaVector4Span, PixelConversionModifiers.Scale);

for (int x = this.bounds.X; x < this.bounds.Right; x++)
{
Expand All @@ -140,29 +142,29 @@ public void Invoke(in RowInterval rows)
int offsetX = x + fxr;
offsetX = Numerics.Clamp(offsetX, 0, maxX);

Vector4 vector = sourceOffsetRow[offsetX].ToVector4();
Vector4 vector = Vector4.Clamp(sourceOffsetRow[offsetX].ToScaledVector4(), Vector4.Zero, Vector4.One);
Copy link
Member Author

@antonfirsov antonfirsov Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, ToScaledVector4 doesn't guarantee that the vector will be clamped to 0..1 since a pixel implementation is free to return anything. (In fact RgbaVector doesn't clamp!)

With clamping I was able to undo the buffer overallocation in 99771d6. (Which was in fact ad-hoc since RgbaVector can hold an arbitrary high value, way above 255.)

I think the PR should be good now if you are also fine with the changes @JimBobSquarePants .

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t we fix RgbaVector? ScaledVector is supposed to guarantee 0…1

Copy link
Member

Choose a reason for hiding this comment

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

Just had a look at several of our pixel formats. None of the sanitize the range in construction. We should fix that across the board.

Copy link
Member Author

@antonfirsov antonfirsov Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

None of the sanitize the range in construction

I wonder which are the pixel types where ToScaledVector4() could go out of range? To me it looks like it can happen only with floating point ones (RgbaVector, HalfVector2|4, HalfSingle ...) Do you see any other ones?

Sanitizing in the constructor is insufficient since (1) there are public fields/properties to manipulate the values (2) low-level MemoryMarshal.Cast-like manipulation is completely normal when working with pixel buffers and it can also push values out of range. If we want a guarantee that ToScaledVector4() implementations always return values in 0..1, clipping should happen in ToScaledVector4().

However, even if guarantee this in our own pixel types, arbitrary user pixel types can still potentially violate the contract. If it only leads to an IndexOutOfRangeException, we can blame it on the user. However, if we have unsafe indexing depending on the the values returned from ToScaledVector4() (like we used to do in OilPaint) we have a serious bug in the processor. We may have a similar problem here:

ImageSharp/src/ImageSharp/Processing/Processors/Normalization/AutoLevelProcessor{TPixel}.cs

Lines 151 to 153 in e81efa3

var vector = Unsafe.Add(ref vectorRef, (uint)x);
int luminance = ColorNumerics.GetBT709Luminance(ref vector, levels);
float scaledLuminance = Unsafe.Add(ref cdfBase, (uint)luminance) / noOfPixelsMinusCdfMin;

My recommendation is to revisit these processors before doing anything else.

Copy link
Member

Choose a reason for hiding this comment

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

All of the pixel formats accepting a float lack sanitation so Short2, Short4, and the NormalizedXX variants would be on the list also.

Scaled vector shouldn't clip. It should normalize the scale of the min max values of the pixel format. The mutability of the pixel formats makes sanitation difficult but as you say, avoiding unnecessary unsafe code turns a trivial exception to a serious one so we should definitely review and fix any processors that could be affected.

Regarding this PR. That clamp is going to hurt performance and should be unnecessary given the correct Vector4 range. I'd say we revert it and merge before continuing to sanitize the processors.

Copy link
Member Author

Choose a reason for hiding this comment

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

See ac0c1f9. With aéé the confusion this PR started with, it would make sense to squash on merge.


float sourceRed = vector.X;
float sourceBlue = vector.Z;
float sourceGreen = vector.Y;

int currentIntensity = (int)MathF.Round((sourceBlue + sourceGreen + sourceRed) / 3F * (this.levels - 1));

Unsafe.Add(ref intensityBinRef, (uint)currentIntensity)++;
Unsafe.Add(ref redBinRef, (uint)currentIntensity) += sourceRed;
Unsafe.Add(ref blueBinRef, (uint)currentIntensity) += sourceBlue;
Unsafe.Add(ref greenBinRef, (uint)currentIntensity) += sourceGreen;
intensityBinsSpan[currentIntensity]++;
redBinSpan[currentIntensity] += sourceRed;
blueBinSpan[currentIntensity] += sourceBlue;
greenBinSpan[currentIntensity] += sourceGreen;

if (Unsafe.Add(ref intensityBinRef, (uint)currentIntensity) > maxIntensity)
if (intensityBinsSpan[currentIntensity] > maxIntensity)
{
maxIntensity = Unsafe.Add(ref intensityBinRef, (uint)currentIntensity);
maxIntensity = intensityBinsSpan[currentIntensity];
maxIndex = currentIntensity;
}
}

float red = MathF.Abs(Unsafe.Add(ref redBinRef, (uint)maxIndex) / maxIntensity);
float blue = MathF.Abs(Unsafe.Add(ref blueBinRef, (uint)maxIndex) / maxIntensity);
float green = MathF.Abs(Unsafe.Add(ref greenBinRef, (uint)maxIndex) / maxIntensity);
float red = redBinSpan[maxIndex] / maxIntensity;
float blue = blueBinSpan[maxIndex] / maxIntensity;
float green = greenBinSpan[maxIndex] / maxIntensity;
float alpha = sourceRowVector4Span[x].W;

targetRowVector4Span[x] = new Vector4(red, green, blue, alpha);
Expand All @@ -171,7 +173,7 @@ public void Invoke(in RowInterval rows)

Span<TPixel> targetRowAreaPixelSpan = this.targetPixels.DangerousGetRowSpan(y).Slice(this.bounds.X, this.bounds.Width);

PixelOperations<TPixel>.Instance.FromVector4Destructive(this.configuration, targetRowAreaVector4Span, targetRowAreaPixelSpan);
PixelOperations<TPixel>.Instance.FromVector4Destructive(this.configuration, targetRowAreaVector4Span, targetRowAreaPixelSpan, PixelConversionModifiers.Scale);
}
}
}
Expand Down
19 changes: 19 additions & 0 deletions tests/ImageSharp.Benchmarks/Processing/OilPaint.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using BenchmarkDotNet.Attributes;
using SixLabors.ImageSharp.PixelFormats;
using SixLabors.ImageSharp.Processing;

namespace SixLabors.ImageSharp.Benchmarks.Processing;

[Config(typeof(Config.MultiFramework))]
public class OilPaint
{
[Benchmark]
public void DoOilPaint()
{
using Image<RgbaVector> image = new Image<RgbaVector>(1920, 1200, new(127, 191, 255));
image.Mutate(ctx => ctx.OilPaint());
}
}
7 changes: 7 additions & 0 deletions tests/ImageSharp.Tests/Processing/Processors/Effects/OilPaintTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,11 @@ public void InBox<TPixel>(TestImageProvider<TPixel> provider, int levels, int br
$"{levels}-{brushSize}",
ImageComparer.TolerantPercentage(0.01F));
}

[Fact]
public void Issue2518()
{
using Image<RgbaVector> image = new Image<RgbaVector>(1920, 1200, new(127, 191, 255));
image.Mutate(ctx => ctx.OilPaint());
}
}