-
-
Notifications
You must be signed in to change notification settings - Fork 851
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
Decode PNG to source stream's pixel type #1813
Comments
Looking at the code it might be best to do it like this. Anything else get complicated quickly. In |
doing this will confuse users.. they will call This would be fine if its opt in, but by default we need to decode into a full feature rich pixel format that doesn't cause confusion and surprise to users... if we are 'broken' (in their eyes) they wont use the product and will tell everyone else to not use it either. |
@SixLabors/core I think we're going to need a larger conversation among the group to gain a consensus on what we want the non-generic API to do. |
I want to optimize for the 90%+ majority who don't do any alpha-sensitive operation. We already delivered a massive
I would better provide an easy, well communicated opt-out setting (Reason: optimize for perf of the 90% users) Configuration.Default.ForceAlphaChannelPresence = true;
using var img = Image.Load(...);
img.Mutate(c => c.DrawXyz(....)); 2.0 should come out with a detailed blogpost & conceptual doc explaining this property, and the main library where alpha can be a concern is Drawing, which is still in alpha. Another solution would be to make this this fully automatic, meaning that an ImageProcessor could alter the pixel type in a non-generic I may be wrong, but as far as I'm aware, no other imaging API-s do this. If you load a PNG of 24 bit RGB with System.Drawing or skia, you have to deal with 24 bit pixel data. |
I don’t believe you are wrong here. That’s my understanding of how all the libraries I’ve researched work. The closest I’ve seen otherwise is WPF with I don’t believe we need any configuration additional properties though. We already have mechanisms to ensure alpha component preservation - The generic API. As a side note the generic API was always supposed to be the primary API. I’d like to keep an emphasis there. Non-generic should be considered a fallback. |
My main motivation back in times with #897 / #904 was the opposite: to make the non-generic API default for basic use cases like thumbnail making in order to:
I prefer to communicate examples with non-generic We should make But I don't think we should roll back our code samples, and start communicating the non-generic overload as secondary, that removes big portion of the value added here. |
I agree. I just want to make it clear that we do not sacrifice the design of the generic API at any point for the non-generic one, nor pad out the API to compensate for a lack of functionality in the non-generic variant. Non-generic is strictly entry-level functionality. |
That also seems cleaner and simpler to me. We should improve our docs and our examples to warn users about the difference between the generic and the non-generic Load, and we should also mention this change in the 2.0 blog post. Alternative suggestions in #1813 (comment) are here for completeness. |
PNG decoder is not CPU intensive compared to other codecs, but we can save some memory if there is no alpha or if the PNG is grayscale. We can instantiate the
Image<T>
after reading the header chunk depending onheader.ColorType
andheader.BitDepth
.It would also help preserving the original images precision in case of
header.BitDepth == 16
.The text was updated successfully, but these errors were encountered: