python-pillow / Pillow

Python Imaging Library (Fork)
https://python-pillow.org
Other
12.06k stars 2.21k forks source link

Eliminate experimental modes #2228

Open homm opened 7 years ago

homm commented 7 years ago

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 in ImagingNewPrologueSubtype, this is not an experiment. This is just another unfinished feature which someone else needs to support.

My suggestions are:

Questionable modes:

wiredfool commented 7 years ago

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 I;16 modes in GIS type applications, and I'm pretty sure that the BGR;15 type modes are used in texture mapping and other image compression for 3d graphics. I don't know if we should be making the various partial width modes just pack/unpack modes to the standard RGB, or if they should stay as storage modes. The 16 bit per channel modes seem to be something that's showing up in more places. There are outstanding requests for I;16S and multi-band 16 bit images, and I think we've had a couple of people think about doing the work on the multi-band ones. That points to not dropping the support, but rather rationalizing the storage for wider channels.

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.

PA is distinct from an RGBA palette. A RGBA palette image is limited to 256 color+transparency combinations, where a PA is a 256 color image with a 256 grey mask.

homm commented 7 years ago

Pack/unpack support

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.

homm commented 7 years ago

I know we've people using the I;16 modes in GIS type applications

I'm totally for 16 bit per channel modes support for L and other modes, аnd I hope that we will do that but for now, this doesn't look like something useful. How they are using Pillow for this? Which exactly operations are working with I;16 mode? On the other hand, it's s bit strange to remove I;16 mode now and add complete support for 16-bit modes latter, so it is reasonable to keep I;16 itself as is.

Bit this is not applicable to I;16B, I;16L and I;16N as storage modes. We just need to choose one 16-bit format and use it. There are no advantages of using several endings for processing, they can be easily converted one to another. Its support just adds complexity.

I'm pretty sure that the BGR;15 type modes are used in texture mapping

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 frombuffer and tobytes and this is enough for manipulating.

BGR;15, BGR;16 and BGR;24 have no advantages over RGB in terms of processing and the final result.

homm commented 7 years ago

Internally, I think we're converting the a and b channels to an unsigned, then back on save.

This explains why HSV resampling works as expected. So we need to fix comments in ImagingNewPrologueSubtype at least.

wiredfool commented 7 years ago

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.

wiredfool commented 7 years ago

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.

wiredfool commented 7 years ago

Looking at C layer functions that dispatch or check on IMAGINGTYPE*:

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.

behnam commented 6 years ago

I'm using Pillow-5.1.0 and have a problem with some webp file being loaded as RGBX, then the PNG encoder not being able to handle RGBX. Does this problem fall under this issue (meaning RGB should be used instead of RGBX), or would it be a PNG encoding problem (meaning RGBX should be handled similar to RGB)?

Yay295 commented 1 year ago

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.

I think this is a good argument for keeping all of the modes. If someone is loading an I;16B image and using something else to process it, they probably don't want Pillow to automatically convert it to I;16L.

Yay295 commented 1 year ago

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. frombytes() and tobytes() could be those methods, and everything else would work with unpacked native-endian data.

wiredfool commented 1 year ago

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 RGB if it's anything other than RGB or a lossless permutation is a good idea. There are too many other colorspaces that are used for that to be a good idea.

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: 1) Multi res. There are 5 image sizes in the tiff, from essentiall thumbnail to 500x500m across the globe. 2) Multi-channel. There can be multiple planes of image data, each with a distinct meaning, all contained in the same file. (Not just RGB, things like wind velocity, mean wind energy, or individual month averages) 3) Not just 8 bit channels. Bit, Byte, Int16, and Floats are all in play.

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.

Yay295 commented 1 year ago

I think the only potential backwards compatibility issue would be with Image.frombuffer() (and Image.fromarray(), which uses frombuffer) because "changes to the original buffer object are reflected in this image". The buffer object would have to use native endianness instead of the mode's endianness.

radarhere commented 1 year ago

BGR;32 was removed in #7010