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

Ensure PixelOperations.To(TPixel) uses scaling. #2050

Merged
merged 2 commits into from
Mar 9, 2022
Merged

Ensure PixelOperations.To(TPixel) uses scaling. #2050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
666ae9e
Ensure PixelOperations.To(TPixel) uses scaling.
JimBobSquarePants Mar 9, 2022
d1cfa8d
Add explicit test
JimBobSquarePants Mar 9, 2022
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
6 changes: 2 additions & 4 deletions src/ImageSharp/ImageFrame{TPixel}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,10 +362,8 @@ internal override void CopyPixelsTo<TDestinationPixel>(MemoryGroup<TDestinationP
return;
}

this.PixelBuffer.FastMemoryGroup.TransformTo(destination, (s, d) =>
{
PixelOperations<TPixel>.Instance.To(this.GetConfiguration(), s, d);
});
this.PixelBuffer.FastMemoryGroup.TransformTo(destination, (s, d)
=> PixelOperations<TPixel>.Instance.To(this.GetConfiguration(), s, d));
}

/// <inheritdoc/>
Expand Down
2 changes: 1 addition & 1 deletion src/ImageSharp/Metadata/Profiles/Exif/Tags/ExifTag.String.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public abstract partial class ExifTag
/// <summary>
/// Gets the MDFileUnits exif tag.
/// </summary>
public static ExifTag<string> MDFileUnits => new ExifTag<string>(ExifTagValue.MDFileUnits);
public static ExifTag<string> MDFileUnits { get; } = new ExifTag<string>(ExifTagValue.MDFileUnits);
Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed this by accident but it was something I found the other day when looking at the EXIF tags.


