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

Support creating BGR;15, BGR;16 and BGR;24 images, but drop support for BGR;32 #7010

Merged
merged 5 commits into from Mar 25, 2023

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Mar 13, 2023

#6769 (comment) mentioned that creating BGR images does not work. Image.new("BGR;15", (1, 1)) does not work, and neither does BGR;16, BGR;24 or BGR;32.

So what are these modes?

ImagingUnpackBGR15(UINT8 *out, const UINT8 *in, int pixels) {
int i, pixel;
/* RGB, reversed bytes, 5 bits per pixel */

ImagingUnpackBGR16(UINT8 *out, const UINT8 *in, int pixels) {
int i, pixel;
/* RGB, reversed bytes, 5/6/5 bits per pixel */

So with that pattern, BGR;24 would allow 8 bits per channel, the same size as an RGB channel. And yes, when converting, the values are just reversed.

rgb2bgr24(UINT8 *out, const UINT8 *in, int xsize) {
int x;
for (x = 0; x < xsize; x++, in += 4) {
*out++ = in[2];
*out++ = in[1];
*out++ = in[0];
}

So this PR adds support for creating images with those three image modes.

Continuing on, with that pattern, one could imagine that BGR;32 is a mode that supports 10 or 11-bit values for each channel.

However, there isn't any concrete information about that. Pillow doesn't currently support converting images to BGR;32, nor does it support any kind of unpacking with this mode.

In fact, the only code about this mode is

} else if (strcmp(mode, "BGR;32") == 0) {
/* EXPERIMENTAL */
/* 32-bit reversed true colour */
im->bands = 1;
im->pixelsize = 4;
im->linesize = (xsize * 4 + 3) & -4;
im->type = IMAGING_TYPE_SPECIAL;

which I think can't be accessed through our Python API without other support for the mode, and
"BGR;32": ("RGB", "L", ("B", "G", "R"), endian + "u4"),

which doesn't offer any value to the user if nothing else can be done with this mode.

So if ImageMode has the only functionality that would change for users (and it has only been in there since #5935), I'm going to suggest just dropping support for BGR;32 without deprecation - there isn't any demonstrated need for BGR;32, and we would just be inventing a mode by trying to specify what it is.

@radarhere radarhere changed the title Dropped support for BGR;32 mode Support creating BGR;15, BGR;16 and BGR;24 images, but drop support for BGR;32 Mar 19, 2023
IMAGE_MODES1 = ["L", "LA", "RGB", "RGBA"]
IMAGE_MODES2 = ["I", "I;16", "BGR;15"]
IMAGE_MODES1 = ["LA", "RGB", "RGBA"]
IMAGE_MODES2 = ["L", "I", "I;16"]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later in this file, IMAGE_MODES2 are tested to raise "color must be int or single-element tuple", while IMAGE_MODES1 are tested to raise "color must be int or tuple". This makes sense, as all IMAGE_MODES2 are single channel, while IMAGE_MODES1 have multiple channels.

def test_putpixel_unrecognized_mode(self):
im = hopper("BGR;15")
with pytest.raises(ValueError, match="unrecognized image mode"):
im.putpixel((0, 0), 0)
Copy link
Member Author

@radarhere radarhere Mar 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error came from converting an image to a mode that putpixel was unable to getink with. However, all modes that an image can be converted to are now supported by getink after this PR.

The error should still be in C in case we break something in the future, but I don't think it can be tested.

@hugovk
Copy link
Member

hugovk commented Mar 25, 2023

Let's also include this in release notes.

@hugovk hugovk merged commit 20293e1 into python-pillow:main Mar 25, 2023
61 of 63 checks passed
@radarhere radarhere deleted the bgr32 branch March 25, 2023 20:15
@radarhere radarhere mentioned this pull request Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants