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

Add missing ImageModes #5935

Merged
merged 2 commits into from
Jan 11, 2022

Conversation

FirefoxMetzger
Copy link
Contributor

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.

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

nulano commented Jan 6, 2022

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

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 👍

@@ -52,6 +52,11 @@ def getmode(mode):
"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

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

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

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

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 :)

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.

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