Skip to content

Add missing ImageModes#5935

Merged
radarhere merged 2 commits intopython-pillow:mainfrom
FirefoxMetzger:add-missing-ImageModes
Jan 11, 2022
Merged

Add missing ImageModes#5935
radarhere merged 2 commits intopython-pillow:mainfrom
FirefoxMetzger:add-missing-ImageModes

Conversation

@FirefoxMetzger
Copy link
Contributor

@FirefoxMetzger FirefoxMetzger commented Jan 6, 2022

Fixes #5933

This PR adds the modes mentioned in the docs to ImageMode. It also adds the mode BGR to be used as a basetype for the missing modes.

Sidenote: To get pillow 9.1.0.dev0 to build on Windows I had to do some modifications to the script in winbuild\README.md, e.g., because the script/snippet builds for python3.8, but calls python3.7 to prepare - which I don't have. I will leave my modified script here, since I don't know where else to put it, but if there is interest in a dedicated DOC PR for this, I can do that, too:

set PYTHON=C:\Users\<username>\AppData\Local\Programs\Python\Python38
set PILLOW_REPO=<git-clone-location>\Pillow

cd /D %PILLOW_REPO%\winbuild
%PYTHON%\python.exe build_prepare.py -v --depends=%PILLOW_REPO%\depends
build\build_dep_all.cmd
build\build_pillow.cmd install

path %PILLOW_REPO%\winbuild\build\bin;%PATH%
%PYTHON%\python.exe -m pip install -U -e .
%PYTHON%\python.exe selftest.py
%PYTHON%\python.exe -m pytest -vx --cov PIL --cov Tests --cov-report term --cov-report xml Tests
%PILLOW_REPO%\winbuild\build\build_pillow.cmd bdist_wheel

Sidenote2: I didn't manage to get the full test suite to pass after building pillow. One test (Tests/test_image_access.py::TestEmbeddable::test_embeddable) keeps failing. However, I think this is because the test can't find python38.lib on my system rather than something related to the PR.

@FirefoxMetzger
Add missing ImageModes
f40c005 
The modes are mentioned in the docs, but weren't part of ImageMode.
@nulano
Copy link
Contributor

nulano commented Jan 6, 2022 •
edited
Loading

Did you read the instructions for the Windows build, or just the sample script? I think most of your issues with it are explained in the longer documentation: https://github.com/python-pillow/Pillow/blob/main/winbuild/build.rst

For example, you do not need to pass the --depends=... parameter, and you don't need to pip install -e . if you build_pillow.cmd --inplace develop instead of build_pillow.cmd install.

One test (Tests/test_image_access.py::TestEmbeddable::test_embeddable) keeps failing.

A known issue, sort of, #5811 (comment). You might notice at the bottom of this PR it says All checks have passed. This includes testing on Windows, but this test is skipped if you look at the log.

@FirefoxMetzger
Copy link
Contributor Author

FirefoxMetzger commented Jan 6, 2022

Did you read the instructions for the Windows build, or just the sample script?

@nulano Neither, because I missed the full instructions. I found Readme.md and it appeared to get me close enough, so I didn't follow the links therein. The full instructions indeed make the script cleaner and I should have read them; my bad.

If you don't think it is worthwhile to change or add to the existing example then let's not worry about it. That's why I put it as a side note to begin with; mainly to see if it is useful and/or if there is any interest in it :)

A known issue, sort of, #5811 (comment). You might notice at the bottom of this PR it says All checks have passed. This includes testing on Windows, but this test is skipped if you look at the log.

Then it's all good from my side 👍

radarhere
radarhere reviewed Jan 7, 2022
View reviewed changes
src/PIL/ImageMode.py Outdated
"HSV": ("RGB", "L", ("H", "S", "V")),
# extra experimental modes
"RGBa": ("RGB", "L", ("R", "G", "B", "a")),
"BGR": ("BGR", "L", ("B", "G", "R")),
Copy link
Member

@radarhere radarhere Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see the need for this? BGR is not listed in the docs or in Storage.c

Copy link
Contributor Author

@FirefoxMetzger FirefoxMetzger Jan 7, 2022

Choose a reason for hiding this comment

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

@radarhere Sure, we can remove it. In this case, what should be the base mode for the respective BGR;X? That mode itself?

Copy link
Member

@radarhere radarhere Jan 10, 2022

Choose a reason for hiding this comment

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

You've already listed the base mode for BGR;* as just RGB.

Copy link
Contributor Author

@FirefoxMetzger FirefoxMetzger Jan 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@radarhere Oh, right. That was unintentional on my part. Initially I thought the basemode for BGR;* should be BGR and added mode BGR correspondingly.

However, since RGB is the desired mode, I will keep it and drop mode BGR.

Edit: Done :)

@radarhere radarhere merged commit 9a9afd3 into python-pillow:main Jan 11, 2022
@FirefoxMetzger FirefoxMetzger deleted the add-missing-ImageModes branch January 11, 2022 04:48
@CastixGitHub CastixGitHub mentioned this pull request Apr 5, 2022
Closed
@radarhere radarhere mentioned this pull request Mar 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@radarhere radarhere radarhere left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

BGR;[15,16,34,32] modes are documented but not part of ImageMode

3 participants

@FirefoxMetzger @nulano @radarhere