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

Segfault from path editor example with QtAgg #28002

Closed
rcomer opened this issue Apr 1, 2024 · 11 comments · Fixed by #28231
Closed

Segfault from path editor example with QtAgg #28002

rcomer opened this issue Apr 1, 2024 · 11 comments · Fixed by #28231

Comments

@rcomer
Copy link
Member

rcomer commented Apr 1, 2024

I was playing with the path editor example and found that with QtAgg if I resize the window I get

QWidget::repaint: Recursive repaint detected
QWidget::paintEngine: Should no longer be called
QPainter::begin: Paint device returned engine == 0, type: 1
QPainter::end: Painter not active, aborted
Segmentation fault (core dumped)

Removing the blit line from draw as proposed at #11431 fixes this for me. TkAgg works fine whether the blit line is there or not.

I am on Ubuntu 22.04 and tested this with main.

Originally posted by @rcomer in #11431 (comment)

@Impaler343
Copy link
Contributor

Wow! I came across this too right now!

@ksunden
Copy link
Member

ksunden commented Apr 1, 2024

Which Qt bindings are you using? I do see the extra prints, but do not see any seg faults on my machine (I tried with both pyqt and pyside, qt6 and qt5).

From a quick scan of the linked PR, it looks like this is a race condition, so that may explain why I'm not seeing it but you are, so that is extra fun.

@rcomer
Copy link
Member Author

rcomer commented Apr 1, 2024

I have pyqt 5.15.9. I created the environment using the conda yml.

edit: though I created the env in December so perhaps I should make a new one and try again.

edit 2: confirmed same result with a fresh environment.

@Impaler343
Copy link
Contributor

Uninstalling PySide2 somehow fixed this for me

@tacaswell
Copy link
Member

I see this too with pyqt6.

@tacaswell
Copy link
Member

Also confirmed that #27913 does not fix the problem.

@rcomer
Copy link
Member Author

rcomer commented Apr 5, 2024

I see this too with pyqt6.

I don’t know whether to 🎉 or ☹️

@tacaswell
Copy link
Member

On the plus side, I can get rid of the segfault 🎉

On the down side, the figure does not repaint properly on resize until you hit a keyboardkey....

@tacaswell
Copy link
Member

This is what I have been playing with before I sign off for the night:

diff --git a/galleries/examples/event_handling/path_editor.py b/galleries/examples/event_handling/path_editor.py
index d6e84b4540..f08637a188 100644
--- a/galleries/examples/event_handling/path_editor.py
+++ b/galleries/examples/event_handling/path_editor.py
@@ -91,6 +91,7 @@ class PathInteractor:
 
     def on_draw(self, event):
         """Callback for draws."""
+        print('in on draw cb')
         self.background = self.canvas.copy_from_bbox(self.ax.bbox)
         self.ax.draw_artist(self.pathpatch)
         self.ax.draw_artist(self.line)
@@ -120,7 +121,7 @@ class PathInteractor:
             self.line.set_visible(self.showverts)
             if not self.showverts:
                 self._ind = None
-        self.canvas.draw()
+        self.canvas.draw_idle()
 
     def on_mouse_move(self, event):
         """Callback for mouse movements."""
@@ -146,4 +147,5 @@ ax.set_title('drag vertices to update path')
 ax.set_xlim(-3, 4)
 ax.set_ylim(-3, 4)
 
+fig.canvas.draw()
 plt.show()
diff --git a/lib/matplotlib/backend_bases.py b/lib/matplotlib/backend_bases.py
index e90c110c19..2479a07322 100644
--- a/lib/matplotlib/backend_bases.py
+++ b/lib/matplotlib/backend_bases.py
@@ -1806,9 +1806,11 @@ class FigureCanvasBase:
     def _idle_draw_cntx(self):
         self._is_idle_drawing = True
         try:
+            print("enter idle conext")
             yield
         finally:
             self._is_idle_drawing = False
