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

Make System.Drawing.Common only supported on Windows #234

Merged
merged 21 commits into from
Jul 22, 2021

Conversation

terrajobst
Copy link
Member

No description provided.

@terrajobst terrajobst requested review from marklio and safern July 15, 2021 19:24
@safern
Copy link
Member

safern commented Jul 15, 2021

FYI: @ericstj @tarekgh @danmoseley @marek-safar @akoeplinger

@terrajobst terrajobst changed the title Add "Make System.Drawing.Common only supported on Windows" Make System.Drawing.Common only supported on Windows Jul 15, 2021
@chucker
Copy link

chucker commented Jul 15, 2021

I find “Common” too confusing, then.

Lots of stuff currently depends on it and expects cross-platform. IMHO, you should instead deprecate the package and consider a clearer name.

@terrajobst
Copy link
Member Author

Renaming a package/assembly is a breaking change. The marginal gain of a clearer name is far outperformed with the impact on the ecosystem IMHO.

Also, the platform compatibilty analyzer will inform you at build time that this component is Windows-only, so it's not like this would be a landmine for cross-platform code.

@atrauzzi
Copy link

The rationale behind the deprecation is sound, but the action taken -- well, I'll just say that the optics are suspect.

(If for some reason this same issue was affecting Windows, I'd put some money on Microsoft being more willing to invest effort to shore things up.)

Is this going to end up being something that people expect to have available, but are blindsided because it touches some parts of the MS ecosystem which are viewed as conduits to Windows adoption?


***Note:** This only affect `System.Drawing.Common` and would leave
`System.Drawing.Primitives` as-is. That assembly on contains primitive types
that don't depend on GDI+, such `Rectangle`, `Point`, and `Size`.*
Copy link
Member

Choose a reason for hiding this comment

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

Is there anything else in System.Drawing.Common that doesn't depend on libgdiplus and we should push down?

Copy link
Member

Choose a reason for hiding this comment

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

I haven't seen any, but still doing an analysis to see if we need to push types down.

@Tornhoof
Copy link

Aren't you a little late in the .NET 6 release cycle for such a breaking change? Considering that you want to release in ~4 months and you're already in Preview 6 and afaik Feature Development is mostly done. I imagine quite a few people are already either testing their applications with the preview bits or planning to do so and that change might throw a wrench into their plans considering that .NET 5 will be out of maintenance ~3 months later (and Xmas is right in the middle of these 3 months).

Looking at the products, I had my sticky fingers in, in the last 12 months, I immediately recognize the QR libraries and atleast one PDF library (Aspose.PDF, I expect pretty much all the pdf libraries on the market to use System.Drawing.Common) and these will make migrating to .NET 6 a lot more complicated.

(FWIW: I personally wholeheartedly agree with this step, from an enterprise dev pov I think it's too late.)

@safern
Copy link
Member

safern commented Jul 15, 2021

Looking at the products, I had my sticky fingers in, in the last 12 months, I immediately recognize the QR libraries and atleast one PDF library (Aspose.PDF, I expect pretty much all the pdf libraries on the market to use System.Drawing.Common) and these will make migrating to .NET 6 a lot more complicated.

While that is a valid concern,System.Drawing.Common is a standalone NuGet package which doesn't ship as part of the shared framework, so these libraries can definitely upgrade to .NET 6 and use the 5.0.0 version of the System.Drawing.Common package.

System.Drawing.Common doesn't have anything .NET 6 special and the only thing that couples it with .NET 6 is the release cycle.

terrajobst and others added 2 commits July 15, 2021 13:09
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@Tornhoof
Copy link

Tornhoof commented Jul 15, 2021

While that is a valid concern,System.Drawing.Common is a standalone NuGet package which doesn't ship as part of the shared framework

May I recommend that you point that out in the Q/A section of the document? I was not immediately aware of that fact while reading the document.

terrajobst and others added 2 commits July 15, 2021 13:11
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@hughbe
Copy link

hughbe commented Jul 15, 2021

As one of the most active contributors to libgdiplus, I hate to feel like I've wasted my time, but I do support this change. Libgdiplus doesn't have any maintainers, is riddled with incompatibilities, bugs, security flaws and missing features (e.g. metafiles, lots of brush features, regions). Also, its slow! I don't see anything other than a re-write as an option to fix this really, which is far too much work. Open source projects have already gone far enough.

However, could there be some sort of analyzer suggesting things like ImageSharp where we read images from files, for example?

terrajobst and others added 2 commits July 15, 2021 15:37
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@MaximRouiller
Copy link

Looking at the products, I had my sticky fingers in, in the last 12 months, I immediately recognize the QR libraries and atleast one PDF library (Aspose.PDF, I expect pretty much all the pdf libraries on the market to use System.Drawing.Common) and these will make migrating to .NET 6 a lot more complicated.

While that is a valid concern,System.Drawing.Common is a standalone NuGet package which doesn't ship as part of the shared framework, so these libraries can definitely upgrade to .NET 6 and use the 5.0.0 version of the System.Drawing.Common package.

System.Drawing.Common doesn't have anything .NET 6 special and the only thing that couples it with .NET 6 is the release cycle.

Are we expecting the 5.0.0 to have ongoing support past .NET support timeline? Just saying because otherwise it means you're stopping support for non-Windows platforms.

That kind of breaking change is exactly the type of scenario in my opinion that deserves a namespace/assembly change. Especially if we want Linux/Mac support.

No?

@safern
Copy link
Member

safern commented Jul 20, 2021

@RussKie: My main concern is that we're proposing/making changes like this so late in the release cycle.

@AraHaan: I think the best bet is to actually add the switch in .NET 7 (since it is too late for .NET 6), and then remove it and all support in .NET 8 for System.Drawing.Common on non-Windows OS's.

  1. In order to make previews useful, we need to be able to make breaking changes between them. However, we generally try to avoid breaking changes once we reach RC.

  2. The feedback that this is late is fair. Hence we don't plan to remove the functionality and instead only put it behind a runtime switch. The reason and motivation is to drive people's attention to the fact System.Drawing.Common isn't supported on non-Windows platforms and while this can be turned on, it won't ever get full fidelity.

  3. We want to use 6.0 to give people enough time to adopt to the recommended alternative so that we can remove the runtime switch in 7.0. Also since it is an LTS release we want to avoid more new libraries/apps using .NET 6 to adopt this legacy and broken xplat library.

Given that we are not removing functionality we don't consider this design change as blocking. The analyzer and default throwing behavior ensures that what we had in our documentation is communicated more visibly.

@safern
Copy link
Member

safern commented Jul 21, 2021

@terrajobst good to merge?

@danmoseley
Copy link
Member

@terrajobst I think we can merge this now?

@terrajobst terrajobst merged commit eed7a92 into dotnet:main Jul 22, 2021
@terrajobst terrajobst deleted the making-system-drawing-windows-only branch July 22, 2021 18:24
@ANahr
Copy link

ANahr commented Jul 22, 2021

Besides I also know of actual commercial software that expects System.Drawing.Common to be xplat as well too.

@AraHaan could you share some of these popular applications/libraries? You mentioned SVG.NET.

Not directed to me, But e.g. ClosedXML uses it which alone has more than 12 Mio Downloads.

@danmoseley
Copy link
Member

@safern we should probably reach out proactively to some of these top users like ClosedXML

@safern
Copy link
Member

safern commented Jul 30, 2021

@safern we should probably reach out proactively to some of these top users like ClosedXML

Yup, I'll do that.

@danmoseley
Copy link
Member

Thanks. that will help us validate the decision to remove the code next release. Also perhaps we can aggregate best practice suggestions for moving off it by looking at what they choose to do.

@Pangamma
Copy link

Pangamma commented Oct 21, 2021

This completely destroys one of my image editor projects. I'll have to stay on the older version of System.Drawing.Common after this change. Already tried switching to two other alternative frameworks and it is just a really heavy lift. Not worth the effort basically. Really sad that newer versions will start breaking code for linux users.

@ericstj
Copy link
Member

ericstj commented Oct 21, 2021

@Pangamma maybe you could open an issue in dotnet/runtime which describes the difficulty porting to those other frameworks? Perhaps we could share some resources to make that easier. I know @safern was working on some documentation around this and we're open to helping folks make the transition by contributing to some of those cross platform libraries.

Note that the only thing done in this release is to introduce an exception which can be disabled with System.Drawing.EnableUnixSupport, so you can use the latest package if you set this.

@Pangamma
Copy link

@Pangamma maybe you could open an issue in dotnet/runtime which describes the difficulty porting to those other frameworks? Perhaps we could share some resources to make that easier. I know @safern was working on some documentation around this and we're open to helping folks make the transition by contributing to some of those cross platform libraries.

Note that the only thing done in this release is to introduce an exception which can be disabled with System.Drawing.EnableUnixSupport, so you can use the latest package if you set this.

As long as we can disable the exception it should be... okay for now.
The main things I needed to swap out was palette quantization, image resizing, dithering, and image save/load. Also combining tiled images onto larger images. It's not a lot of different use cases. It's just that the code is everywhere. It would be nice to know which drawing functions will blow up on linux and which ones will not. Right now my console app (.net 5) actually works great on linux with no issues. I'm hoping it will remain strong in the future.

@JimBobSquarePants
Copy link

JimBobSquarePants commented Oct 22, 2021

The main things I needed to swap out was palette quantization, image resizing, dithering, and image save/load. Also combining tiled images onto larger images.

@Pangamma ImageSharp has all that functionality baked in. If you do decide to port please ping me. It'll be useful to observe and help out.

we're open to helping folks make the transition by contributing to some of those cross platform libraries.

@ericstj I think that has to absolutely happen, and sooner rather than later. That's a lot of pressure for Microsoft to put on OSS projects like ImageSharp who receive little to no assistance/funding.

@danmoseley
Copy link
Member

@JimBobSquarePants agreed.

@AraHaan
Copy link
Member

AraHaan commented Oct 22, 2021

I feel like the best course of action is have System.Drawing.Common on *nix systems to actually not use GDI+ backend but instead wrap ImageSharp so that way those who need to port from it would not actually need to and it avoids using GDI+ that contains issues. Besides I think all the functionality in System.Drawing.Common is in ImageSharp no?

But then again, why not just have it use ImageSharp for all platforms as well (including Windows) that way one can compile System.Drawing.Common once and it works for all OS's without any needs to checking what the operating system is inside of it's code.

So perhaps the .NET Foundation should consider partnering with SixLabors so they can move System.Drawing.Common into unconditionally using ImageSharp. Also there would be more benefits as well too (performance ones) given away for free too just by System.Drawing.Common using ImageSharp.

Besides some people even may want to use System.Drawing.Common in order to basically be their GUI framework for drawing for the OS they are making entirely in C# (yes people done this before).

Only issue is however, I do not know if ImageSharp can draw images to a computer screen.

@danmoseley
Copy link
Member

The System.Drawing surface area is a thin layer over Windows API's. I do not know whether that would map properly to any other library and give a reasonable result. I am not familiar with the libgdiplus implementation, but that may have been part of the challenge they had. @antonfirsov ?

Just to reiterate for anyone coming here: code that uses System.Drawing on Windows today is not affected: that is not going away.

@filipnavara
Copy link
Member

The System.Drawing surface area is a thin layer over Windows API's. I do not know whether that would map properly to any other library and give a reasonable result.

FWIW we still maintain the implementation built on top of CoreGraphics (forked from https://github.com/mono/sysdrawing-coregraphics). Of course that's not suitable as a cross-platform solution since it's macOS/iOS-only but it shows that writing a wrapper like that is possible.

@danmoseley
Copy link
Member

would SkiaSharp be more relevant? it is client targeted.

@antonfirsov
Copy link
Member

antonfirsov commented Oct 22, 2021

Before we go too far with conclusions, @Pangamma can you provide more details about your application/use-case? Is it a sever app? Or does it have ui? Is it xplat? How complex is it?What does it do in nutshell?

@Pangamma
Copy link

Pangamma commented Oct 22, 2021

Before we go too far with conclusions, @Pangamma can you provide more details about your application/use-case? Is it a sever app? Or does it have ui? How complex is it?What does it do in nutshell?

It's actually open source. Here you go. https://taylorlove.info/pixelstacker/
https://github.com/Pangamma/PixelStacker/tree/users/pangamma/rewrite-everything/src/PixelStacker.Logic

@Pangamma
Copy link

would SkiaSharp be more relevant? it is client targeted.

Skiasharp is super fast, but it ends up adding around 18mb to the final output program which is normally 5mb. Also the output exe is not fully contained inside a single file anymore when I do that. It has satellite DLLs for skiasharp which is a bit of a no-go for my personal use case.

@AraHaan

Only issue is however, I do not know if ImageSharp can draw images to a computer screen.

I haven't found a way to directly do that yet either. Would be nice though. Right now my best imagined approach would be to save the data to an array in memory then load that same data using the winforms compatible bitmap class. It would get the job done, I suppose.

@AraHaan
Copy link
Member

AraHaan commented Oct 22, 2021

True but System.Drawing.Bitmap is not xplat though since it's using GDI+ apis and as such is Windows only in nature now.

Sadly Some people utilize System.Drawing.Bitmap for pixel manipulations for image data that technically do have xplat alternatives like ImageSharp however (for those who only use it for modifying image data).

Also unfortunately for me I was tempted a few years ago to abuse System.Drawing.Graphics to make a pseudo GUI application for all platforms where I could control how everything would look like. But since that class is also GDI+ in nature it is also Windows-specific too.

@antonfirsov
Copy link
Member

antonfirsov commented Oct 23, 2021

I think that any attempt to maintain System.Drawing as a viable cross platform option is going to be a compatibility bug farm and maintenance hell no matter if we try to back it by ImageSharp, SkiaSharp, CoreGraphics or Cairo, therefore waste of time and energy on the wrong thing.

I'm much more in favor of a strategy where we instead help users migrating to modern libraries, and make sure that those libs provide superior developer experience.

Only issue is however, I do not know if ImageSharp can draw images to a computer screen.

It cannot, but it still can be used for double-buffered rendering which might be good enough in some cases. We even have an API to wrap arbitrary memory buffers as Image<TPixel> instances, if that helps anyone.

If you need native, GPU-accelerated drawing, you should be probably looking at MAUI and Maui.Graphics, though I'm not sure if they support "single-file deployment" in the manner described in #234 (comment). /cc @mattleibow

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.