Allow ImageSharp to determine the best pixel format on load. #235
Conversation
Codecov Report
@@ Coverage Diff @@
## main #235 +/- ##
===================================
- Coverage 85% 85% -1%
===================================
Files 71 71
Lines 1945 1961 +16
Branches 286 290 +4
===================================
+ Hits 1672 1680 +8
- Misses 195 203 +8
Partials 78 78
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I'll add some tests to bring coverage up tomorrow |
|
Is the alpha channel enough? what about grayscale source images? there is currently no way that I can see for processor to tell us they plan on doing things like drawing a 2nd image frame over the top etc. |
|
Yeah should be enough. The idea is that individual processors will instruct the middleware whether an alpha channel is required so a custom drawing processor would always return true unless the developer wanted more fine grained control. |
|
@SixLabors/core OK. I have this working and am happy with the result. Please ignore the code coverage issues around LRU cache. I'm gonna open another PR to deal with that. |
There was a problem hiding this comment.
not a huge fan of the name RequiresAlphaAwarePixelFormat() as it more than just alpha it also impacts other things too, user uploading a PNG in L8 and a processor wanting water mark the image with red text they would also need to enable this flag otherwise it'll end up grayscale but none of that output is anything to do with Alpha as it'd work just as well on Rgb24 as well as Rgba32.
|
Agreed there.... What would you call it then? I've tried two names so far and one was only marginally better than the other 😵😅 "RequiresDeepColorPixelFormat" perhaps? https://en.wikipedia.org/wiki/Color_depth#Deep_color_(30-bit) Or should we return a bit depth enum Default
Bit24
Bit32and load the appropriate pixel format then?
It's actually impossible with the current architecture to change the color format of your input image unless you adjusted the metadata in one of the event handlers. e.g. If I were writing a watermarking processor I would also always use 32bit because that avoids me having to check the parsed color for an alpha component. |
|
I don't think we actually need to worry about 24 bit depths I was using that to demonstrate the difference really. Some options off the top of my head:
or inverted logic and call it
|
|
Inverting it is a fantastic idea! I’ll implement the change tomorrow. |
|
After a bit of research I ended up not inverting the logic and simply renaming the method. According to https://en.wikipedia.org/wiki/Color_depth#True_color_(24-bit)
|
Prerequisites
Description
This one's for @antonfirsov
This is a breaking change that massively optimizes memory usage for most common scenarios by allowing individual
IImageWebProcessorinstances to return a value indicating whether an alpha component is required in order to correctly process the image based upon the commands passed to that processor.If an
IImageWebProcessor.RequiresAlphaAwarePixelFormat(CommandCollection, CommandParser, CultureInfo)instance call returnstruethen we load anImage<Rgba32>as previously. If however, it returnsfalsethen we simply loadImage. This allows individual image formats to deliver the best pixel format based upon the encoded input stream.Control of the returned value can be fine-grained. For example,
ResizeWebProcessorwill only returntrueif it receives commands indicating that aBoxPadorPadresize operation should be performed since they can have the background color set.I've also thrown in some cleanup here to fix up the unit tests as the way they were constructed was causing issues against he non-threadsafe AWS test shim.