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

Charge target to netstandard2.0 and remove dependency to ImageSharp #32

Closed
wants to merge 2 commits into from

Conversation

PhenX
Copy link

@PhenX PhenX commented Mar 31, 2019

No description provided.

@Sappharad
Copy link
Contributor

As far as I know, your change appears to go against the purpose of this fork. (But I can't speak for @ststeiger so I don't know what he intended) There is already another .NET Standard fork of PdfSharp that uses System.Drawing. System.Drawing requires additional libraries to be installed to use outside of Windows, (libgdiplus on Linux for example) and is not officially supported in headless (services, web apps) environments despite the fact that most people used it that way on Windows. It is also not thread safe.

ImageSharp was designed to solve these problems - It's multi-platform .NET managed code with no native dependencies, works headless, and is thread safe. By getting rid of ImageSharp in your PR you re-introduce those limitations.

I started using ImageSharp and this fork a few weeks ago and found that it (ImageSharp) supported all of the drawing features we needed despite being slightly slower to draw than GDI+. Given that my team was incorrectly using System.Drawing in a server environment for years when we should not have been, we agreed that switching to ImageSharp was a good idea. I would recommend keeping ImageSharp unless Microsoft intends to make System.Drawing officially supported in headless environments.

Thoughts?

@ststeiger
Copy link
Owner

@Sappharad: Yea, not entirely.
If I guess correctly, he wanted to add it as interface, so you don't need to have ImageSharp as dependency.
He then added a default-source back that uses GDI+ (mono/Windows).
But it misses an implemented ImageSource for ImageSharp.
We just need to re-add a class that implements ImageSource.
I already merged another pull-request that boost the framework to NetStandard 2.0
I have little time at the moment, so I haven't merged this, just yet.
Another reason I didn't merge it, is that this breaks backwards compatiblity, which is bad.

@PhenX
Copy link
Author

PhenX commented Apr 3, 2019

@Sappharad @ststeiger As far as I understand, System.Drawing.Common is now cross platform : https://www.hanselman.com/blog/HowDoYouUseSystemDrawingInNETCore.aspx

Microsoft has released System.Drawing.Common to provide access to GDI+ graphics functionality cross-platform.

However, while I agree about the backwards compatibility break, I recommend using official packages provided by Microsoft.

The ImageSource implementation should be retrieved via dependeny injection, in my opinion, if you want to keep it customizable.

@Sappharad
Copy link
Contributor

Sappharad commented Apr 3, 2019

@PhenX The same article you linked does mention the native dependencies that must be installed on the server to use it on Linux

sudo apt install libc6-dev 
sudo apt install libgdiplus

The article you linked also mentions that you should not be using System.Drawing for new code.

There's lots of great options for image processing on .NET Core now! It's important to understand that this System.Drawing layer is great for existing System.Drawing code, but you probably shouldn't write NEW image management code with it. Instead, consider one of the great other open source options.

Scott's first recommendation is ImageSharp, which is exactly what you removed in this PR.

@ststeiger
Copy link
Owner

ststeiger commented Apr 4, 2019

@PhenX: Be careful about everything you read on hanselman's blog.
It's usually not really well researched, obsolete the day it is published, doesn't go into depth, and he doesn't tell you the bad parts, because he's a developer evangelist, not an actual user.

So for one, you need superuser-rights to install libgdiplus.
You might not have or even get them.
Then, it doesn't really work on Android/iOS (NetStandard is NetCore + Android + iOS).

Besides, I ported the full PdfSharp 1.5 to NetStandard, here.
So if you want to use GDI+, use that.
There are some issues, however.
See issues 5 and 6

Also, in some settings on Azure, you can't use GDI+.
And you shouldn't use GDI+, because the way it is built, it leaks memory, for no good reason.
GDI+ is an antique overcomplicated system that wasn't designed to be run on servers.
If your application has a few hundred users and should be always available, you can't just restart the server every now and then (apart from the fact that you then need to monitor memory in order to know when to restart). Also, memory isn't free, so if you run on AWS/Google/Azure, you'll pay for every byte you use, so you don't want to use more than necessary. Then, there still is that issue of security, buffer overflows etc. that you don't have with a fully managed library.

@aggsol
Copy link

aggsol commented Aug 8, 2019

@PhenX: Be careful about everything you read on hanselman's blog.
It's usually not really well researched, obsolete the day it is published, doesn't go into depth, and he doesn't tell you the bad parts, because he's a developer evangelist, not an actual user.

haha, I had to chuckle at this...

Besides, I ported the full PdfSharp 1.5 to NetStandard, here.

Which of your ports would you recomment to use on Linux then? Core or NetStandard?

@viceice
Copy link

viceice commented Jun 9, 2022

System.Drawing.Common is no longer crossplatform on net6.0

https://docs.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/system-drawing-common-windows-only

@PhenX PhenX closed this Mar 15, 2024
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.

5 participants