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

Fix putdata() for I;16N on big-endian #7209

Merged
merged 16 commits into from
Mar 30, 2024
Merged

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Jun 11, 2023

Added some tests for Image methods that deal with bytes, and a test that helper.hopper() works for all image modes.

One of the tests failed though: https://github.com/Yay295/Pillow/actions/runs/5237154342/jobs/9455150887
FAILED Tests/test_image.py::TestImageBytes::test_pixels_after_getdata_putdata[I;16N-1-2] - assert (1, 1029, 515, 1543) == (256, 1284, 770, 1798)

For some reason doing image.putdata(image.getdata()) on an I;16N image on a big-endian machine (s390x) changes the pixels.

@Yay295
Copy link
Contributor Author

Yay295 commented Jun 12, 2023

Looks like the problem is here:

Pillow/src/_imaging.c

Lines 1574 to 1590 in 706f8f6

int endian = strncmp(image->mode, "I;16", 4) == 0 ? (strcmp(image->mode, "I;16B") == 0 ? 2 : 1) : 0;
double value;
for (i = x = y = 0; i < n; i++) {
set_value_to_item(seq, i);
if (scale != 1.0 || offset != 0.0) {
value = value * scale + offset;
}
if (endian == 0) {
image->image8[y][x] = (UINT8)CLIP8(value);
} else {
image->image8[y][x * 2 + (endian == 2 ? 1 : 0)] = CLIP8((int)value % 256);
image->image8[y][x * 2 + (endian == 2 ? 0 : 1)] = CLIP8((int)value >> 8);
}
if (++x >= (int)image->xsize) {
x = 0, y++;
}
}

The code is handling for endianness for "I;16B", but not "I;16N" on a big-endian machine.

@Yay295 Yay295 marked this pull request as ready for review June 12, 2023 22:23
src/_imaging.c Outdated Show resolved Hide resolved
@radarhere
Copy link
Member

a test that helper.hopper() works for all image modes

Why would we need to test that this runs without error for all image modes? hopper() isn't part of Pillow, just the test suite. I don't see a need to start testing the tests.

@Yay295 Yay295 force-pushed the bytes_tests branch 2 times, most recently from aae84f0 to e6ddb98 Compare June 13, 2023 13:02
@Yay295
Copy link
Contributor Author

Yay295 commented Jun 13, 2023

Yay295 force-pushed the bytes_tests branch

GitHub's suggestion auto-commit added an extra brace for some reason.

Why would we need to test that this runs without error for all image modes? hopper() isn't part of Pillow, just the test suite. I don't see a need to start testing the tests.

You're right. This will essentially be tested just by using it in other tests anyways.

@Yay295
Copy link
Contributor Author

Yay295 commented Jul 2, 2023

rebased on main

Tests/test_image.py Outdated Show resolved Hide resolved
@Yay295
Copy link
Contributor Author

Yay295 commented Mar 20, 2024

Can this be merged?

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

This PR has a fix to src/_imaging.c, let's rename the PR about that change, rather than about just adding tests.

Tests/test_image.py Outdated Show resolved Hide resolved
Yay295 and others added 2 commits March 20, 2024 08:55
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@Yay295 Yay295 changed the title Tests for byte methods Fix _putdata() for mode I;16N on big-endian Mar 20, 2024
@radarhere radarhere changed the title Fix _putdata() for mode I;16N on big-endian Fix putdata() for mode I;16N on big-endian Mar 21, 2024
@radarhere radarhere changed the title Fix putdata() for mode I;16N on big-endian Fix putdata() for I;16N on big-endian Mar 21, 2024
@radarhere radarhere removed the Testing label Mar 22, 2024
@radarhere
Copy link
Member

I've created Yay295#13 with some suggestions.

@hugovk
Copy link
Member

hugovk commented Mar 30, 2024

Thanks!

@hugovk hugovk merged commit 0164158 into python-pillow:main Mar 30, 2024
58 checks passed
@Yay295 Yay295 deleted the bytes_tests branch March 30, 2024 15:03
@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.

3 participants