/// <summary>
/// Gets the SEMInfo exif tag.
Expand Down
4 changes: 2 additions & 2 deletions ...mageSharp/PixelFormats/PixelImplementations/PixelOperations/RgbaVector.PixelOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public partial struct RgbaVector
internal class PixelOperations : PixelOperations<RgbaVector>
{
private static readonly Lazy<PixelTypeInfo> LazyInfo =
new Lazy<PixelTypeInfo>(() => PixelTypeInfo.Create<RgbaVector>(PixelAlphaRepresentation.Unassociated), true);
new(() => PixelTypeInfo.Create<RgbaVector>(PixelAlphaRepresentation.Unassociated), true);
Copy link
Member

Choose a reason for hiding this comment

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

I have to say I am not a fan of this syntax for object creation... I find it a lot harder to read. especially in cases like this:

// in some piece of unchanged code
var list = new List<ObjType>();
.
.
.
// lots of lines later 
.
.
.
// in the middle of a PR change block, there is no context available while review what the hell this thing is.
list.Add(new(arg1, "arg2", 34)
{
    OptionalProperty = bar
});

not going to block any PRs etc over this just wanted to comment on the fact I feel it drastically harms readability in most cases... the only exception I can kind of agree on is property initialization where var is not allowed. Or possibly collection initializers where the Type of clear in the collection type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, definitely something to be aware of. I really like it in some scenarios though


/// <inheritdoc />
public override PixelTypeInfo GetPixelTypeInfo() => LazyInfo.Value;
Expand All @@ -34,7 +34,7 @@ public override void From<TSourcePixel>(
{
Span<Vector4> destinationVectors = MemoryMarshal.Cast<RgbaVector, Vector4>(destinationPixels);

PixelOperations<TSourcePixel>.Instance.ToVector4(configuration, sourcePixels, destinationVectors);
PixelOperations<TSourcePixel>.Instance.ToVector4(configuration, sourcePixels, destinationVectors, PixelConversionModifiers.Scale);
}

/// <inheritdoc />
Expand Down
24 changes: 12 additions & 12 deletions src/ImageSharp/PixelFormats/PixelOperations{TPixel}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ namespace SixLabors.ImageSharp.PixelFormats
public partial class PixelOperations<TPixel>
where TPixel : unmanaged, IPixel<TPixel>
{
private static readonly Lazy<PixelTypeInfo> LazyInfo = new Lazy<PixelTypeInfo>(() => PixelTypeInfo.Create<TPixel>(), true);
private static readonly Lazy<PixelTypeInfo> LazyInfo = new(() => PixelTypeInfo.Create<TPixel>(), true);

/// <summary>
/// Gets the global <see cref="PixelOperations{TPixel}"/> instance for the pixel type <typeparamref name="TPixel"/>
Expand Down Expand Up @@ -116,29 +116,29 @@ public virtual void From<TSourcePixel>(
Span<TPixel> destinationPixels)
where TSourcePixel : unmanaged, IPixel<TSourcePixel>
{
const int SliceLength = 1024;
int numberOfSlices = sourcePixels.Length / SliceLength;
const int sliceLength = 1024;
int numberOfSlices = sourcePixels.Length / sliceLength;

using IMemoryOwner<Vector4> tempVectors = configuration.MemoryAllocator.Allocate<Vector4>(SliceLength);
using IMemoryOwner<Vector4> tempVectors = configuration.MemoryAllocator.Allocate<Vector4>(sliceLength);
Span<Vector4> vectorSpan = tempVectors.GetSpan();
for (int i = 0; i < numberOfSlices; i++)
{
int start = i * SliceLength;
ReadOnlySpan<TSourcePixel> s = sourcePixels.Slice(start, SliceLength);
Span<TPixel> d = destinationPixels.Slice(start, SliceLength);
PixelOperations<TSourcePixel>.Instance.ToVector4(configuration, s, vectorSpan);
this.FromVector4Destructive(configuration, vectorSpan, d);
int start = i * sliceLength;
ReadOnlySpan<TSourcePixel> s = sourcePixels.Slice(start, sliceLength);
Span<TPixel> d = destinationPixels.Slice(start, sliceLength);
PixelOperations<TSourcePixel>.Instance.ToVector4(configuration, s, vectorSpan, PixelConversionModifiers.Scale);
this.FromVector4Destructive(configuration, vectorSpan, d, PixelConversionModifiers.Scale);
}

int endOfCompleteSlices = numberOfSlices * SliceLength;
int endOfCompleteSlices = numberOfSlices * sliceLength;
int remainder = sourcePixels.Length - endOfCompleteSlices;
if (remainder > 0)
{
ReadOnlySpan<TSourcePixel> s = sourcePixels.Slice(endOfCompleteSlices);
Span<TPixel> d = destinationPixels.Slice(endOfCompleteSlices);
vectorSpan = vectorSpan.Slice(0, remainder);
PixelOperations<TSourcePixel>.Instance.ToVector4(configuration, s, vectorSpan);
this.FromVector4Destructive(configuration, vectorSpan, d);
PixelOperations<TSourcePixel>.Instance.ToVector4(configuration, s, vectorSpan, PixelConversionModifiers.Scale);
this.FromVector4Destructive(configuration, vectorSpan, d, PixelConversionModifiers.Scale);
}
}

Expand Down
5 changes: 4 additions & 1 deletion tests/ImageSharp.Tests.ruleset
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
<?xml version="1.0" encoding="utf-8"?>
<RuleSet Name="ImageSharp.Tests" ToolsVersion="16.0">
<RuleSet Name="ImageSharp.Tests" ToolsVersion="17.0">
<Include Path="..\shared-infrastructure\sixlabors.tests.ruleset" Action="Default" />
<Rules AnalyzerId="StyleCop.Analyzers" RuleNamespace="StyleCop.Analyzers">
<Rule Id="SA1313" Action="None" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Allows us to replace "dummy" with discard "_"

</Rules>
<Rules AnalyzerId="xunit.analyzers" RuleNamespace="xunit.analyzers">
<Rule Id="xUnit1004" Action="None" />
<Rule Id="xUnit1013" Action="None" />
Expand Down
61 changes: 35 additions & 26 deletions tests/ImageSharp.Tests/PixelFormats/PixelOperations/PixelOperationsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,30 +320,51 @@ public void ToVector4(int count)
(s, d) => this.Operations.ToVector4(this.Configuration, s, d.GetSpan()));
}

public static readonly TheoryData<object> Generic_To_Data = new TheoryData<object>
public static readonly TheoryData<object> Generic_To_Data = new()
{
new TestPixel<A8>(),
new TestPixel<Abgr32>(),
new TestPixel<Rgba32>(),
new TestPixel<Argb32>(),
new TestPixel<Bgr24>(),
new TestPixel<Bgr565>(),
new TestPixel<Bgra32>(),
new TestPixel<Rgb24>(),
new TestPixel<L8>(),
new TestPixel<Bgra4444>(),
new TestPixel<Bgra5551>(),
new TestPixel<Byte4>(),
new TestPixel<HalfSingle>(),
new TestPixel<HalfVector2>(),
new TestPixel<HalfVector4>(),
new TestPixel<L16>(),
new TestPixel<L8>(),
new TestPixel<La16>(),
new TestPixel<La32>(),
new TestPixel<NormalizedByte2>(),
new TestPixel<NormalizedByte4>(),
new TestPixel<NormalizedShort2>(),
new TestPixel<NormalizedShort4>(),
new TestPixel<Rg32>(),
new TestPixel<Rgb24>(),
new TestPixel<Rgb48>(),
new TestPixel<Rgba64>()
new TestPixel<Rgba1010102>(),
new TestPixel<Rgba32>(),
new TestPixel<Rgba64>(),
new TestPixel<RgbaVector>(),
new TestPixel<Short2>(),
new TestPixel<Short4>(),
};

[Theory]
[MemberData(nameof(Generic_To_Data))]
public void Generic_To<TDestPixel>(TestPixel<TDestPixel> dummy)
public void Generic_To<TDestPixel>(TestPixel<TDestPixel> _)
where TDestPixel : unmanaged, IPixel<TDestPixel>
{
const int Count = 2134;
TPixel[] source = CreatePixelTestData(Count);
var expected = new TDestPixel[Count];
const int count = 2134;
TPixel[] source = CreatePixelTestData(count);
var expected = new TDestPixel[count];

PixelConverterTests.ReferenceImplementations.To<TPixel, TDestPixel>(this.Configuration, source, expected);

TestOperation(source, expected, (s, d) => this.Operations.To(this.Configuration, s, d.GetSpan()));
TestOperation(source, expected, (s, d) => this.Operations.To(this.Configuration, s, d.GetSpan()), false);
Copy link
Member Author

Choose a reason for hiding this comment

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

We need approximate comparison here now we've added all the types. Comparison is still very accurate.

}

[Theory]
Expand Down Expand Up @@ -1234,23 +1255,11 @@ public void Verify()
}

