Speed up some conversions

[akx]

Experiments to speed up the other conversions didn't really yield fruit.

[hugovk]

Rebased following benchmark image update: #9831.

Edit: or not, due to conflict :)

Merging main in instead.

[codspeed-hq]

Merging this PR will improve performance by 76.97%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 4 improved benchmarks
✅ 599 untouched benchmarks
⏩ 335 skipped benchmarks¹

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
⚡	test_convert[1237x811-P-RGBA]	21.4 ms	10.4 ms	×2.1
⚡	test_convert[1237x811-P-RGB]	19.5 ms	10.7 ms	+82.84%
⚡	test_convert[1237x811-PA-RGB]	22 ms	13.2 ms	+66.8%
⚡	test_convert[1237x811-PA-RGBA]	23.9 ms	15.3 ms	+56.38%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing akx:faster-convert-x (f1d30a5) with main (6b5a7db)}

Footnotes

1. 335 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[radarhere]

Suggested change

	#define SET_ALPHA_32(v, alpha) v = ((v & 0xFFFFFF00u) | (alpha))
	#define SET_ALPHA_32(v, alpha) v = ((v & 0xFFFFFF00u) | alpha)

[akx]

Same here - defensive parentheses if alpha expands to an expression.

EDIT: Prior art in e.g. the B16/L16/S16 macros in _imaging.c.

[radarhere]

Suggested change

	#define SET_ALPHA_32(v, alpha) v = ((v & 0x00FFFFFFu) | ((UINT32)(alpha) << 24))
	#define SET_ALPHA_32(v, alpha) v = ((v & 0x00FFFFFFu) | ((UINT32)alpha << 24))

[akx]

Rather not, because alpha could be a more complex expression.

EDIT: Prior art in e.g. the B16/L16/S16 macros in _imaging.c.

[radarhere]

Could you mention here what your thinking was behind the new tests?

[akx]

Could you mention here what your thinking was behind the new tests?

• test_i_to_l_saturation: a naive implementation might just & 0xFF a more-than-8-bit value to make it fit, which would cause wrapping instead of saturation.
• test_p_to_rgb_exact: tests for the behavior of SET_ALPHA_32, which has that little-endian/big-endian trickery. Also codifies the current behavior of how a P mode image with a palette that has alpha gets converted (which is also touched upon in Ensure Image and core palettes are in sync after PA conversion #9834's new test)

[radarhere]

• test_i_to_l_saturation: a naive implementation might just & 0xFF a more-than-8-bit value to make it fit, which would cause wrapping instead of saturation.

You mean on main

diff --git a/src/libImaging/Convert.c b/src/libImaging/Convert.c
index 4a35a9a1f..bab3e7bc0 100644
--- a/src/libImaging/Convert.c
+++ b/src/libImaging/Convert.c
@@ -594,7 +594,7 @@ i2l(UINT8 *out, const UINT8 *in_, int xsize) {
         if (v <= 0) {
             *out = 0;
         } else if (v >= 255) {
-            *out = 255;
+            *out = v & 255;
         } else {
             *out = (UINT8)v;
         }

? That would cause two already existing tests in test_file_gif.py to fail.

• test_p_to_rgb_exact: tests for the behavior of SET_ALPHA_32, which has that little-endian/big-endian trickery.

Guessing at what you think we might get wrong on this branch,

diff --git a/src/libImaging/Convert.c b/src/libImaging/Convert.c
index 3d0653139..72e2a93d6 100644
--- a/src/libImaging/Convert.c
+++ b/src/libImaging/Convert.c
@@ -979,9 +979,9 @@ pa2f(UINT8 *out_, const UINT8 *in, int xsize, ImagingPalette palette) {
 
 // Set the alpha channel of the UINT32 `v` in-place to the given value.
 #ifdef WORDS_BIGENDIAN
-#define SET_ALPHA_32(v, alpha) v = ((v & 0xFFFFFF00u) | (alpha))
+#define SET_ALPHA_32(v, alpha) v = ((v & 0x00FFFFFFu) | (alpha))
 #else
-#define SET_ALPHA_32(v, alpha) v = ((v & 0x00FFFFFFu) | ((UINT32)(alpha) << 24))
+#define SET_ALPHA_32(v, alpha) v = ((v & 0xFFFFFF00u) | ((UINT32)(alpha) << 24))
 #endif
 
 static void

causes 36 tests to fail, including Tests/test_image_convert.py::test_rgba_pa.

I don't naturally expect a performance PR to add more tests, since no functionality should be changed.

You're adding tests not for coverage, basic correctness, or for regression checking, but to ensure that we don't make a hypothetical mistake in the future? I may be wrong, but that sounds like a step too far to me.

[akx]

[...]? That would cause two already existing tests in test_file_gif.py to fail.

Fair enough. Removed.

Guessing at what you think we might get wrong on this branch, [...] causes 36 tests to fail, including Tests/test_image_convert.py::test_rgba_pa.

Thanks for checking! Removed as well. The big-endian architecture Docker CI tests should then scream too. 👍

You're adding tests not for coverage, basic correctness, or for regression checking, but to ensure that we don't make a hypothetical mistake in the future?

Somewhat the other way around, really: to check that I didn't make mistakes within the branch. The new tests were stacked first in the branch's commits, so I could verify that they were green before optimizing things and after optimizing things.

[radarhere]

Suggested change

	*out = (UINT8)(v <= 0 ? 0 : v >= 255 ? 255 : v);
	*out = v <= 0 ? 0 : (v >= 255 ? 255 : (UINT8)v);

Two ideas I'm suggesting here

1. Why not only cast v to UINT8?
2. Rather than a ? b : c ? d : e, use a ? b : (c ? d : e). ...yes, I find it easier to read.