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

Eliminate experimental modes #2228

Open
homm opened this issue Nov 17, 2016 · 13 comments
Open

Eliminate experimental modes #2228

homm opened this issue Nov 17, 2016 · 13 comments

Comments

@homm
Copy link
Member

homm commented Nov 17, 2016

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:

  • Remove following modes with IMAGING_TYPE_SPECIAL type: I;16, I;16L, I;16B, I;16N, BGR;15, BGR;16, BGR;24, BGR;32
  • Remove RGBX mode as full equivalent of RGB
  • Keep widespread 1, P, L, LA, RGB and RGBA modes
  • Keep alternative linear (or near linear) color spaces CMYK and YCbCr
  • Keep derivatives with multiplicated alpha La and RGBa
  • Apparently, keep wide modes F and I
  • Create full list of the supported modes, monitor correctness of new or modified operations with all of them and include tests

Questionable modes:

  • Alternative color spaces with signed channels LAB and HSV
  • Palette with alpha PA. As I know palette supports RGBA modes, so it can handle "palette with alpha" in a different way
@wiredfool
Copy link
Member

wiredfool commented Nov 17, 2016

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:

  • Full support - Anything that Pillow can do can be done in these modes. RGB/RGBA would be examples here. This mostly covers the 1 mode as well, but it's a little bit special.
  • Partial support - There's basic support, we can read, write and store (in memory) images in this format, but there are holes in the coverage for other operations. (e.g. cropping works, but not necessarily resampling). This is the case with the I;16 modes, and the partial width RGB/BGR modes
  • Pack/unpack support - These are the raw modes, and they're only useful in transit to one of the storage modes.
  • Unsupported - Pixel formats that we just don't do, like multiband high bit depth and signed I;16.

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
Copy link
Member Author

homm commented Nov 17, 2016

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
Copy link
Member Author

homm commented Nov 17, 2016

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
Copy link
Member Author

homm commented Nov 17, 2016

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

IMAGING_TYPE_SPECIAL limitations:

  • Histogram is broken
  • Point only works for I;16
  • RankFilter raises ModeError
  • Resample raises ModeError

IMAGING_TYPE_UINT8 only functions:

  • ImageChops
  • Alpha Composite
  • GetBand
  • ImagingBlend
  • ImagingModeFilter

Appears to work with IMAGING_TYPE_SPECIAL:

  • Geometry Filters
  • Bounding Box
  • GetPixel
  • GetInk

Items that are not listed aren't dispatching on imaging type, so presumably they are supposed to work.

@behnam

This comment has been minimized.

@Yay295
Copy link
Contributor

Yay295 commented Sep 25, 2022

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
Copy link
Contributor

Yay295 commented Nov 30, 2022

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
Copy link
Member

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
Copy link
Contributor

Yay295 commented Dec 2, 2022

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
Copy link
Member

BGR;32 was removed in #7010

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

No branches or pull requests

6 participants