-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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.
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
base: main
Are you sure you want to change the base?
Simplify the qt backend by using buffers to construct the image to be restored #27913
Conversation
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): |
There was a problem hiding this comment.
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)
@@ -47,25 +46,20 @@ 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
Ideally, I guess the goal of separating the rasterizer and the GUI (which would be great) would be attained once the qtagg backend can simply be |
Maybe I'm a bit too ambitious, but I see the existence of I do think that it probably requires a Ultimately, in that world the abstractions would be clear, and you would construct |
…xtend the API of the inaccessible PyBufferRegion object (Agg only)
You need to have a FigureCanvasAgg because you need to be able to render figures in a headless context. If anything, if switching to non-multiple-inheritance, FigureCanvasQTAgg() should be replaced by something like FigureCanvasAgg(gui=QtGui) rather than the other way round. But (even though I agree that MI is messy) I think it would be a quite nasty refactor to get there. (I guess alternatively you could only have a RendererAgg and let figures not always have a canvas but sometimes only have a renderer, but that would be a pretty big refactor too.) |
Agreed. This isn't desirable (and tells us that the renderer and the figure canvas abstractions are slightly wrong).
I've been processing what you said here, and I think I'm not 100% I can understand the reasoning. The canvas is the thing that the renderer draws onto. Rasterising renderers can all draw onto their own internal pixel buffer, and then a standard interface can be used to put such a raster onto the appropriate canvas, right? If I understand the problem is that some renderers can draw to multiple canvas types (not just rasters), and that therefore some will want to implement specific In summary:
With an architecture such as this, |
Your design sounds like a good idea, but there are still some issues with print_foo dispatch. For example, currently if I set the backend to mplcairo.qt (FigureCanvasQt(rasterizer=MplCairoRenderer) in your API?) and call savefig to a svg format, the svg file will be generated with mplcairo. In your proposed design, FigureCanvasQt probably(?) does not expose a print_svg method (unless you dynamically add those based on whatever the underlying renderer provides?) so matplotlib would just fall back to its default svg-able backend (which is the default svg backend). |
@@ -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)) |
There was a problem hiding this comment.
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)
In any case, the patch looks correct to me, but I am not absolutely sure we can drop the various qt backcompat-related codes yet (possibly, I just haven't looked). |
FWIW, I checked with pyqt6 locally and it was fine (as too was pyside and pyqt5). |
Also, as a side point: another idea I had was that we could also try setting up GUI backends that are not based on a raster renderer, but on vector backends, and uses the rasterizer provided by the GUI package: it should be possible to plug a svg output into a QSvgWidget (https://doc.qt.io/qt-6/qsvgwidget.html) or a Tk SVG photoimage (https://core.tcl-lang.org/tips/doc/trunk/tip/507.md), or a pdf output into a QPdfView (https://doc.qt.io/qt-6/qpdfview.html). |
PR summary
Pass a numpy array when constructing a QImage in the qt backend(s), instead of dealing with pointers.
Motivation:
I'm fiddling with a new renderer (more on that at a not-too-distant future date 😉), and it would be super convenient if I was able to implement a
copy_from_bbox
andrestore_region
in pure Python which is usable by thebackend_qtagg
machinery (at least during my prototyping phase). Unfortunately, the way this was done before this change it was impossible to create such a renderer in pure python... but now it is simply using the array interface and one additional method, it is easy for me to do that.Note that in future, it is plausible that we remove
PyBufferRegion
entirely, and simply have a common (non-Agg specific) region representation (containing image & bounding box). Such a standardisation would take us a step closer to backends being separated from renderers (rasterising ones) - this is something that would dramatically simplify the backend machinery, IMO (plus is a motivation for me if I end up producing a new type of rasterising renderer).In an earlier draft of this MR, I had removed the bounding box from the
copy_from_bbox
response (reflecting the fact that the response should be an image for the given bbox), but there are a number of places in which it is convenient to have the opaque response actually represent an image + a bounding box (e.g. in the widgets code). As a result, I pivotted on what we see in this MR - essentially, that means that a renderer is free to return a response fromcopy_from_bbox
which is not actually representative of the given bbox (bigger, smaller, whatever).Obvious reviewers of this PR are @QuLogic and @anntzer - of course, happy for others to review too 😉