cleanup projects and environment variables #2812

kasperk81 · 2024-09-22T08:48:22Z

cleanup projects and use modern environment variables.

CLAassistant · 2024-09-22T08:48:27Z

All committers have signed the CLA.

kasperk81 · 2024-09-22T14:27:10Z

tests/ImageSharp.Benchmarks/Bulk/Shuffle3Channel.cs

@@ -43,21 +43,21 @@ public void Shuffle3()
 //
 // |               Method |                Job |                              EnvironmentVariables | Count |      Mean |    Error |   StdDev |    Median | Ratio | RatioSD | Gen 0 | Gen 1 | Gen 2 | Allocated |
 // |--------------------- |------------------- |-------------------------------------------------- |------ |----------:|---------:|---------:|----------:|------:|--------:|------:|------:|------:|----------:|
-// |             Shuffle3 | 1. No HwIntrinsics | COMPlus_EnableHWIntrinsic=0,COMPlus_FeatureSIMD=0 |    96 |  48.46 ns | 1.034 ns | 2.438 ns |  47.46 ns |  1.00 |    0.00 |     - |     - |     - |         - |
+// |             Shuffle3 | 1. No HwIntrinsics | DOTNET_EnableHWIntrinsic=0,DOTNET_FeatureSIMD=0 |    96 |  48.46 ns | 1.034 ns | 2.438 ns |  47.46 ns |  1.00 |    0.00 |     - |     - |     - |         - |


.net 6 onwards, DOTNET_ is preferred over COMPlus_ dotnet/runtime@fbdcec9 it first uses DOTNET_, and then COMPlus_ as a legacy fallback.

JimBobSquarePants

Thanks for the updates. I’m in Europe on holiday just now so cannot review the changes properly but will do so when I’m back in Australia (3 days)

tests/ImageSharp.Tests/ImageSharp.Tests.csproj

JimBobSquarePants · 2024-10-01T04:34:45Z

Looks like .NET 9 introduces a bunch of new required fixes. A good thing but also a chore.

kasperk81 · 2024-10-01T11:42:46Z

.net9 work can be a separate pr from this one.

kasperk81 · 2024-10-01T12:44:23Z

reverted, here is the history: https://github.com/SixLabors/ImageSharp/commits/522843cf11e6f965a3ad8f82304a573dd44222ff/. it fixes all CA warnings, passes all CI jobs with .net8, but fails all CI jobs with .net9 due to ONE test failure:

