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.

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

Replace System.Drawing with SixLabors.ImageSharp #4

Merged
merged 3 commits into from
Jul 17, 2022
Merged

Replace System.Drawing with SixLabors.ImageSharp #4

merged 3 commits into from
Jul 17, 2022

Conversation

ahouts
Copy link
Contributor

@ahouts ahouts commented Jun 2, 2022

Microsoft recommends switching to a third party library for cross platform image manipulation:
https://docs.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/system-drawing-common-windows-only

I implemented the Save/Load bitmap functions with SixLabors.ImageSharp, all examples seem to work on my system now.

closes #3

Feel free to close this if you want to take a different approach, I was mostly interested in getting it to work on my system.

@alexdmiller
Copy link

I am still seeing this error on macOS:

Unhandled exception. System.TypeInitializationException: The type initializer for 'GUI' threw an exception.
 ---> System.NullReferenceException: Object reference not set to an instance of an object.
   at GUI..cctor() in /Users/alex/projects/MarkovJunior-2/source/GUI.cs:line 30
   --- End of inner exception stack trace ---
   at GUI.Draw(String name, Branch root, Branch current, Int32[] bitmap, Int32 WIDTH, Int32 HEIGHT, Dictionary`2 palette)
   at Program.Main() in /Users/alex/projects/MarkovJunior-2/source/Program.cs:line 67

@ersinesen
Copy link

You should clone the correct branch:
git clone --branch replace-System.Drawing-with-SixLabors.ImageSharp https://github.com/ahouts/MarkovJunior.git

And check the branch:
git branch -a

source/Graphics.cs Outdated Show resolved Hide resolved
@PaulusParssinen
Copy link
Contributor

PaulusParssinen commented Jun 2, 2022

I wonder if you can use Bgra32 directly so you don't need to swap the endianness manually 🤔

@ahouts
Copy link
Contributor Author

ahouts commented Jun 2, 2022

I wonder if you can use Bgra32 directly so you don't need to swap the endianness manually 🤔

Yeah that would probably be a better approach. I assumed the inverted byte order was endian shenanigans, so I didn't even think to check if the library supported it.
I'll switch to using that when I get home.

@ajzaff
Copy link

ajzaff commented Jun 3, 2022

Works great on Debian Linux. Thanks for writing it.

Copy link

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Love ImageSharp being used! 😄
I've left with some performance/memory improvements that can be done though.

Comment on lines +15 to +26
var image = Image.Load<Bgra32>(filename);
int width = image.Width, height = image.Height;
var result = new int[width * height];
for (var j = 0; j < height; j += 1)
{
for (var i = 0; i < width; i += 1)
{
result[j * width + i] = unchecked((int) image[i, j].Bgra);
}
}

image.Dispose();

Choose a reason for hiding this comment

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

You can use a using to avoid the explicit call to Dispose.
But most importantly, you can vectorize all this with the built-in API to copy the pixel buffer:

Suggested change
var image = Image.Load<Bgra32>(filename);
int width = image.Width, height = image.Height;
var result = new int[width * height];
for (var j = 0; j < height; j += 1)
{
for (var i = 0; i < width; i += 1)
{
result[j * width + i] = unchecked((int) image[i, j].Bgra);
}
}
image.Dispose();
using var image = Image.Load<Bgra32>(filename);
int width = image.Width;
int height = image.Height;
int[] result = new int[width * height];
image.CopyPixelDataTo(MemoryMarshal.Cast<int, Bgra32>(result));

Comment on lines +37 to +48
var formattedData = new Bgra32[data.Length];
for (var i = 0; i < data.Length; i += 1)
{
formattedData[i] = new Bgra32
{
Bgra = unchecked((uint) data[i])
};
}

var image = Image.WrapMemory(Configuration.Default, new Memory<Bgra32>(formattedData), width, height);
image.SaveAsPng(filename);
image.Dispose();

Choose a reason for hiding this comment

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

You can use a using and skip the whole allocation + copy by wrapping the source directly:

Suggested change
var formattedData = new Bgra32[data.Length];
for (var i = 0; i < data.Length; i += 1)
{
formattedData[i] = new Bgra32
{
Bgra = unchecked((uint) data[i])
};
}
var image = Image.WrapMemory(Configuration.Default, new Memory<Bgra32>(formattedData), width, height);
image.SaveAsPng(filename);
image.Dispose();
fixed (int* pData = data)
{
using var image = Image.WrapMemory<Bgra32>(pData, width, height);
image.SaveAsPng(filename);
}

A couple notes:

  • Need to add <AllowUnsafeBlocks>true</AllowUnsafeBlocks> to the .csproj for this
  • Make sure to add width/height validation as well before this suggested code.

Choose a reason for hiding this comment

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

Strongly recommend that this advice is followed. @Sergio0694 helped a lot to design and optimize the ImageSharp APIs

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks @Sergio0694 and @JimBobSquarePants, I updated it!

@ahouts
Copy link
Contributor Author

ahouts commented Jun 4, 2022

Love ImageSharp being used! smile I've left with some performance/memory improvements that can be done though.

Doesn't seem likely this PR will be merged given @mxgmn's response on #3 and here: #9 (comment). TBH I can appreciate the zero dependency approach @mxgmn seems to be going for given the only dependency this project had has caused issues.

I'll leave this PR open though so in the interim it has visibility until the permanent solution is implemented.

@gabrielelibardi gabrielelibardi mentioned this pull request Jun 5, 2022
@mxgmn mxgmn merged commit 4009094 into mxgmn:main Jul 17, 2022
@ahouts ahouts deleted the replace-System.Drawing-with-SixLabors.ImageSharp branch November 29, 2022 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Drawing removed on all operating systems except Windows
8 participants