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

Decode PNG to source stream's pixel type #1813

Closed
antonfirsov opened this issue Nov 7, 2021 · 8 comments · Fixed by #1861
Closed

Decode PNG to source stream's pixel type #1813

antonfirsov opened this issue Nov 7, 2021 · 8 comments · Fixed by #1861

Comments

@antonfirsov
Copy link
Member

antonfirsov commented Nov 7, 2021

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 on header.ColorType and header.BitDepth.

It would also help preserving the original images precision in case of header.BitDepth == 16.

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Nov 30, 2021

Looking at the code it might be best to do it like this. Anything else get complicated quickly.

In PngDecoder.Decode[Async]
Identify (skipping metadata)
Call correct Decode<TPixel> based upon PngColorType and PngBitDepth returned from Identify.

@tocsoft
Copy link
Member

tocsoft commented Nov 30, 2021

doing this will confuse users.. they will call Image.Load() and then when they try drawing etc then suddenly things wont work as expected.

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.

@JimBobSquarePants
Copy link
Member

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

@antonfirsov
Copy link
Member Author

antonfirsov commented Nov 30, 2021

I want to optimize for the 90%+ majority who don't do any alpha-sensitive operation. We already delivered a massive Image != Image<Rgba32> case with #1773, and that helped us to outperform standard SkiaSharp resize in terms of memory footprint, I really don't want to undo it, and I don't see why would be PNG special to deviate

This would be fine if its opt in

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 Image, so alpha could magically appear, when an operation needs it. This would require an implementation of the non-generic Image with composition over inheritance, and also some tweaks on the ImageProcessor API. This is non-trivial work that may delay the delivery of ImageSharp 2.0 by another month or two, which I believe may hurt us more than making alpha-aware drawing less convenient in certain cases.

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.

@JimBobSquarePants
Copy link
Member

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 BitmapCreateOptions.PreservePixelFormat but that’s opt in, not out.

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.

@antonfirsov
Copy link
Member Author

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:

  • Remove any signs of pixel type which is noise for non-experts
  • Enable alpha-free loads, that save 25% memory. (And match other libraries on this front.)

I prefer to communicate examples with non-generic Image.Load() in all the simple use-cases. Note that this is what we actually do today, and users actually started to adapt this pattern in the wild, which is good - makes their code simpler, and saves them memory.

We should make Image.Load<Rgba32>(...) the default form in all Drawing code snippets, or other examples where alpha could matter. We can place a short comment in those samples, explaining why the alpha channel has significance, and we can extend non-generic Image.Load docs with a disclaimer "Depending on the source format, this may load an image without an alpha channel." or similar.

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.

@JimBobSquarePants
Copy link
Member

We should make Image.Load<Rgba32>(...) the default form in all Drawing code snippets, or other examples where alpha could matter. We can place a short comment in those samples, explaining why the alpha channel has significance, and we can extend non-generic Image.Load docs with a disclaimer "Depending on the source format, this may load an image without an alpha channel." or similar.

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.

@antonfirsov
Copy link
Member Author

nor pad out the API

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.

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

Successfully merging a pull request may close this issue.

3 participants