Skip to content

Allow getpixel() to accept a list #7355

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.

Sign up for GitHub

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

Merged
hugovk merged 3 commits into from
Aug 30, 2023
Merged

Allow getpixel() to accept a list #7355

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8c3135
Allow getpixel to accept a list
radarhere Aug 28, 2023
69a81dd
Convert list to tuple in Python instead of C
radarhere Aug 28, 2023
f9f367f
Always cast to a tuple
radarhere Aug 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Tests/test_image_access.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,10 @@ def check(self, mode, expected_color=None):
def test_basic(self, mode):
self.check(mode)

def test_list(self):
im = hopper()
assert im.getpixel([0, 0]) == (20, 20, 70)

@pytest.mark.parametrize("mode", ("I;16", "I;16B"))
@pytest.mark.parametrize(
"expected_color", (2**15 - 1, 2**15, 2**15 + 1, 2**16 - 1)
Expand Down
10 changes: 7 additions & 3 deletions src/_imaging.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1146,11 +1146,15 @@ static inline int
_getxy(PyObject *xy, int *x, int *y) {
PyObject *value;

if (!PyTuple_Check(xy) || PyTuple_GET_SIZE(xy) != 2) {
int tuple = PyTuple_Check(xy);
Copy link
Member

@homm homm Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So many checks here. Isn't a way do something like PyArg_ParseTuple(xy, "ii", x, y)?

Copy link
Member Author

@radarhere radarhere Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly - but if your goal is simplicity, then I should mention that the simplest solution is to implement this change in Python instead of C.

diff --git a/src/PIL/Image.py b/src/PIL/Image.py
index 476ed0122..6ea711b56 100644
--- a/src/PIL/Image.py
+++ b/src/PIL/Image.py
@@ -1565,7 +1565,7 @@ class Image:
         self.load()
         if self.pyaccess:
             return self.pyaccess.getpixel(xy)
-        return self.im.getpixel(xy)
+        return self.im.getpixel(tuple(xy))
 
     def getprojection(self):
         """

Copy link
Member Author

@radarhere radarhere Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original intention here was to make this change close to the source of the error though. If the change is being made higher up, then my argument that it would be awkward to update the error message for just this scenario falls apart, since you could just add a new error message.

Then I feel less sure about this change, as the discourse in this issue doesn't have a clear conclusion, and I'm reminded of #3738, in particular #3738 (comment) and #3738 (comment).

#3738 (comment)

Also, tuples and lists are for slightly different things (immutable/mutable, structure/order). Does that really matter? I think maybe it does: semantically you know a coordinate like (100, 200) can't have a third value; but [100, 200] looks like it could. So I'm tending towards keeping it as is.

Copy link
Member

@hugovk hugovk Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have strong feelings in #3738 (comment), but in the years since then am now tending the other way, maybe we could accept more iterables for these kinds of inputs?

Copy link
Member Author

@radarhere radarhere Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've pushed a commit with a simpler change in Python.

if (
!(tuple && PyTuple_GET_SIZE(xy) == 2) &&
!(PyList_Check(xy) && PyList_GET_SIZE(xy) == 2)
) {
goto badarg;
}

value = PyTuple_GET_ITEM(xy, 0);
value = tuple ? PyTuple_GET_ITEM(xy, 0) : PyList_GET_ITEM(xy, 0);
if (PyLong_Check(value)) {
*x = PyLong_AS_LONG(value);
} else if (PyFloat_Check(value)) {
Expand All @@ -1164,7 +1168,7 @@ _getxy(PyObject *xy, int *x, int *y) {
}
}

value = PyTuple_GET_ITEM(xy, 1);
value = tuple ? PyTuple_GET_ITEM(xy, 1) : PyList_GET_ITEM(xy, 1);
if (PyLong_Check(value)) {
*y = PyLong_AS_LONG(value);
} else if (PyFloat_Check(value)) {
Expand Down