+            print("exit idle conext")
 
     def is_saving(self):
         """
@@ -1892,6 +1894,7 @@ class FigureCanvasBase:
         before saving output to disk. For example computing limits,
         auto-limits, and tick values.
         """
+        print("womp womp")
 
     def draw_idle(self, *args, **kwargs):
         """
diff --git a/lib/matplotlib/backends/backend_agg.py b/lib/matplotlib/backends/backend_agg.py
index 92253c02c1..b1d7ec8f6d 100644
--- a/lib/matplotlib/backends/backend_agg.py
+++ b/lib/matplotlib/backends/backend_agg.py
@@ -379,6 +379,7 @@ class FigureCanvasAgg(FigureCanvasBase):
 
     def draw(self):
         # docstring inherited
+        print("In the real Agg draw")
         self.renderer = self.get_renderer()
         self.renderer.clear()
         # Acquire a lock on the shared font cache.
diff --git a/lib/matplotlib/backends/backend_qt.py b/lib/matplotlib/backends/backend_qt.py
index db593ae77d..c288af2150 100644
--- a/lib/matplotlib/backends/backend_qt.py
+++ b/lib/matplotlib/backends/backend_qt.py
@@ -385,6 +385,7 @@ class FigureCanvasQT(FigureCanvasBase, QtWidgets.QWidget):
             QtWidgets.QWidget.resizeEvent(self, event)
             # emit our resize events
             ResizeEvent("resize_event", self)._process()
+            print("in resize event")
             self.draw_idle()
         finally:
             self._in_resize_event = False
@@ -457,17 +458,13 @@ class FigureCanvasQT(FigureCanvasBase, QtWidgets.QWidget):
             self._event_loop.quit()
 
     def draw(self):
-        """Render the figure, and queue a request for a Qt draw."""
-        # The renderer draw is done here; delaying causes problems with code
-        # that uses the result of the draw() to update plot elements.
-        if self._is_drawing:
-            return
-        with cbook._setattr_cm(self, _is_drawing=True):
-            super().draw()
+        print("hit og draw")
+        self._draw()
         self.update()
 
     def draw_idle(self):
         """Queue redraw of the Agg buffer and request Qt paintEvent."""
+        print("in draw idle")
         # The Agg draw needs to be handled by the same thread Matplotlib
         # modifies the scene graph from. Post Agg draw request to the
         # current event loop in order to ensure thread affinity and to
@@ -475,11 +472,17 @@ class FigureCanvasQT(FigureCanvasBase, QtWidgets.QWidget):
         # TODO: queued signal connection might be safer than singleShot
         if not (getattr(self, '_draw_pending', False) or
                 getattr(self, '_is_drawing', False)):
+            print("   and did something")
             self._draw_pending = True
-            QtCore.QTimer.singleShot(0, self._draw_idle)
+
+            print(f"   {self._draw_pending=}")
+            #QtCore.QTimer.singleShot(0, self._draw_idle)
+        self.update()
+
 
     def blit(self, bbox=None):
         # docstring inherited
+        print("in blit")
         if bbox is None and self.figure:
             bbox = self.figure.bbox  # Blit the entire canvas if bbox is None.
         # repaint uses logical pixels, not physical pixels like the renderer.
@@ -488,14 +491,18 @@ class FigureCanvasQT(FigureCanvasBase, QtWidgets.QWidget):
         self.repaint(l, self.rect().height() - t, w, h)
 
     def _draw_idle(self):
+        print(f"called with {self._draw_pending=}")
+        if not self._draw_pending:
+            return
+        if self._is_idle_drawing:
+            return
+
         with self._idle_draw_cntx():
-            if not self._draw_pending:
-                return
             self._draw_pending = False
             if self.height() < 0 or self.width() < 0:
                 return
             try:
-                self.draw()
+                self._draw()
             except Exception:
                 # Uncaught exceptions are fatal for PyQt5, so catch them.
                 traceback.print_exc()
diff --git a/lib/matplotlib/backends/backend_qtagg.py b/lib/matplotlib/backends/backend_qtagg.py
index 256e50a3d1..afca423520 100644
--- a/lib/matplotlib/backends/backend_qtagg.py
+++ b/lib/matplotlib/backends/backend_qtagg.py
@@ -5,6 +5,7 @@ Render to qt from agg.
 import ctypes
 
 from matplotlib.transforms import Bbox
+from matplotlib import cbook
 
 from .qt_compat import QT_API, QtCore, QtGui
 from .backend_agg import FigureCanvasAgg
@@ -15,6 +16,18 @@ from .backend_qt import (  # noqa: F401 # pylint: disable=W0611
 
 class FigureCanvasQTAgg(FigureCanvasAgg, FigureCanvasQT):
 
+    def _draw(self):
+        """Render the figure, and queue a request for a Qt draw."""
+        # The renderer draw is done here; delaying causes problems with code
+        # that uses the result of the draw() to update plot elements.
+        print("entered real draw")
+        if self._is_drawing:
+            return
+        with cbook._setattr_cm(self, _is_drawing=True):
+            print(type(self).mro())
+            super().draw()
+        print("finished real draw")
+
     def paintEvent(self, event):
         """
         Copy the image from the Agg canvas to the qt.drawable.
