Skip to content

Use enums for Modes and RawModes in C #9100

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

Open
wants to merge 60 commits into
base: main
Choose a base branch
from

Conversation

eyedav
Copy link

@eyedav eyedav commented Jul 19, 2025

Rebase of PR #8510 (Use enums for Modes and RawModes in C) on latest main, done during EuroPython25's Sprint: creating a new PR as agreed with @wiredfool.

@radarhere radarhere changed the title Rebase of Use enums for Modes and RawModes in C Use enums for Modes and RawModes in C Jul 19, 2025
@radarhere
Copy link
Member

radarhere commented Jul 20, 2025

Would you mind writing up an explanation for why this is helpful?

@wiredfool
Copy link
Member

One of the things that I'd like to do here is move (almost) all of this stuff from Storage.c into the mode struct:

 } else if (mode == IMAGING_MODE_RGBX) {
        /* 32-bit true colour images with padding */
        im->bands = im->pixelsize = 4;
        im->linesize = xsize * 4;
        strcpy(im->band_names[0], "R");
        strcpy(im->band_names[1], "G");
        strcpy(im->band_names[2], "B");
        strcpy(im->band_names[3], "X");
}

The only thing here that can't be in the mode struct and static per mode is the linesize.

 } else if (mode == IMAGING_MODE_RGBX) {
        /* 32-bit true colour images with padding */
        im->linesize = xsize * 4;
}
... // and then
if (im->mode->bands == 4) {
}

So that gains us a lot less complexity in Storage.c, and 4 fewer allocations for each new image, as the mode bands are static and not generated fresh for each image. It's possible that we can just set im->linesize with a formula, but not clear yet.

(depending on the metadata standard) I'm hoping that we could statically generate the arrow mode metadata and store it there as well. This would work if it's just mode based, but it's less useful if it's going to include image specific metadata, like sizes.

@wiredfool
Copy link
Member

@eyedav I've got updates that fix the compilation errors and pass the tests here: https://github.com/wiredfool/Pillow/tree/mode_enums . I can't see how to make a PR or push against your repo -- there should be a setting somewhere to allow admins to push to this branch that you can enable.

@eyedav
Copy link
Author

eyedav commented Jul 20, 2025

@wiredfool the setting to allow maintainers to push to this branch was actually already enabled, I just tried to uncheck and check it again a moment ago. Could you please try a second time? Otherwise, as a workaround I could temporarily add you as collaborator to my fork.

@wiredfool
Copy link
Member

@eyedav I've tried again with no luck. If you can add me I'd appreciate it.

@@ -11,4 +11,4 @@ sphinx
types-atheris
types-defusedxml
types-olefile
types-setuptools
types-setuptools>=75.2.0
Copy link
Member

@radarhere radarhere Jul 19, 2025

Choose a reason for hiding this comment

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

Suggested change
types-setuptools>=75.2.0
types-setuptools

We're currently at 80.9.0. I think this has sufficiently propagated?

[IMAGING_MODE_RGBa] = {"RGBa"}, [IMAGING_MODE_YCbCr] = {"YCbCr"},

[IMAGING_MODE_BGR_15] = {"BGR;15"}, [IMAGING_MODE_BGR_16] = {"BGR;16"},
[IMAGING_MODE_BGR_24] = {"BGR;24"},
Copy link
Member

Choose a reason for hiding this comment

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

BGR;* modes (but not rawmodes) were removed in #9053

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.

4 participants