xUnit.net 00:06:14.30]   Finished:    SixLabors.ImageSharp.Tests
  ImageSharp.Tests test net9.0 failed with 1 error(s) and 14 warning(s) (375.2s)
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : 
      [xUnit.net 00:00:00.29] SixLabors.ImageSharp.Tests: BenchmarkDotNet=v0.13.0, OS=ubuntu 20.04 (container)
      AMD EPYC 7763, 1 CPU, 4 logical and 2 physical cores
      .NET SDK=9.0.100-rc.1.24452.12
        [Host] : .NET 9.0.0 (9.0.24.43107), X64 RyuJIT
      
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:00:07.92]     CreateAndResize [SKIP]
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:00:12.99]     Encode_Animated_VisualTest [SKIP]
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:00:18.76]     ResizeBicubic [SKIP]
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:01:09.30]     LargeImage [SKIP]
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:01:13.66]     Benchmark_ToVector4 [SKIP]
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:01:27.97]     LoadAsync_IsCancellable [SKIP]
    /workspaces/ImageSharp/tests/ImageSharp.Tests/TestUtilities/ImageComparison/ImageComparer.cs(125): error TESTERROR: 
      TiffDecoder_CanDecode_TiledWithBadZlib (33ms): Error Message: SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison.ImageDifferenceIsOverThresholdExcepti
      on : Image difference is over threshold
      !Test Environment OS : Linux
      Test Environment is CI : False
      Test Environment is .NET Core : True
      Test Environment is Mono : False
      Test Environment OS Architecture : X64
      Test Environment Process Architecture : X64
      Report ImageFrame 0: 
      Total difference: 0.0018%
      [Δ(0,0,55512,0) @ (10,107)];
      [Δ(3855,65535,0,0) @ (11,107)]
      
      Stack Trace:
         at SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison.ImageComparerExtensions.VerifySimilarity[TPixelA,TPixelB](ImageComparer comparer, Image`1 exp
      ected, Image`1 actual, Func`3 predicate) in /workspaces/ImageSharp/tests/ImageSharp.Tests/TestUtilities/ImageComparison/ImageComparer.cs:line 125
         at SixLabors.ImageSharp.Tests.TestImageExtensions.CompareToReferenceOutput[TPixel](Image`1 image, ImageComparer comparer, ITestImageProvider provider, Ob
      ject testOutputDetails, String extension, Boolean grayscale, Boolean appendPixelTypeToFileName, Boolean appendSourceFileOrDescription, IImageDecoder decoder
      ) in /workspaces/ImageSharp/tests/ImageSharp.Tests/TestUtilities/TestImageExtensions.cs:line 230
         at SixLabors.ImageSharp.Tests.Formats.Tiff.TiffDecoderTests.TiffDecoder_CanDecode_TiledWithBadZlib[TPixel](TestImageProvider`1 provider) in /workspaces/I
      mageSharp/tests/ImageSharp.Tests/Formats/Tiff/TiffDecoderTests.cs:line 721
         at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
         at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:02:39.38]     PrintNonNormalizedKernelMap [SKIP]
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:03:26.48]     LoadResizeSave [SKIP]
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:03:30.27]     Decoder_ParseStream_SaveSpectralResult [SKIP]
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:03:36.50]     BenchmarkMagickBmpDecoder [SKIP]
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:03:36.51]     BenchmarkMagickPngDecoder [SKIP]
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:03:36.51]     BenchmarkSystemDrawingPngDecoder [SKIP]
    /home/codespace/.dotnet9/sdk/9.0.100-rc.1.24452.12/Microsoft.TestPlatform.targets(48,5): warning : [xUnit.net 00:03:36.51]     BenchmarkSystemDrawingBmpDecoder [SKIP]
  ImageSharp.Benchmarks net9.0 succeeded (0.1s) → artifacts/bin/tests/ImageSharp.Benchmarks/Debug/net9.0/ImageSharp.Benchmarks.dll

Test summary: total: 36767, failed: 1, succeeded: 36753, skipped: 13, duration: 375.2s

https://github.com/kasperk81/ImageSharp/actions/runs/11124894544 it doesn't show in github action logs had to run it locally.

@adamsitnik @eerhardt this is either .net9 regression with ReadExactly or a fix but it doesn't repro in .net8 as you can see in this github run https://github.com/kasperk81/ImageSharp/actions/runs/11124894544. unfortunately i don't have a simple repro.

eerhardt · 2024-10-01T13:44:09Z

@kasperk81 - can you log an issue in dotnet/runtime?

JimBobSquarePants · 2024-10-30T09:01:27Z

Hey @kasperk81 Did you ever raise that issue? I'm wondering whether there is a zlib implementation change in .NET 9 which might be responsible for the differences given the test input image has damaged compression.

kasperk81 · 2024-10-30T09:47:09Z

@JimBobSquarePants i didn't get a chance to isolate a repro to report something actionable upstream. it's off by 0.0018% can we adjust the tolerance for VerifySimilarity? i haven't checked how much it differs net8.0, i.e. slightly less than 0.0018% or exactly 0%?

JimBobSquarePants · 2024-10-30T09:51:49Z

I'll pull down your branch and compare the output. It's likely we can simply adjust that tolerance.

JimBobSquarePants · 2024-11-14T11:08:28Z

I was right about the zlib changes. The underlying library .NET uses has changed. It actually decodes an extra 2 pixels in the busted image!

https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-9/libraries#compression-with-zlib-ng

eerhardt · 2024-11-14T16:00:23Z

src/ImageSharp.ruleset

@@ -1,4 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <RuleSet Name="ImageSharp" ToolsVersion="17.0">
  <Include Path="..\shared-infrastructure\sixlabors.ruleset" Action="Default" />
+  <Rules AnalyzerId="Microsoft.CodeAnalysis.CSharp.NetAnalyzers" RuleNamespace="Microsoft.CodeAnalysis.CSharp.NetAnalyzers">
+    <Rule Id="CA2022" Action="Info" />


For my learning, why was this analyzer downgraded from the default Warning to Info? The issues that this analyzer flags are typically bugs when reading from Streams.

The warnings were in areas of our code where it wasn’t really applicable. Our decoders are designed to handle degenerate images and in each warning instance we had pre-allocated an empty buffer to read to. If we don’t get a full read then those buffers remain partially unfillled and the decoding process will return when we look for the next chunk.

Hmm, I'm wondering if we have a missed use case / bug in our analyzer then. Do you happen to have a link to example code that it flagged? Maybe it just couldn't understand the pattern.

I don't think the analyzer will ever be possible to follow the code well enough here.

Take this for example. In ReadFrame in the GIF decoder we attempt to read a local color palette from a stream. The analyzer will warn us that we might not get the full read though.

ImageSharp/src/ImageSharp/Formats/Gif/GifDecoderCore.cs

Line 432 in 2d7cf48

stream.Read(this.currentLocalColorTable.GetSpan()[..length]);

In this instance we simply do not care as the following method ReadFrameColors reads and checks the next byte. If it's invalid then we do nothing.

ImageSharp/src/ImageSharp/Formats/Gif/GifDecoderCore.cs

Lines 533 to 534 in 2d7cf48

int minCodeSize = stream.ReadByte();

if (LzwDecoder.IsValidMinCodeSize(minCodeSize))

We also do a sense check and break when performing the next iteration to find additional frames so the decoder will quit after decoding as much information as possible.

ImageSharp/src/ImageSharp/Formats/Gif/GifDecoderCore.cs

Lines 155 to 156 in 2d7cf48

nextFlag = stream.ReadByte();

if (nextFlag == -1)

So, our code is safe, we could perhaps break earlier but we would end up complicating the code in order to do so. Info will work in our scenarios as it will prompt us to check the code but not inhibit our ability to work.

Taking a quick look, I believe the reason why the code is safe is because BufferedReadStream guarantees it will fill out the entire passed in buffer on each call to .Read.

ImageSharp/src/ImageSharp/IO/BufferedReadStream.cs

Lines 176 to 195 in 2d7cf48

public override int Read(Span<byte> buffer)

{

this.cancellationToken.ThrowIfCancellationRequested();

// Too big for our buffer. Read directly from the stream.

int count = buffer.Length;

if (count > this.BufferSize)

{

return this.ReadToBufferDirectSlow(buffer);

}

// Too big for remaining buffer but less than entire buffer length

// Copy to buffer then read from there.

if ((uint)this.readBufferIndex > (uint)(this.BufferSize - count))

{

return this.ReadToBufferViaCopySlow(buffer);

}

return this.ReadToBufferViaCopyFast(buffer);

}

ReadToBufferDirectSlow and ReadToBufferViaCopySlow read from the underlying stream in a loop until it fills the buffer or hits the end of the stream.

ImageSharp/src/ImageSharp/IO/BufferedReadStream.cs

Lines 347 to 357 in 2d7cf48

// Read doesn't always guarantee the full returned length so read a byte

// at a time until we get either our count or hit the end of the stream.

int count = buffer.Length;

int n = 0;

int i;

do

{

i = baseStream.Read(buffer[n..count]);

n += i;

}

while (n < count && i > 0);

ImageSharp/src/ImageSharp/IO/BufferedReadStream.cs

Lines 320 to 326 in 2d7cf48

private int ReadToBufferViaCopySlow(Span<byte> buffer)

{

// Refill our buffer then copy.

this.FillReadBuffer();

return this.ReadToBufferViaCopyFast(buffer);

}

ImageSharp/src/ImageSharp/IO/BufferedReadStream.cs

Lines 270 to 288 in 2d7cf48

private void FillReadBuffer()

{

this.cancellationToken.ThrowIfCancellationRequested();

Stream baseStream = this.BaseStream;

if (this.readerPosition != baseStream.Position)

{

baseStream.Seek(this.readerPosition, SeekOrigin.Begin);

}

// Read doesn't always guarantee the full returned length so read a byte

// at a time until we get either our count or hit the end of the stream.

int n = 0;

int i;

do

{

i = baseStream.Read(this.readBuffer, n, this.BufferSize - n);

n += i;

}

while (n < this.BufferSize && i > 0);

cleanup projects and environment variables

418ce0e

kasperk81 commented Sep 22, 2024

View reviewed changes

JimBobSquarePants reviewed Sep 23, 2024

View reviewed changes

tests/ImageSharp.Tests/ImageSharp.Tests.csproj Outdated Show resolved Hide resolved

kasperk81 added 2 commits September 23, 2024 18:57

add net9.0

9cc5ff2

net9.0

56b28fc

kasperk81 force-pushed the refactor branch from 522843c to 56b28fc Compare October 1, 2024 12:33

Revert .net9

e6cf1e5

Merge branch 'main' into refactor

5f19369

JimBobSquarePants added 3 commits November 14, 2024 13:26

Merge branch 'main' into refactor

93299e1

Test against NET 9

87d6f00

FIx build

62744c6

JimBobSquarePants merged commit 2d7cf48 into SixLabors:main Nov 14, 2024
8 checks passed

kasperk81 deleted the refactor branch November 14, 2024 14:19

eerhardt reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup projects and environment variables #2812

cleanup projects and environment variables #2812

kasperk81 commented Sep 22, 2024

CLAassistant commented Sep 22, 2024 •

edited

Loading

kasperk81 Sep 22, 2024

JimBobSquarePants left a comment

JimBobSquarePants commented Oct 1, 2024

kasperk81 commented Oct 1, 2024

kasperk81 commented Oct 1, 2024

eerhardt commented Oct 1, 2024

JimBobSquarePants commented Oct 30, 2024

kasperk81 commented Oct 30, 2024

JimBobSquarePants commented Oct 30, 2024

JimBobSquarePants commented Nov 14, 2024

eerhardt Nov 14, 2024

JimBobSquarePants Nov 14, 2024

eerhardt Nov 15, 2024

JimBobSquarePants Nov 15, 2024

eerhardt Nov 18, 2024

	int minCodeSize = stream.ReadByte();
	if (LzwDecoder.IsValidMinCodeSize(minCodeSize))

	public override int Read(Span<byte> buffer)
	{
	this.cancellationToken.ThrowIfCancellationRequested();

	// Too big for our buffer. Read directly from the stream.
	int count = buffer.Length;
	if (count > this.BufferSize)
	{
	return this.ReadToBufferDirectSlow(buffer);
	}

	// Too big for remaining buffer but less than entire buffer length
	// Copy to buffer then read from there.
	if ((uint)this.readBufferIndex > (uint)(this.BufferSize - count))
	{
	return this.ReadToBufferViaCopySlow(buffer);
	}

	return this.ReadToBufferViaCopyFast(buffer);
	}

	// Read doesn't always guarantee the full returned length so read a byte
	// at a time until we get either our count or hit the end of the stream.
	int count = buffer.Length;
	int n = 0;
	int i;
	do
	{
	i = baseStream.Read(buffer[n..count]);
	n += i;
	}
	while (n < count && i > 0);

	private int ReadToBufferViaCopySlow(Span<byte> buffer)
	{
	// Refill our buffer then copy.
	this.FillReadBuffer();

	return this.ReadToBufferViaCopyFast(buffer);
	}

	private void FillReadBuffer()
	{
	this.cancellationToken.ThrowIfCancellationRequested();
	Stream baseStream = this.BaseStream;
	if (this.readerPosition != baseStream.Position)
	{
	baseStream.Seek(this.readerPosition, SeekOrigin.Begin);
	}

	// Read doesn't always guarantee the full returned length so read a byte
	// at a time until we get either our count or hit the end of the stream.
	int n = 0;
	int i;
	do
	{
	i = baseStream.Read(this.readBuffer, n, this.BufferSize - n);
	n += i;
	}
	while (n < this.BufferSize && i > 0);

cleanup projects and environment variables #2812

cleanup projects and environment variables #2812

Conversation

kasperk81 commented Sep 22, 2024

CLAassistant commented Sep 22, 2024 • edited Loading

kasperk81 Sep 22, 2024

Choose a reason for hiding this comment

JimBobSquarePants left a comment

Choose a reason for hiding this comment

JimBobSquarePants commented Oct 1, 2024

kasperk81 commented Oct 1, 2024

kasperk81 commented Oct 1, 2024

eerhardt commented Oct 1, 2024

JimBobSquarePants commented Oct 30, 2024

kasperk81 commented Oct 30, 2024

JimBobSquarePants commented Oct 30, 2024

JimBobSquarePants commented Nov 14, 2024

eerhardt Nov 14, 2024

Choose a reason for hiding this comment

JimBobSquarePants Nov 14, 2024

Choose a reason for hiding this comment

eerhardt Nov 15, 2024

Choose a reason for hiding this comment

JimBobSquarePants Nov 15, 2024

Choose a reason for hiding this comment

eerhardt Nov 18, 2024

Choose a reason for hiding this comment

CLAassistant commented Sep 22, 2024 •

edited

Loading