Skip to content

skip QImage leak workaround for PySide2 >= 5.12 #21172

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

Merged
merged 1 commit into from
Sep 26, 2021

Conversation

pijyoi
Copy link
Contributor

@pijyoi pijyoi commented Sep 25, 2021

PR Summary

While testing out Matplotlib 3.5.0b1 with pyqtgraph, pyqtgraph CI pipelines were failing for PySide2 bindings.
pyqtgraph/pyqtgraph#1999

The segfault was due to an old PySide2 QImage leak workaround. This leak was addressed in https://bugreports.qt.io/browse/PYSIDE-140 and versions of PySide2 >= 5.12 no longer need this workaround.

This PR skips the workaround for PySide2 >= 5.12.

The other change of casting to int for PyQt6 avoids the following warning:

  c:\work\venv\allqt\lib\site-packages\matplotlib\backends\backend_qtagg.py:67: DeprecationWarning: an integer is required (got type PyQt6.sip.voidptr).  Implicit conversion to integers using __int__ is deprecated, and may be removed in a future version of Python.

Note: pyqtgraph only exercises the changes made to backend_qtagg.py. The same changes made to backend_qtcairo.py were not tested.

PR Checklist

  • [N/A] Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [N/A ] New features are documented, with examples if plot related.
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

the leak was addressed in PYSIDE-140
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@jklymak
Copy link
Member

jklymak commented Sep 25, 2021

Is it at all possible to add corresponding tests for our test suite?

@pijyoi
Copy link
Contributor Author

pijyoi commented Sep 25, 2021

I am not sure how the test should look like because:

  1. the original code was messing with Python internals
  2. the first Qt for Python release was 5.12, which fixes the leak.
  3. the last version with the bug 5.11 was EOL in 2019

@pijyoi
Copy link
Contributor Author

pijyoi commented Sep 26, 2021

A minimal example demonstrating the issue.

Expected behavior: A few windows are drawn in succession.

  • Works with PyQt5, PyQt6, PySide6 on master (after changing the 1st line import)
  • Works with PySide2 with this PR

Actual behavior with PySide2 on master: Crashes after the first window

Tested on Windows 10, Python 3.8, PySide2 5.15

from PySide2 import QtCore, QtWidgets

import numpy as np
from matplotlib.backends.backend_qt5agg import FigureCanvasQTAgg as FigureCanvas
from matplotlib.figure import Figure

class MatplotlibWidget(QtWidgets.QWidget):
    def __init__(self, size=(5.0, 4.0), dpi=100):
        QtWidgets.QWidget.__init__(self)
        self.figure = Figure(size, dpi=dpi)
        self.canvas = FigureCanvas(self.figure)
        self.canvas.setParent(self)
        self.subplot = self.figure.add_subplot(111)
        self.show()

    def plot(self, x, y):
        self.subplot.plot(x, y)
        self.canvas.draw()

app = QtWidgets.QApplication([])
timer = QtCore.QTimer()
timer.setInterval(1000)
max_cnt = 5
cnt = 0
mw = None

def on_timeout():
    global cnt, mw
    if mw is not None:
        mw.close()

    if cnt >= max_cnt:
        app.exit()
        return

    cnt += 1
    mw = MatplotlibWidget()
    x = np.linspace(0, 1, 1000)
    y = np.sin(2*np.pi*cnt*x)
    mw.plot(x, y)

timer.timeout.connect(on_timeout)
timer.start()

app.exec() if hasattr(app, 'exec') else app.exec_()

@tacaswell
Copy link
Member

We may be able to remove the skip of installing pyside2 on OSX on GHA with this patch

@timhoffm timhoffm merged commit cde68fb into matplotlib:master Sep 26, 2021
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Sep 26, 2021
@pijyoi pijyoi deleted the pyside2_qimage_leak_fixed branch September 26, 2021 22:44
timhoffm added a commit that referenced this pull request Sep 26, 2021
…172-on-v3.5.x

Backport PR #21172 on branch v3.5.x (skip QImage leak workaround for PySide2 >= 5.12)
@pijyoi
Copy link
Contributor Author

pijyoi commented Sep 27, 2021

I was curious to understand why versions <= 3.4.3 didn't cause a segfault.
The following addition in 3.5.0 increases the refcnt of buf in the else branch, thus when the refcnt is forcibly set to 1 later, that's the wrong value.

            if QT_API == "PyQt6":
                from PyQt6 import sip
                ptr = int(sip.voidptr(buf))
            else:
                ptr = buf

As the code is now with this PR merged, PySide2 < 5.12 should (continue to) segfault. I guess that's probably not an issue, given that PySide2 < 5.12 is EOL.

tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Oct 12, 2021
…ixed

skip QImage leak workaround for PySide2 >= 5.12
tacaswell pushed a commit that referenced this pull request Oct 20, 2021
skip QImage leak workaround for PySide2 >= 5.12
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.

4 participants