-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Eliminate experimental modes #2228
Comments
Experimental is a word, a more accurate word would be legacy. It's released, in the wild, and I'm pretty sure that a lot of it is used. From what I can tell, there are 4 classes of support for modes:
I totally agree on putting together a list of the supported modes and monitoring the correctness of operations with them. Tests are always good. I don't think that we should be dropping support for the special modes, as I know we've people using the Lab is a good color space to have, it's the perceptual match to the RGB for phosphor and CMYK for ink. If you're doing resampling, it's probably the best/most accurate space to be in. A straight line in Lab space is perceptually smooth, where it's not in RGB or HSV. Internally, I think we're converting the a and b channels to an unsigned, then back on save. (or at least, I remember looking at samples and trying to rationalize what was happening in the image formats) HSV is useful for a few things. I didn't think that it was a signed format though.
|
Oh, I missed this in the text, but all of this is not related to the packing/unpacking. Of course, we should support as many raw modes as we need for supported image formats. |
I'm totally for 16 bit per channel modes support for Bit this is not applicable to
Yes, there are lots of storage formats in the world. Like BGRa in Cairo. But this doesn't mean that we should support processing in such modes. We can read and write them through
|
This explains why |
I'm not sure what they're doing with the various modes, but I think that a huge part of the value is simply a load, offload to numpy/scipy and the reverse back to saving into a tiff. The endian issues with the I;16 date back to some of the tiff processing, It's likely that most of those should just wind up as the pack/unpack raw modes. |
One thing I've noticed with the various modes is that when converting between different widths of equivalent images, e.g. I;16 to L, the default conversion is to clip the range to 0-255. There's no simple way to upconvert an L image to the MSB of an I;16 or I;32, nor a way to downconvert that MSB to an L image. #2574 is an example of that. Also need to investigate the ability to save in a specific format given a more general one, e.g. save an I;16S from an I;32 image. |
Looking at C layer functions that dispatch or check on IMAGING_TYPE_*: IMAGING_TYPE_SPECIAL limitations:
IMAGING_TYPE_UINT8 only functions:
Appears to work with IMAGING_TYPE_SPECIAL:
Items that are not listed aren't dispatching on imaging type, so presumably they are supposed to work. |
This comment has been minimized.
This comment has been minimized.
I think this is a good argument for keeping all of the modes. If someone is loading an |
I do think a consistent internal format would be very helpful for processing images though. It would probably work to use RGB and native endianness internally, and have specific methods for exporting and importing using the experimental modes. |
I think for the most part, native endianness is a good idea, legacy issues aside. On the other hand, I don't think that putting data into There is definitely a broader discussion to be had on storage formats, especially when we're getting to multi-byte multi-planar images. There are definitely several ways this could go, depending on what we're really trying to support. First -- It would be good to have an internal storage that's extendable to any raster image that we're likely to see. The most complex image I've personally dealt with would be a >10 channel Cloud Oriented GeoTiff, with 5 levels of pyramid previews. These are:
Now I'm not arguing that we're going to need to serve the interests of the Geo community, mainly because they've got tools that do this already that interface with python (gdal and friends). But if we're going to go through the effort of figuring out an internal format, I wouldn't want to do it twice. Second -- We should be looking at something that's easily zero copy sharable with the data community. We've currently got numpy/pandas covered, but if we were using something like Arrow, we'd interop with a huge ecosystem, potentially even outside of the python world. (PyArrow is a common addition to pandas, and it allows for zero copy arrow->pandas dataframe. Also, polars is really nice) I don't know how well this would work with some of our procesing, but it may open up avenues for us to easily do SIMD accelerated operations on the data using external libraries (like polars). Third -- I think we might want to think about composite storage for images -- either planar storage where we have n single channel data structures, one per band, or tiled storage where we have one per tile in the original image. This could allow us to partially load giant images (like GeoTiffs) and process them efficiently. |
I think the only potential backwards compatibility issue would be with |
BGR;32 was removed in #7010 |
Every time when I open Pillow's C sources, I absolutely scared by all these experimental modes. Do I need to worry how not to break them? Do I need to implement features for all of them? How can I write tests for them?
In my opinion, an experiment must have the following characteristics: hypothesis; subject; deadline or exit condition; metric; outcomes. When someone adds another
else if
inImagingNewPrologueSubtype
, this is not an experiment. This is just another unfinished feature which someone else needs to support.My suggestions are:
IMAGING_TYPE_SPECIAL
type:I;16
,I;16L
,I;16B
,I;16N
,BGR;15
,BGR;16
,BGR;24
,BGR;32
RGBX
mode as full equivalent ofRGB
1
,P
,L
,LA
,RGB
andRGBA
modesCMYK
andYCbCr
La
andRGBa
F
andI
Questionable modes:
LAB
andHSV
PA
. As I know palette supportsRGBA
modes, so it can handle "palette with alpha" in a different wayThe text was updated successfully, but these errors were encountered: