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 ImagingAccess for I;16N on big-endian #7921

Merged
merged 11 commits into from
Apr 25, 2024
Merged

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Mar 30, 2024

Also changed the image mode info to use a NamedTuple.

The test currently fails for BGR;15 and BGR;16.

__________ TestImage.test_pixels_from_new_image_by_color[mode16] __________

self = <Tests.test_image.TestImage object at 0x7fa65defb1c0>, mode = ImageModeInfo(name='BGR;15', num_bands=3, pixel_size=2)

    @pytest.mark.parametrize("mode", image_modes)
    def test_pixels_from_new_image_by_color(self, mode: ImageModeInfo) -> None:
        color = 123 if mode.num_bands == 1 else tuple(range(mode.num_bands))
        im = Image.new(mode.name, (2, 2), color=color)

>       assert im.getpixel((0, 0)) == color
E       assert (0, 0, 0) == (0, 1, 2)
E         At index 1 diff: 0 != 1
E         Use -v to get more diff

Tests/test_image.py:670: AssertionError
__________ TestImage.test_pixels_from_new_image_by_color[mode17] __________

self = <Tests.test_image.TestImage object at 0x7fa65def9a80>, mode = ImageModeInfo(name='BGR;16', num_bands=3, pixel_size=2)

    @pytest.mark.parametrize("mode", image_modes)
    def test_pixels_from_new_image_by_color(self, mode: ImageModeInfo) -> None:
        color = 123 if mode.num_bands == 1 else tuple(range(mode.num_bands))
        im = Image.new(mode.name, (2, 2), color=color)

>       assert im.getpixel((0, 0)) == color
E       assert (0, 0, 0) == (0, 1, 2)
E         At index 1 diff: 0 != 1
E         Use -v to get more diff

Tests/test_image.py:670: AssertionError

@Yay295
Copy link
Contributor Author

Yay295 commented Mar 30, 2024

Looks like the issue with BGR;15 and BGR;16 was previously mentioned in #7303.

@Yay295 Yay295 marked this pull request as ready for review March 30, 2024 21:36
@Yay295 Yay295 changed the title Add test for creating an image from color values Fix ImagingAccess for I;16N on big-endian Mar 30, 2024
Tests/test_image.py Outdated Show resolved Hide resolved
@radarhere
Copy link
Member

Why you feel test_pixels_from_new_image_by_color is necessary?

I would think we are already testing this at

# check initial color
im = Image.new(mode, (1, 1), expected_color)
actual_color = im.getpixel((0, 0))
assert actual_color == expected_color, (
f"initial color failed for mode {mode}, "
f"expected {expected_color} got {actual_color}"
)
with almost all of the modes you're testing here.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 1, 2024

Because I didn't know that test existed. It looks like it will work with all of the modes though, so I'll update that instead.

@radarhere
Copy link
Member

I recognise this is relatively minor, but I would rather not add pixel size to Tests/helper.py just for use in a single test.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 6, 2024

It can be used for more tests in the future. I'm trying to keep my pull requests small so they actually get merged in a reasonable time frame.

@radarhere
Copy link
Member

more tests in the future

Such as? Apart from the scenario where a user manually constructs data for frombytes(), it's not immediately obvious when a user needs to know pixel size.

I would rather think of the test suite as testing the inputs and outputs of Pillow, and not containing data specific to our internals that most users don't need to know.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 6, 2024

I don't see an alternative. We need to know the pixel size of each mode for this test, so the pixel sizes need to be included in the list of mode names. There could be a separate list just for this test, but that defeats the point of having a common list of modes, and there would need to be another check that the two lists always have the same modes.

@hugovk
Copy link
Member

hugovk commented Apr 6, 2024

I recognise this is relatively minor, but I would rather not add pixel size to Tests/helper.py just for use in a single test.

It can be used for more tests in the future.

I agree with @radarhere, let's not move it now just in case, we can move it later when we need to move it, which might never happen (re: https://wiki.c2.com/?YouArentGonnaNeedIt).

I'm trying to keep my pull requests small so they actually get merged in a reasonable time frame.

Yes, this is the way, thank you :)

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 6, 2024

I thought of another way to do it. It's not efficient, but I suppose at least it's only being done in this one test.

@radarhere radarhere mentioned this pull request Apr 7, 2024
@Yay295
Copy link
Contributor Author

Yay295 commented Apr 8, 2024

So it looks like when getting BGR 15/16 data the values are scaled to be 0-255,

