enthought / pyface

pyface: traits-capable windowing framework
Other
105 stars 55 forks source link

Fix image memory issues #1109

Closed corranwebster closed 2 years ago

corranwebster commented 2 years ago

Fixes #1108 (hopefully).

This stashes a back-reference to a QImage when it is used to create a QPixmap to ensure that the QImage isn't gc-d until we're done with it.

The example from TraitsUI looks like this after the fix: image

Unfortunately I don't see a way to create a regression test.

For a reviewer - to replicate set up a Windows/Python3.6/Pyside2 environment with the main branch of TraitsUI and the main branch of Pyface - you should see corruption or get a segfault. Switching to this branch should fix the problem.

jwiggins commented 2 years ago

How is the QImage being created? Is it using the same strategy to keep a reference to its backing buffer? See https://github.com/python-pillow/Pillow/blob/7.2.x/src/PIL/ImageQt.py#L166-L170

corranwebster commented 2 years ago

How is the QImage being created? Is it using the same strategy to keep a reference to its backing buffer?

For arrays we stash a reference to (a shuffled copy) of the array here: https://github.com/enthought/pyface/blob/8adf2a1db26e57cafafd4d2a884d1deff5f35a3d/pyface/ui/qt4/util/image_helpers.py#L169

For a PIL ImageQt the original Image instance is stored in a trait on the PILImage class: https://github.com/enthought/pyface/blob/8adf2a1db26e57cafafd4d2a884d1deff5f35a3d/pyface/i_pil_image.py#L34

Edit: plus it looks like PIL is also stashing a reference.

jwiggins commented 2 years ago

If this code is only consuming QImage instances that are also created by pyface and have a ref to their underlying buffer, perhaps having the QPixmap reference the buffer would avoid keeping a QImage around unnecessarily?

I'm speculating a little, so don't take this too seriously. I'm 👍 on the proposed solution but only wonder if perhaps the QImage object is made redundant here.

corranwebster commented 2 years ago

If this code is only consuming QImage instances that are also created by pyface and have a ref to their underlying buffer, perhaps having the QPixmap reference the buffer would avoid keeping a QImage around unnecessarily?

There's no way I know of to create a QPixmap directly from a numpy array - it can be created from various file formats directly, but not an array of RGB(A) as far as I can see - this likely because the QPixmap data is device-dependent.

There is a question of whether the ImageEditor should be storing a QPixmap or a QImage - I suspect QPixmap was chosen because that was what was easily available (but maybe for speed/efficiency too).

corranwebster commented 2 years ago

LGTM, but a comment explaining why this code is doing what it's doing would be useful.

Done.