@@ -22,11 +35,13 @@ class FigureCanvasQTAgg(FigureCanvasAgg, FigureCanvasQT):
         In Qt, all drawing should be done inside of here when a widget is
         shown onscreen.
         """
+        print("in paintEvent")
         self._draw_idle()  # Only does something if a draw is pending.
 
         # If the canvas does not have a renderer, then give up and wait for
         # FigureCanvasAgg.draw(self) to be called.
         if not hasattr(self, 'renderer'):
+            print("no renderer")
             return
 
         painter = QtGui.QPainter(self)
@@ -61,7 +76,9 @@ class FigureCanvasQTAgg(FigureCanvasAgg, FigureCanvasQT):
             qimage.setDevicePixelRatio(self.device_pixel_ratio)
             # set origin using original QT coordinates
             origin = QtCore.QPoint(rect.left(), rect.top())
+            print("about to paint image")
             painter.drawImage(origin, qimage)
+            print("painted image")
             # Adjust the buf reference count to work around a memory
             # leak bug in QImage under PySide.
             if QT_API == "PySide2" and QtCore.__version_info__ < (5, 12):

I think the current problem is some multiple inheritance issue


class A:
    def foo(self):
        print("A foo")

class B(A):
    def foo(self):
        print("B foo")
        super().foo()


class C(A):
    def bar(self):
        print("C bar")
        super().foo()

class D(B, C):
    ...



d = D()

gives

In [26]: d.bar()
C bar
A foo

but

class C(A):
    def bar(self):
        print("C bar")
        self.foo()

works in the toy.

Something is not right with the draw/draw_idle/their private versions / update but I have not sort it out yet. It looks like we have two generations of attempts to make draw_idle work correctly layered on top of eachother (and neither fully works).

@fomightez
Copy link

Related: For those trying to use the path editor example in current JupyterLab and Jupyter Notebook 7+, see here.

tacaswell added a commit to tacaswell/matplotlib that referenced this issue May 15, 2024
on_draw is triggered via the draw_event callback which is processed after the
rest of the rendering in finished, but before the buffer is painted to the
screen so the `blit` call was at best redundent (causing two copies of the
buffer from our side to the GUI side) and at worst causes segfaults in some
versions of Qt.

Closes matplotlib#28002
@tacaswell
Copy link
Member

Good news! The simplest fix is to remove self.canvas.blit(...) from on_draw. There may still be improvements that can be made to how we interact with the GUI backends, but the immediate problem is to not do useless work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants