Support creating BGR;15, BGR;16 and BGR;24 images, but drop support for BGR;32#7010
Merged
hugovk merged 5 commits intopython-pillow:mainfrom Mar 25, 2023
Merged
Support creating BGR;15, BGR;16 and BGR;24 images, but drop support for BGR;32#7010hugovk merged 5 commits intopython-pillow:mainfrom
hugovk merged 5 commits intopython-pillow:mainfrom
Conversation
radarhere
commented
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"] |
Member
Author
There was a problem hiding this comment.
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.
radarhere
commented
Mar 19, 2023
| def test_putpixel_unrecognized_mode(self): | ||
| im = hopper("BGR;15") | ||
| with pytest.raises(ValueError, match="unrecognized image mode"): | ||
| im.putpixel((0, 0), 0) |
Member
Author
There was a problem hiding this comment.
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.
Member
|
Let's also include this in release notes. |
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#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?
Pillow/src/libImaging/Unpack.c
Lines 691 to 693 in 3b4b77b
Pillow/src/libImaging/Unpack.c
Lines 736 to 738 in 3b4b77b
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.
Pillow/src/libImaging/Convert.c
Lines 285 to 291 in 3b4b77b
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
Pillow/src/libImaging/Storage.c
Lines 155 to 161 in 079caf6
which I think can't be accessed through our Python API without other support for the mode, and
Pillow/src/PIL/ImageMode.py
Line 64 in 079caf6
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.