Inspect arg as Python object, instead of using PyErr_Clear() by radarhere · Pull Request #9726 · python-pillow/Pillow

radarhere · 2026-06-28T13:38:39Z

In _convert_transparent(), instead of failing a call to PyArg_ParseTuple(), calling PyErr_Clear() afterwards and then trying again,

Pillow/src/_imaging.c

Lines 1084 to 1089 in 095bbc3

    
           if (PyArg_ParseTuple(args, "s(iii)", &mode_name, &r, &g, &b)) { 
        
               const ModeID mode = findModeID(mode_name); 
        
               return PyImagingNew(ImagingConvertTransparent(self->image, mode, r, g, b)); 
        
           } 
        
           PyErr_Clear(); 
        
           if (PyArg_ParseTuple(args, "si", &mode_name, &r)) {

an alternative strategy would be to parse the second argument as a PyObject ("O") and then check if it is a tuple or not.

A similar change could be made in _convert_matrix().

hugovk · 2026-06-29T16:35:16Z

+    if (!PyArg_ParseTuple(args, "sO", &mode_name, &matrix)) {
+        return NULL;
+    }
+    if (PyTuple_Size(matrix) == 12) {


This fails with lists:

from PIL import Image rgb = Image.new("RGB", (8, 8), (255, 0, 0)) def transparent(t): rgb.info["transparency"] = t return rgb.convert("RGBA") cases = { "matrix tuple(4)": lambda: rgb.convert("L", matrix=(0.3, 0.59, 0.11, 0.0)), "matrix list(4)": lambda: rgb.convert("L", matrix=[0.3, 0.59, 0.11, 0.0]), "matrix tuple(12)": lambda: rgb.convert("RGB", matrix=tuple(range(12))), "matrix list(12)": lambda: rgb.convert("RGB", matrix=list(range(12))), "transparency tuple": lambda: transparent((255, 0, 0)), "transparency list": lambda: transparent([255, 0, 0]), } for label, fn in cases.items(): try: fn() print(f"OK {label}") except Exception as e: print(f"RAISE {label}: {type(e).__name__}: {e}")

Before:

OK matrix tuple(4) OK matrix list(4) OK matrix tuple(12) OK matrix list(12) OK transparency tuple OK transparency list

After:

OK matrix tuple(4) RAISE matrix list(4): SystemError: <method 'convert_matrix' of 'ImagingCore' objects> returned a result with an exception set OK matrix tuple(12) RAISE matrix list(12): SystemError: /Users/nad/build_macos_installer/installer/variant/binaries/build_source/Objects/tupleobject.c:96: bad argument to internal function OK transparency tuple RAISE transparency list: TypeError: 'list' object cannot be interpreted as an integer

Maybe worth adding a test?

Suggested change

if (PyTuple_Size(matrix) == 12) {

if (PySequence_Size(matrix) == 12) {

matrix should be a tuple?

Pillow/src/PIL/Image.py

Lines 1017 to 1020 in 9c9fece

def convert(

self,

mode: str | None = None,

matrix: tuple[float, ...] | None = None,

Pillow/src/PIL/Image.py

Lines 1054 to 1055 in 9c9fece

:param matrix: An optional conversion matrix. If given, this

should be 4- or 12-tuple containing floating point values.

Should according to type hint and docstring, but not enforced.

There is existing code using lists:

https://github.com/search?q=%2F%5C.convert%5C%28.*%2C+matrix%3D%5C%5B%2F&type=code

Do we want to change the behaviour in this PR?

Ok, I pushed a commit to allow for lists.

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>

codspeed · 2026-06-30T04:12:03Z

Merging this PR will not alter performance

✅ 603 untouched benchmarks
⏩ 335 skipped benchmarks¹

_{Comparing radarhere:clear (ce207da) with main (ab7e054)}

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. ↩

akx · 2026-09-02T12:38:41Z

+    PyObject *matrix;
+    if (!PyArg_ParseTuple(args, "sO", &mode_name, &matrix)) {
+        return NULL;
+    }
+    Py_ssize_t size = PySequence_Size(matrix);


Review note to self: it's OK to not decref matrix, since "O" in ParseTuple doesn't create a strong reference.

akx · 2026-09-02T12:43:24Z

+    } else if (!PyArg_ParseTuple(
+                   args, "s(ffff)", &mode_name, m + 0, m + 1, m + 2, m + 3
+               )) {
+        return NULL;


With this, passing in a sequence of any other length than 4 or 12 always yields

argument 2 must be tuple of length 4, not 5

which is a little confusing (since 12 is valid too):

>>> i.convert("L", matrix=(5,5,5,5,5)) Traceback (most recent call last): File "<python-input-9>", line 1, in <module> i.convert("L", matrix=(5,5,5,5,5)) ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^ File "PIL/Image.py", line 1084, in convert im = self.im.convert_matrix(mode, matrix) TypeError: argument 2 must be tuple of length 4, not 5

Could just do

if (!(size == 4 || size == 12)) { return PyErr_Format(PyExc_TypeError, "matrix must be a sequence of 4 or 12 floats"); }

above instead of the size == -1 check without much extra ado?

From what I see, main also behaves that way. Not saying we can't do this idea, just seems to be a bit of scope creep for this PR.

It does.

The PR description doesn't really tell the reader why the preimage PyErr_Clear() approach was inadvisable, or why this change needs to be done in the first place. In that light, I assume the intent could be "Improve error handling in convert and convert_transparent", in which case improving errors to be more descriptive altogether (in the spirit of #9784) wouldn't be amiss?

I've created #9956

akx · 2026-09-02T12:46:41Z

-    im = hopper("RGB")
+    im = hopper()
+    assert im.mode == "RGB"


Wouldn't im = hopper("RGB") be clear enough about this Hopper's mode?

Ok, I've pushed a commit.

akx · 2026-09-02T13:14:23Z

+    if (PySequence_Check(transparency)) {
+        if (!PyArg_ParseTuple(args, "s(iii)", &mode_name, &r, &g, &b)) {
+            return NULL;
+        }
+    } else if (!PyArg_ParseTuple(args, "si", &mode_name, &r)) {
+        return NULL;
    }
-    return NULL;


The same comment as above about confusing error messages stands here (the allowed values are either a single integer, or a 3-sequence of integers), but it's much harder to reach from userland, since you'd either need to call i.im.convert_transparent() by hand, or have explicitly set some image's .info["transparency"] to an invalid value. Might not be worth addressing.

>>> i.im.convert_transparent("RGBA", "hello") Traceback (most recent call last): File "<python-input-22>", line 1, in <module> i.im.convert_transparent("RGBA", "hello") ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^ TypeError: argument 2 must be 3-item tuple, not str >>> i.im.convert_transparent("RGBA", 123) <ImagingCore object at 0x1034fca50> >>> i.im.convert_transparent("RGBA", [1,2,4]) <ImagingCore object at 0x1034ffeb0>

radarhere force-pushed the clear branch from 58da170 to cdbc1b1 Compare June 28, 2026 13:53

Inspect arg as Python object, instead of using PyErr_Clear()

5f80642

radarhere force-pushed the clear branch from cdbc1b1 to 5f80642 Compare June 28, 2026 14:21

hugovk reviewed Jun 29, 2026

View reviewed changes

radarhere and others added 2 commits June 30, 2026 13:54

Check for PyTuple_Size() failure

d3b91de

Allow for transparency to be a list

a5a7a18

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>

radarhere added 2 commits June 30, 2026 14:54

Added test

a99854d

Accept list as conversion matrix

9197939

radarhere force-pushed the clear branch from fd15012 to 9197939 Compare June 30, 2026 11:00

Merge branch 'main' into clear

47f1a4a

akx reviewed Sep 2, 2026

View reviewed changes

Remove assert

b846010

radarhere mentioned this pull request Sep 4, 2026

Correct error message for matrix length #9956

Merged

Merge branch 'main' into clear

a1115c8

radarhere commented Sep 6, 2026

View reviewed changes

Comment thread Tests/test_image_convert.py Outdated

Remove extra line

ce207da

hugovk merged commit 5eee920 into python-pillow:main Sep 7, 2026
57 of 58 checks passed

radarhere deleted the clear branch September 7, 2026 10:30

	if (PyArg_ParseTuple(args, "s(iii)", &mode_name, &r, &g, &b)) {
	const ModeID mode = findModeID(mode_name);
	return PyImagingNew(ImagingConvertTransparent(self->image, mode, r, g, b));
	}
	PyErr_Clear();
	if (PyArg_ParseTuple(args, "si", &mode_name, &r)) {

	if (PyTuple_Size(matrix) == 12) {
	if (PySequence_Size(matrix) == 12) {

	def convert(
	self,
	mode: str \| None = None,
	matrix: tuple[float, ...] \| None = None,

	:param matrix: An optional conversion matrix. If given, this
	should be 4- or 12-tuple containing floating point values.

Uh oh!

Conversation

radarhere commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codspeed Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radarhere Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

radarhere commented Jun 28, 2026 •

edited

Loading

codspeed Bot commented Jun 30, 2026 •

edited

Loading

radarhere Sep 2, 2026 •

edited

Loading