// TODO: We really need a PixelTypeInfo.BitsPerComponent property!!
private static bool IsComplexPixel()
private static bool IsComplexPixel() => default(TDest) switch
{
switch (default(TDest))
{
case HalfSingle _:
case HalfVector2 _:
case L16 _:
case La32 _:
case NormalizedShort2 _:
case Rg32 _:
case Short2 _:
return true;

default:
return Unsafe.SizeOf<TDest>() > sizeof(int);
}
}
HalfSingle or HalfVector2 or L16 or La32 or NormalizedShort2 or Rg32 or Short2 => true,
_ => Unsafe.SizeOf<TDest>() > sizeof(int),
};
}
}
}
16 changes: 16 additions & 0 deletions tests/ImageSharp.Tests/PixelFormats/RgbaVectorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,5 +189,21 @@ public void RgbaVector_FromGrey8()
// assert
Assert.Equal(expected, rgba.ToScaledVector4());
}

[Fact]
public void Issue2048()
{
// https://github.com/SixLabors/ImageSharp/issues/2048
RgbaVector green = Color.Green.ToPixel<RgbaVector>();
Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually covered in the PixelOperation tests but I like to be explicit for serious bugs.

using Image<RgbaVector> source = new(Configuration.Default, 1, 1, green);
using Image<HalfVector4> clone = source.CloneAs<HalfVector4>();

Rgba32 srcColor = default;
Rgba32 cloneColor = default;
source[0, 0].ToRgba32(ref srcColor);
clone[0, 0].ToRgba32(ref cloneColor);

Assert.Equal(srcColor, cloneColor);
}
}
}
18 changes: 9 additions & 9 deletions tests/ImageSharp.Tests/TestUtilities/TestPixel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0.

using System;
using System.Numerics;
using SixLabors.ImageSharp.PixelFormats;
using Xunit.Abstractions;

Expand All @@ -16,6 +17,11 @@ public TestPixel()

public TestPixel(float red, float green, float blue, float alpha)
{
Guard.MustBeBetweenOrEqualTo(red, 0F, 1F, nameof(red));
Copy link
Member Author

Choose a reason for hiding this comment

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

This was unbounded when it shouldn't be.

Guard.MustBeBetweenOrEqualTo(green, 0F, 1F, nameof(green));
Guard.MustBeBetweenOrEqualTo(blue, 0F, 1F, nameof(blue));
Guard.MustBeBetweenOrEqualTo(alpha, 0F, 1F, nameof(alpha));

this.Red = red;
this.Green = green;
this.Blue = blue;
Expand All @@ -33,14 +39,11 @@ public TestPixel(float red, float green, float blue, float alpha)
public TPixel AsPixel()
{
var pix = default(TPixel);
pix.FromVector4(new System.Numerics.Vector4(this.Red, this.Green, this.Blue, this.Alpha));
pix.FromScaledVector4(new Vector4(this.Red, this.Green, this.Blue, this.Alpha));
Copy link
Member Author

Choose a reason for hiding this comment

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

Best to be explicit despite guard.

return pix;
}

internal Span<TPixel> AsSpan()
{
return new Span<TPixel>(new[] { this.AsPixel() });
}
internal Span<TPixel> AsSpan() => new(new[] { this.AsPixel() });

public void Deserialize(IXunitSerializationInfo info)
{
Expand All @@ -58,9 +61,6 @@ public void Serialize(IXunitSerializationInfo info)
info.AddValue("alpha", this.Alpha);
}

public override string ToString()
{
return $"{typeof(TPixel).Name}{this.AsPixel().ToString()}";
}
public override string ToString() => $"{typeof(TPixel).Name}{this.AsPixel()}";
}
}