static void
get_pixel_BGR15(Imaging im, int x, int y, void *color) {
UINT8 *in = (UINT8 *)&im->image8[y][x * 2];
UINT16 pixel = in[0] + (in[1] << 8);
char *out = color;
out[0] = (pixel & 31) * 255 / 31;
out[1] = ((pixel >> 5) & 31) * 255 / 31;
out[2] = ((pixel >> 10) & 31) * 255 / 31;
}
static void
get_pixel_BGR16(Imaging im, int x, int y, void *color) {
UINT8 *in = (UINT8 *)&im->image8[y][x * 2];
UINT16 pixel = in[0] + (in[1] << 8);
char *out = color;
out[0] = (pixel & 31) * 255 / 31;
out[1] = ((pixel >> 5) & 63) * 255 / 63;
out[2] = ((pixel >> 11) & 31) * 255 / 31;
}

but when setting that data the values aren't scaled

Pillow/src/_imaging.c

Lines 607 to 623 in 33a73b5

if (!strcmp(im->mode, "BGR;15")) {
UINT16 v = ((((UINT16)r) << 7) & 0x7c00) +
((((UINT16)g) << 2) & 0x03e0) +
((((UINT16)b) >> 3) & 0x001f);
ink[0] = (UINT8)v;
ink[1] = (UINT8)(v >> 8);
ink[2] = ink[3] = 0;
return ink;
} else if (!strcmp(im->mode, "BGR;16")) {
UINT16 v = ((((UINT16)r) << 8) & 0xf800) +
((((UINT16)g) << 3) & 0x07e0) +
((((UINT16)b) >> 3) & 0x001f);
ink[0] = (UINT8)v;
ink[1] = (UINT8)(v >> 8);
ink[2] = ink[3] = 0;
return ink;

Though that doesn't explain why my original test passed.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 8, 2024

No wait, that can't be it, because this is only failing on big-endian.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 10, 2024

Does anyone know what the endianness of BGR;15 and BGR;16 is supposed to be?

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 11, 2024

It seems like BGR;15 and BGR;16 are always treated as little-endian, which would explain why they're not working on a big-endian machine, but I haven't yet found a place where they're being treated as native-endian rather than explicitly little-endian.

@radarhere radarhere mentioned this pull request Apr 13, 2024
@Yay295
Copy link
Contributor Author

Yay295 commented Apr 16, 2024

I added some logging in another branch and found some things.

First, the original RGB Hopper pixel data for the failing pixel is (117, 104, 121).

On a little-endian machine I found this:
After BGR;15 Conversion: 0b0011100110101111
After BGR;15 Conversion: (123, 106, 115)
After getdata putdata: 0b0011100110101111
After getdata putdata: (123, 106, 115)

The pixel values were changed a bit due to the RGB to BGR;15 conversions, but they're close enough.

On a big-endian machine I found this:
After BGR;15 Conversion: 0b1010111100111001
After BGR;15 Conversion: (205, 205, 90)
After getdata putdata: 0b0010111100111001
After getdata putdata: (205, 205, 90)

The pixel values are correct for that byte data, but the bytes are reversed. Also, the first bit (which is nothing in BGR;15) is set at the start, but not the end.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 16, 2024

All tests should be passing now. Some of the code was reading/writing RGB instead of BGR, and memcpy copies data in native-endianness when we need it to be in little-endian, so that can't be used for these modes.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 19, 2024

Should I rebase this, or can it be merged?

@radarhere
Copy link
Member

I don't see any need to rebase - there's no merge conflict.

If #7978 is merged, I'm not a fan of the BGR;* changes here, as I think introducing changes to BGR;* modes at the same time as deprecating them is not helpful.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 19, 2024

So, what, I have to wait until you finally remove the BGR modes before you'll accept this fix for I;16N?

@radarhere
Copy link
Member

I wasn't suggesting that this wait until the deprecation period ended, only that this wait until the question of deprecation was resolved with #7978 being accepted or rejected.

If you would like this to move ahead faster than that, then my suggestion would to be mark the BGR;* cases as failures for now - see Yay295#17. If #7978 is rejected, then the BGR;* changes here can be revisited.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 20, 2024

Rebased on main with a commit to ignore the BGR test failures instead of the commits to fix them.

@radarhere
Copy link
Member

I found that BGR;16 doesn't actually fail, and have created Yay295#18

Tests/test_image_access.py Outdated Show resolved Hide resolved
Tests/test_image_access.py Outdated Show resolved Hide resolved
Tests/test_image_access.py Outdated Show resolved Hide resolved
Tests/test_image_access.py Outdated Show resolved Hide resolved
@hugovk hugovk merged commit 1138ea5 into python-pillow:main Apr 25, 2024
56 checks passed
@Yay295 Yay295 deleted the testing branch April 25, 2024 18:41
mrbean-bremen pushed a commit to mrbean-bremen/Pillow that referenced this pull request Apr 25, 2024
Fix ImagingAccess for I;16N on big-endian
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