Skip to content
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.

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

Use ImagingCore.ptr instead of ImagingCore.id #8341

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

homm
Copy link
Member

@homm homm commented Sep 1, 2024

Partially suppress #8340

The non-primary extensions in Pillow don't have direct access to Python's _imaging types, such as Imaging_Type and ImagingObject. However, they often still need to interact with objects passed from Python code. Currently, the raw memory pointer to an Imaging object (ImageCore.id) is passed as an integer and then cast to a memory pointer:

static PyObject *
cms_transform_apply(CmsTransformObject *self, PyObject *args) {
    Py_ssize_t idIn, idOut;
    Imaging im, imOut;

    if (!PyArg_ParseTuple(args, "nn:apply", &idIn, &idOut)) {
        return NULL;
    }

    im = (Imaging)idIn;
    imOut = (Imaging)idOut;

    return Py_BuildValue("i", pyCMSdoTransform(im, imOut, self->transform));
}

It's not safe and can lead to segmentation faults. Python has Capsule objects, which provide a safer way to pass pointers to C code. These are exposed through ImageCore.ptr.

This PR deprecates the raw pointer properties ImageCore.id and ImageCore.unsafe_ptrs, and migrates non-primary extensions to interact with Capsule objects.

@homm homm force-pushed the use-ptr branch 5 times, most recently from 1fddcce to 9bd6434 Compare September 2, 2024 01:00
PytestUnraisableExceptionWarning: Exception ignored in: <function PhotoImage.__del__>
AttributeError: 'PhotoImage' object has no attribute '_PhotoImage__photo'
@homm homm force-pushed the use-ptr branch 5 times, most recently from 830a245 to 491e637 Compare September 2, 2024 09:33
@homm
Copy link
Member Author

homm commented Sep 2, 2024

@python-pillow/pillow-team I need help here)

Could anyone debug pypy3.10 on windows? I can't get what's wrong, since as I can see, the capsule object is correctly restored from the pointer and the capsule object itself is alive.

https://github.com/python-pillow/Pillow/actions/runs/10664609198/job/29556218383?pr=8341#step:27:3342

@homm homm force-pushed the use-ptr branch 3 times, most recently from 10db853 to bc83e33 Compare September 2, 2024 10:53
@homm
Copy link
Member Author

homm commented Sep 2, 2024

Get it! In pypy repr(capsule) holds the capsuled pointer itself, not the pointer to the PyObject. And even hex(id(capsule)) is not the pointer to PyObject. Lucky, we can reliably distinguish pypy capsule from cpython capsule by absence of trailing "<>".

https://github.com/pypy/pypy/blob/branches/py3.10/pypy/objspace/std/capsuleobject.py#L39

@radarhere
Copy link
Member

So is the motivation here just to simplify the code?

Also, we don't necessarily need the deprecations - #4532 (comment)

Pillow makes a commitment to stable public interfaces, which are defined at the Python layer. The C interfaces are explicitly internal, and no effort is made to keep them stable even between minor releases, and no support or warning is given when they change. (IIRC, there are/were people using the internal interfaces, but they've always been explicitly on their own with that)

@homm
Copy link
Member Author

homm commented Sep 3, 2024

So is the motivation here just to simplify the code?

Added description to PR.

Also, we don't necessarily need the deprecations

While ImageCore is internal Pillow object, it's still Python API. I believe ImageCore.id could be used by third-party extensions (Github search) and can't be removed in a one day.

docs/deprecations.rst Outdated Show resolved Hide resolved
Co-authored-by: Andrew Murray <[email protected]>
@radarhere radarhere changed the title Use ImageCore.ptr instead of ImageCore.id Use ImagingCore.ptr instead of ImagingCore.id Sep 11, 2024
docs/deprecations.rst Outdated Show resolved Hide resolved
@@ -225,17 +216,11 @@ def __init__(self, image: Image.Image | None = None, **kw: Any) -> None:
self.__mode = image.mode
self.__size = image.size

if _pilbitmap_check():
# fast way (requires the pilbitmap booster patch)
Copy link
Member

@radarhere radarhere Sep 19, 2024

Choose a reason for hiding this comment

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

For the record, the last mention of this patch was removed in Pillow 4.1.0 by #2360

@radarhere
Copy link
Member

In pypy repr(capsule) holds the capsuled pointer itself, not the pointer to the PyObject. And even hex(id(capsule)) is not the pointer to PyObject. Lucky, we can reliably distinguish pypy capsule from cpython capsule by absence of trailing "<>".

What's the advantage of using repr(capsule) in the first place? Why not just pass the capsule itself all the time?

@@ -190,10 +181,10 @@ def paste(self, im: Image.Image) -> None:
if image.isblock() and im.mode == self.__mode:
block = image
else:
block = image.new_block(self.__mode, im.size)
block = Image.core.new_block(self.__mode, im.size)
Copy link
Member

Choose a reason for hiding this comment

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

I imagine this change was just because you didn't like image.new_block(self.__mode, im.size)?

Rather than moving new_block in C, wouldn't it have been better for the C method to access the size on the image object instead of being passed it? Then this call could become image.new_block(self.__mode).

Copy link
Member Author

Choose a reason for hiding this comment

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

I’d like to clarify that the reason for this change is not a matter of personal preference. The new_block method doesn't pertain to an existing image but instead creates a new block based on the passed parameters. As such, it makes more sense for it to reside in the Image.core module rather than being a method of the image object itself.

There is no intent to modify or enhance the behavior; I was simply correcting the method's improper placement.

@homm
Copy link
Member Author

homm commented Sep 22, 2024

What's the advantage of using repr(capsule) in the first place? Why not just pass the capsule itself all the time?

I'm not aware of TK API, but it seems it converts any arguments to strings, at least PyImagingPhotoPut receives arguments as const char **argv. So looks like there is no way to pass the capsule object itself, instead repr(capsule) will be passed any way. I just made this conversion more explicit.

src/PIL/ImageTk.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants