Skip to content

Simplify the qt backend by using buffers to construct the image to be restored #27913

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

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

Simplify the qt backend by using buffers to construct the image to be restored #27913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7121234
Simplify the qt backend by using buffers to construct the image to be…
pelson Mar 13, 2024
3d1ce96
Fix flake
pelson Mar 13, 2024
391ac40
Ensure that we maintain the eraseRect behaviour to avoid reverting th…
pelson Mar 13, 2024
0258fb7
Apply the proposed improvement to backend_qtagg to avoid needing to e…
pelson Mar 14, 2024
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
31 changes: 12 additions & 19 deletions lib/matplotlib/backends/backend_qtagg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,14 @@
Render to qt from agg.
"""

import ctypes
import numpy as np

from matplotlib.transforms import Bbox

from .qt_compat import QT_API, QtCore, QtGui
from .qt_compat import QtGui
from .backend_agg import FigureCanvasAgg
from .backend_qt import _BackendQT, FigureCanvasQT
from .backend_qt import ( # noqa: F401 # pylint: disable=W0611
FigureManagerQT, NavigationToolbar2QT)
from ..transforms import Bbox


class FigureCanvasQTAgg(FigureCanvasAgg, FigureCanvasQT):
Expand Down Expand Up @@ -47,25 +46,19 @@ def paintEvent(self, event):
right = left + width
# create a buffer using the image bounding box
bbox = Bbox([[left, bottom], [right, top]])
buf = memoryview(self.copy_from_bbox(bbox))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would work to simply do buf = memoryview(np.asarray(self.copy_from_bbox(bbox))) here and not touch anything else? This way from the pure python side you just need something that implements __array__ (a standard numpy interface) and there's no need to introduce a new custom API (get_bounds).

See also #23882 and the linked discourse thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the linked issue, very relevant. FWIW, I'm fully in favour of removing BufferRegion (a non-public API), but not so hot on removing restore_region (a public API). If restore_region is simply redirected to draw_image then fair enough, though I don't especially like the idea of gc=None, nor changing the blitting blending model.

That linked discussion was super interesting - I had assumed that there was nobody using a subset bbox for restore_region - it seems like an obscure API to me. I was going to propose it be removed - I would still do that personally (and make it easy to subset the copied region instead).

Ultimately, I think we share the same objective here... I'm guessing this is about standardising the (rasterising) renderer such that in the future we could have backends which are not renderer specific (e.g. a common implementation of a qt backend for both agg and cairo (and other potential renderers)).

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 took a look at this, and you are right. I like it because it avoids extending the existing inaccessible type. It doesn't solve the fact that we do need to return more than just an image (we need the origin of the image too), but I'm confident we could remove BufferRegion at this point and replace it with a simple Python object (currently a numpy subclass). Ideally, it would not need to be a numpy subclass in the future (containment over inheritance, and all)

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 should say: I don't see a need for memoryview at all. I just used your asarray trick.

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 realised that this PR should also cover qtcairo, which has virtually the same implementation (but uses the pre-multiplied format)

img = np.asarray(self.copy_from_bbox(bbox), dtype=np.uint8)

if QT_API == "PyQt6":
from PyQt6 import sip
ptr = int(sip.voidptr(buf))
else:
ptr = buf
# Clear the widget canvas, to avoid issues as seen in
# https://github.com/matplotlib/matplotlib/issues/13012
painter.eraseRect(rect)

painter.eraseRect(rect) # clear the widget canvas
qimage = QtGui.QImage(ptr, buf.shape[1], buf.shape[0],
QtGui.QImage.Format.Format_RGBA8888)
qimage = QtGui.QImage(
img, img.shape[1], img.shape[0],
QtGui.QImage.Format.Format_RGBA8888,
)
qimage.setDevicePixelRatio(self.device_pixel_ratio)
# set origin using original QT coordinates
origin = QtCore.QPoint(rect.left(), rect.top())
painter.drawImage(origin, qimage)
# 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):
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at #25363 I guess this branch is never exercised anymore indeed, but the qt>=5.12 version bound doesn't seem to be documented anywhere but in that PR changelog note; perhaps it should be mentioned in dependencies.rst (even though it is already a consequence of wheel availability for py3.9)? (@oscargus)

ctypes.c_long.from_address(id(buf)).value = 1
painter.drawImage(rect.topLeft(), qimage)

self._draw_rect_callback(painter)
finally:
Expand Down
6 changes: 6 additions & 0 deletions src/_backend_agg_wrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,12 @@ static PyObject *PyRendererAgg_clear(PyRendererAgg *self, PyObject *args)

static PyObject *PyRendererAgg_copy_from_bbox(PyRendererAgg *self, PyObject *args)
{
// Note that whilst the copy_from_bbox call can technically return an image that
// is of a different rect than was requested, this is not used in the underlying
// backend. In the future, this copy_from_bbox will not return a PyBufferRegion,
// and instead simply return an image (the renderer interface may still expose a
// bbox in the response for convenience, but this doesn't need to be a special
// type at the C++ level).
agg::rect_d bbox;
BufferRegion *reg;
PyObject *regobj;
Expand Down