Skip to content

Conversation

@brianpopow
Copy link
Collaborator

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

Fix for issue #2171: The alpha chunk data was written completely (the size of the image in pixels) and not only the compressed data when writing a lossy webp image.

@brianpopow
Copy link
Collaborator Author

There are new failing tests with .Net7.0, this time on macos only. 222 are failing:

Error Message:
   System.PlatformNotSupportedException : Operation is not supported on this platform.
  Stack Trace:
     at System.Runtime.Intrinsics.Vector256.Create(Single value)
   at SixLabors.ImageSharp.Formats.Jpeg.Components.FastFloatingPointDCT.<IDCT8x8_Avx>g__IDCT8x8_1D_Avx|16_0(Block8x8F& block) in /_/src/ImageSharp/Formats/Jpeg/Components/FastFloatingPointDCT.Intrinsic.cs:line 141
   at SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder.JpegComponentPostProcessor.CopyBlocksToColorBuffer(Int32 spectralStep) in /_/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegComponentPostProcessor.cs:line 70

@JimBobSquarePants
Copy link
Member

That’s concerning. @tannergooding sorry to page but are there changes coming to .NET 7 we should know about?

@tannergooding
Copy link
Contributor

System.Runtime.Intrinsics.Vector256.Create(Single value) should work on all platforms, it might just not be accelerated on certain ones (x64 machines without AVX and Arm64 machines in general).

Do you have a simple repro? I don't immediately see any changes to the .NET 7 version or to any code newly using Vector256 in this PR.

@brianpopow
Copy link
Collaborator Author

System.Runtime.Intrinsics.Vector256.Create(Single value) should work on all platforms, it might just not be accelerated on certain ones (x64 machines without AVX and Arm64 machines in general).

Do you have a simple repro? I don't immediately see any changes to the .NET 7 version or to any code newly using Vector256 in this PR.

Hi @tannergooding. I am not 100% sure if this will be enough, but from the error messages in this CI run I would assume just calling:

var scale = Vector256.Create(200.0f);

on a Mac should be enough to trigger this. Since I do not have a Mac, I cannot verify that.
@JimBobSquarePants Maybe you have a chance to test, if that is indeed enough?

@tannergooding
Copy link
Contributor

This might be an issue specifically around a machine with AVX but no AVX2 support. I'll see if I can get a repro

@JimBobSquarePants
Copy link
Member

Thanks @tannergooding I've been unable to reproduce this on my old 2.7 GHz Dual-Core Intel Core i5

@JimBobSquarePants
Copy link
Member

I got some environmental info via #2175

I'm at a loss to explain the cause.

OS=macOS Big Sur 11.6.7 (20G630) [Darwin 20.6.0]
Intel Xeon CPU E5-1650 v2 3.50GHz (Max: 3.34GHz), 1 CPU, 3 logical and 3 physical cores
.NET SDK=7.0.100-preview.6.22352.1
[Host] : .NET 7.0.0 (7.0.22.32404), X64 RyuJIT

@brianpopow
Copy link
Collaborator Author

I got some environmental info via #2175

I'm at a loss to explain the cause.

OS=macOS Big Sur 11.6.7 (20G630) [Darwin 20.6.0]
Intel Xeon CPU E5-1650 v2 3.50GHz (Max: 3.34GHz), 1 CPU, 3 logical and 3 physical cores
.NET SDK=7.0.100-preview.6.22352.1
[Host] : .NET 7.0.0 (7.0.22.32404), X64 RyuJIT

I think Tanner is on the right track with his guess that it's maybe related to a machine with AVX but not AVX2. Intel Xeon CPU E5-1650 only has AVX from what I can tell.

@antonfirsov
Copy link
Member

antonfirsov commented Jul 14, 2022

Edit: Missed your update @brianpopow 😄

This might be an issue specifically around a machine with AVX but no AVX2 support.

Seems correct, E5-1650 v2 is Ivy Bridge.

Maybe we can workaround these issues to get back clean CI until there is an upstream fix? We can disable Avx path on .NET7 when Avx2.IsSupported == false.

if (Avx.IsSupported)
{
IDCT8x8_Avx(ref block);
}

@JimBobSquarePants
Copy link
Member

Maybe we can workaround these issues to get back clean CI until there is an upstream fix? We can disable Avx path on .NET7 when Avx2.IsSupported == false.

You mean for this instance? I think we're just hitting this one cos it comes first. We use Vector256.Create(single) in a few places. Theoretically we could turn off the support by setting the correct environmental variable during CI but I think I'd rather just disable .NET 7 Mac tests for now.

@tannergooding
Copy link
Contributor

tannergooding commented Jul 15, 2022

Ok, I can reproduce the issue and will work on getting a fix up on the dotnet/runtime side.

@JimBobSquarePants
Copy link
Member

AWESOME!

@antonfirsov
Copy link
Member

I'd rather just disable .NET 7 Mac tests for now.

Yeah, makes sense.

@brianpopow brianpopow mentioned this pull request Jul 16, 2022
4 tasks
@JimBobSquarePants JimBobSquarePants added this to the 3.0.0 milestone Jul 18, 2022
@JimBobSquarePants JimBobSquarePants merged commit 1db6b21 into main Jul 18, 2022
@JimBobSquarePants JimBobSquarePants deleted the bp/Issue2171 branch July 18, 2022 03:24
@tannergooding
Copy link
Contributor

@JimBobSquarePants, the issue discussed above should be fixed as of dotnet/runtime#72522 and will land in .NET 7 RC1.

-- Also, sorry for the ping after this has been closed for two weeks; wasn't sure of a better place to relay this small note.

@JimBobSquarePants
Copy link
Member

@tannergooding That's fantastic news, thanks for letting me know.

That's a very interesting conversation regarding AVX/AVX2 issues. I wonder if any of that is causing our NaN handling issues in #2117 . I've struggled to find the specs for Intel Xeon Platinum 8272CL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants