Fix ImagingAccess for I;16N on big-endian

[Yay295]

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]

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

[radarhere]

Why you feel test_pixels_from_new_image_by_color is necessary?

I would think we are already testing this at

Pillow/Tests/test_image_access.py

Lines 186 to 192 in d31148b

	# 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]

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]

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]

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]

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]

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]

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]

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.

[Yay295]

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

Pillow/src/libImaging/Access.c

Lines 90 to 108 in 33a73b5

	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]

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

[Yay295]

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

[Yay295]

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.

[Yay295]

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]

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]

Should I rebase this, or can it be merged?

[radarhere]

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]

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

[radarhere]

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]

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

[radarhere]

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