Add tp_richcompare handler for Imaging_Type/ImagingCore #7260

Yay295 · 2023-07-05T05:10:13Z

Imaging_Type/ImagingCore is exposed by Image.getdata(), so this allows you to properly compare the result of that method without it being an identity comparison. This should also speed up comparison between Image objects, because now the comparison is done in C instead of passing the data back to Python for it to be compared.

Tests/test_lib_image.py

radarhere · 2023-07-05T08:13:45Z

Valgrind is currently failing - https://github.com/python-pillow/Pillow/actions/runs/5460694184/jobs/9937936242?pr=7260

Also, I went to test the speed of image equality with this, radarhere@98e02aa... and found https://github.com/radarhere/Pillow/actions/runs/5461893489/jobs/9940492884#step:8:4787

Fatal Python error: bool_dealloc: deallocating True or False: bug likely caused by a refcount error in a C extension

Yay295 · 2023-07-05T13:51:10Z

oops, Py_NewRef wasn't added until Python 3.10.

radarhere · 2023-07-06T02:53:14Z

src/_imaging.c

+    } else if (!strcmp(mode, "LA") || !strcmp(mode, "La") || !strcmp(mode, "PA")) {
+        // These modes have two channels in four bytes,
+        // so we have to ignore the middle two bytes.
+        mask = 0xff0000ff;


You shouldn't need to ignore the middle two bytes, they should be equal to the first two. See

Pillow/src/libImaging/Convert.c

Lines 234 to 242 in 3ffa8dc

static void

rgb2la(UINT8 *out, const UINT8 *in, int xsize) {

int x;

for (x = 0; x < xsize; x++, in += 4, out += 4) {

/* ITU-R Recommendation 601-2 (assuming nonlinear RGB) */

out[0] = out[1] = out[2] = L24(in) >> 16;

out[3] = 255;

}

}

for example.

I initially wondered if you were adding this mask for speed, but looking at the next block, removing this mask would allow memcmp to be used, which I expect would be faster.

"they should be equal to the first two" You'd think so, but they're not: https://github.com/Yay295/Pillow/actions/runs/5471546420/jobs/9962793523#step:8:1757

This is why I added those last two tests. Except I couldn't find a way to set this up from Python so I marked them to skip.

I added some logging: https://github.com/Yay295/Pillow/actions/runs/5472920560/jobs/9965699214#step:8:1759

After im.reduce() the data is 0xffb8b8b8, but after im.crop().reduce() the data is 0xff0000b8.

they should be equal to the first two

They set to the same with rgb2la conversion, but this doesn't mean that they should be equal

I added a new method to ImagingCore to allow directly setting the internal bytes. All of the tests work now.

also, RGBX technically has 4 channels, so it shouldn't be masked. also, fix the mask for modes with three channels.

homm · 2023-07-06T20:07:29Z

Tests/test_lib_image.py

+
+@pytest.mark.parametrize(
+    ("mode", "rawmode"),
+    (("RGB", "RGBX"), ("YCbCr", "YCbCrX"), ("HSV", None), ("LAB", None)),


It's more semantically.

Suggested change

(("RGB", "RGBX"), ("YCbCr", "YCbCrX"), ("HSV", None), ("LAB", None)),

[("RGB", "RGBX"), ("YCbCr", "YCbCrX"), ("HSV", None), ("LAB", None)],

A previous decision was to use tuples. #6525 (comment)

I've provided a detailed answer there. Hope @radarhere will agree with me

As an example you can look at the Django settings documentation. Settings are almost always immutable, but both lists and tuples are used for different things. Tuples are used for ADMINS items (first value is a name, second is an email), LANGUAGES items, SECURE_PROXY_SSL_HEADER (first value is a name, second is a value of header). While lists are used for all sorts of… well… lists.

I'm not overly concerned about which is used, and I don't think performance considerations are exactly a priority for tests, so I'm happy to go with @homm's preference here.

homm · 2023-07-06T20:07:40Z

Tests/test_lib_image.py

+
+
+@pytest.mark.skip(reason="no way to directly set C bytes from Python")
+@pytest.mark.parametrize("mode", ("LA", "La", "PA"))


Suggested change

@pytest.mark.parametrize("mode", ("LA", "La", "PA"))

@pytest.mark.parametrize("mode", ["LA", "La", "PA"])

homm · 2023-07-07T08:08:46Z

src/_imaging.c

@@ -3534,6 +3534,34 @@ _save_ppm(ImagingObject *self, PyObject *args) {
    return Py_None;
 }

+static PyObject *
+_set_internal_pixel_bytes(ImagingObject *self, PyObject *args) {


Personally I don't think this is a good idea to add core method just for testing. The problems are:

It needs maintenance in future

It exposes internals which will be harder to change

This method itself doesn't tested

However, for a long time I suffer from lack of functionality just to change internal model of the image without copying. Probably we can add something like ImagingCore.rewrite_mode() which will work within the same pixelsize.

It exposes internals which will be harder to change

I don't think it exposes anything that wouldn't also be exposed by ImagingCore.rewrite_mode().

This method itself doesn't tested

That can be added.

Probably we can add something like ImagingCore.rewrite_mode() which will work within the same pixelsize.

Unfortunately, pixelsize/linesize are not available anywhere except on an image - there is no list to loop through that has this information.

Damn, right. Maybe create minimal image in the target mode just to check it's pixel size?

I think that would work. I still don't see how this is any better than the function I already added though.

I can make Image.mode a property, but Image.mode is currently writable, while Image.im.mode is not.

I've created #7271 to look into that.

To unblock this PR I'd be happy with tests for modes we can test without core changes.

With ImagingCore.rewrite_mode() there are none, because ImagingCore.rewrite_mode() cannot update the mode that is currently returned from Image.mode.

I've created Yay295#8 as a suggestion.

Use single integer color instead of adding set_internal_pixel_bytes()

radarhere · 2023-10-06T12:10:24Z

Tests/test_lib_image.py

+    assert img_a.im != img_b.im
+
+
+@pytest.mark.parametrize("mode", [mode for mode in mode_names_not_bgr if mode != "1"])


Why is 1 excluded here?

Because if you scroll up a bit, there's test_not_equal_mode_1 just for mode 1 images, with comments explaining why it's different.

Basically, it's easier to create a mode 1 image from bytes if we use rawmode "1;8".

radarhere · 2023-10-07T02:00:02Z

Tests/test_lib_image.py

+    # alternatively, random.randbytes() in Python 3.9
+    data = secrets.token_bytes(num_img_bytes)


Apart from oss-fuzz, Pillow doesn't use random data in the test suite.

Different image modes need different amounts of bytes to create them, so this seemed like the best way to get enough bytes, and then get a second set of bytes that are different.

radarhere · 2023-10-07T03:23:25Z

src/_imaging.c

+            || _compare_pixels(
+                palette_a->mode,
+                1,
+                palette_a->size * 4,


Why is this multiplied by 4?

The third parameter is the line size, but palettes don't have a line size, so it has to be calculated from the number of pixels and the pixel size. Palettes also don't have a stored pixel size, but currently it's always 4. It's not good to hardcode it like this, but there isn't anywhere to actually get it from.

radarhere · 2023-10-07T03:32:37Z

Tests/test_lib_image.py

+    # Image.frombytes() doesn't work with BGR modes:
+    # unknown raw mode for given image mode
+    # "BGR;15",
+    # "BGR;16",
+    # "BGR;24",


I fixed these errors in #7303, but have since discovered that is not sufficient to get this PR working for those modes. The problem is that those modes are using their lines efficiently, rather than spacing out their data into 4 bytes per pixel.

Pillow/src/libImaging/Storage.c

Lines 149 to 150 in 96d683d

im->pixelsize = 3;

im->linesize = (xsize * 3 + 3) & -4;

Yay295 added 6 commits July 4, 2023 16:57

add tp_richcompare handler for Imaging_Type/ImagingCore

276915e

remove debugging messages

1818032

extract mode "1" to its own test

fedd5da

fix bytes generation

d1d0a87

use rawmode "1;8" when creating mode "1" image from bytes

d3a9226

declare variables before loop

8b23215

radarhere reviewed Jul 5, 2023

View reviewed changes

Tests/test_lib_image.py Outdated Show resolved Hide resolved

use Py_RETURN_* macros for Py_True/Py_False

ae1f9e9

Yay295 force-pushed the image_equals branch from 29d963e to ae1f9e9 Compare July 5, 2023 14:01

radarhere reviewed Jul 6, 2023

View reviewed changes

fix ImagingCore.tp_richcompare test for RGB and YCbCr

bac58a6

also, RGBX technically has 4 channels, so it shouldn't be masked. also, fix the mask for modes with three channels.

Yay295 force-pushed the image_equals branch from 83100a9 to bac58a6 Compare July 6, 2023 15:24

homm reviewed Jul 6, 2023

View reviewed changes

Yay295 added 2 commits July 6, 2023 20:55

add Image.im.set_internal_pixel_bytes() for testing

1713f59

fix bytes for LAB

40597e7

homm reviewed Jul 7, 2023

View reviewed changes

radarhere mentioned this pull request Jul 11, 2023

Set unused bytes to zero when converting to LA/La/PA #7276

Closed

Use single integer color instead of adding set_internal_pixel_bytes()

6bbb4de

radarhere mentioned this pull request Jul 11, 2023

Use single integer color instead of adding set_internal_pixel_bytes() Yay295/Pillow#8

Merged

Merge pull request #8 from radarhere/image_equals

cbbfcb2

Use single integer color instead of adding set_internal_pixel_bytes()

radarhere mentioned this pull request Jul 25, 2023

Support BGR;15, BGR;16 and BGR;24 access, unpacking and putdata #7303

Merged

Yay295 mentioned this pull request Aug 2, 2023

Delegate Image mode and size to ImagingCore #7271

Closed

Merge branch 'main' into image_equals

b053e19

radarhere mentioned this pull request Oct 6, 2023

Test BGR;* modes Yay295/Pillow#10

Closed

radarhere reviewed Oct 6, 2023

View reviewed changes

radarhere reviewed Oct 7, 2023

View reviewed changes

radarhere added 2 commits December 24, 2023 16:55

Merge branch 'main' into image_equals

0974cd8

Merge branch 'main' into image_equals

86583ff

radarhere mentioned this pull request Apr 13, 2024

BGR;15/16 scaling #7970

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tp_richcompare handler for Imaging_Type/ImagingCore #7260

Add tp_richcompare handler for Imaging_Type/ImagingCore #7260

Yay295 commented Jul 5, 2023

radarhere commented Jul 5, 2023

Yay295 commented Jul 5, 2023

radarhere Jul 6, 2023

Yay295 Jul 6, 2023

Yay295 Jul 6, 2023

homm Jul 6, 2023

Yay295 Jul 7, 2023

homm Jul 6, 2023

Yay295 Jul 6, 2023

homm Jul 6, 2023

homm Jul 6, 2023

radarhere Jul 8, 2023

homm Jul 6, 2023

homm Jul 7, 2023

Yay295 Jul 7, 2023

Yay295 Jul 8, 2023

homm Jul 8, 2023 •

edited

Yay295 Jul 8, 2023

Yay295 Jul 9, 2023

Yay295 Jul 9, 2023

homm Jul 9, 2023

Yay295 Jul 9, 2023

radarhere Jul 11, 2023

radarhere Oct 6, 2023

Yay295 Oct 9, 2023

radarhere Oct 7, 2023

Yay295 Oct 9, 2023

radarhere Oct 7, 2023

Yay295 Oct 9, 2023

radarhere Oct 7, 2023

	static void
	rgb2la(UINT8 out, const UINT8 in, int xsize) {
	int x;
	for (x = 0; x < xsize; x++, in += 4, out += 4) {
	/* ITU-R Recommendation 601-2 (assuming nonlinear RGB) */
	out[0] = out[1] = out[2] = L24(in) >> 16;
	out[3] = 255;
	}
	}

	(("RGB", "RGBX"), ("YCbCr", "YCbCrX"), ("HSV", None), ("LAB", None)),
	[("RGB", "RGBX"), ("YCbCr", "YCbCrX"), ("HSV", None), ("LAB", None)],



		@pytest.mark.skip(reason="no way to directly set C bytes from Python")
		@pytest.mark.parametrize("mode", ("LA", "La", "PA"))

	@pytest.mark.parametrize("mode", ("LA", "La", "PA"))
	@pytest.mark.parametrize("mode", ["LA", "La", "PA"])

		assert img_a.im != img_b.im


		@pytest.mark.parametrize("mode", [mode for mode in mode_names_not_bgr if mode != "1"])

		# alternatively, random.randbytes() in Python 3.9
		data = secrets.token_bytes(num_img_bytes)

Add tp_richcompare handler for Imaging_Type/ImagingCore #7260

Are you sure you want to change the base?

Add tp_richcompare handler for Imaging_Type/ImagingCore #7260

Conversation

Yay295 commented Jul 5, 2023

radarhere commented Jul 5, 2023

Yay295 commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homm Jul 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homm Jul 8, 2023 •

edited