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.

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

Conversation

pelson
Copy link
Member

@pelson pelson commented Mar 13, 2024

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 and restore_region in pure Python which is usable by the backend_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 from copy_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 😉

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)

@@ -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))
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.

@anntzer
Copy link
Contributor

anntzer commented Mar 13, 2024

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 class FigureCanvasQTAgg(FigureCanvasAgg, FigureCanvasQT): pass (with everything coming through multiple inheritance) and the (builtin) qtcairo backend can likewise be class FigureCanvasQTCairo(FigureCanvasCairo, FigureCanvasQT): pass.

@pelson
Copy link
Member Author

pelson commented Mar 14, 2024

class FigureCanvasQTAgg(FigureCanvasAgg, FigureCanvasQT): pass (with everything coming through multiple inheritance)

Maybe I'm a bit too ambitious, but I see the existence of FigureCanvasAgg as a failure to define the interface between FigureCanvas and Renderer clearly enough (or maybe I haven't fully grasped the reason for FigureCanvas to be renderer specific - a generic figure canvas can still forward on specific rendering requests, so no API change needed.

I do think that it probably requires a RasterisingRenderer base class though, and I'm not clear what we do when a renderer can be both rasterising and a vector renderer (like your cairo renderer).

Ultimately, in that world the abstractions would be clear, and you would construct canvas = FigureCanvasQt(renderer_type=RendererAgg) (or equivalent at the _Backend level). I'm not sure if we would actually be able to get that far, so your multiple inheritance proposal may be the necessary approach pragmatically.

…xtend the API of the inaccessible PyBufferRegion object (Agg only)
@anntzer
Copy link
Contributor

anntzer commented Mar 14, 2024

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.)

@pelson
Copy link
Member Author

pelson commented Mar 15, 2024

(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).

You need to have a FigureCanvasAgg because you need to be able to render figures in a headless context.

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 print_{fmt} methods. As I understand, the Antigrain library has no such functionality - you just get raw pixel buffers, and therefore technically, I think there should be a generic FigureCanvas which delegates to the appropriate PIL calls (as per the FigureCanvasAgg today). Clearly FigureCanvasCairo would still need to exist such that it can implement the appropriate print_ methods to be able to leverage the functionality of cairo, but it would not be necessary to use the FigureCanvasCairo for graphical backends using cairo for rasterisation.

In summary:

  • A FigureCanvasForRasterRenderer (or some other name) would exist, and it would implement all of the appropriate print_{fmt} methods, with those simply taking raw pixel values and calling PIL (or equivalent)
  • A FigureCanvasQt, FigureCanvasGTK, FigureCanvasTK, etc. which takes raw pixels from a RasterisingRenderer (which the canvas builds from the factory given at __init__), and puts it on the appropriate GUI component, as well as handling events / coord transforms etc. from the specific windowing manager.
  • A FigureCanvasCairo which implements print_svg and other such raster and vector formats, by calling draw on the figure with the right renderer type. This figure canvas is tightly bound to the cairo renderer (i.e. it won't work with an Agg renderer).

With an architecture such as this, Agg, Cairo and other rasterising renderers 😉 can all share the same GUI implementations, from what I can see. Yet it is possible for specific backends to refine the output (headless) formats, if they so exist (e.g. in cairo).

@anntzer
Copy link
Contributor

anntzer commented Mar 18, 2024

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).
Clearly such issues are fixable (by dynamically providing print_foo, as noted above, or by re-designing the format-to-backend dispatch algorithm), I'm just mentioning them there because they'll require some thought.

@@ -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
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)

@anntzer
Copy link
Contributor

anntzer commented Mar 21, 2024

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).

@pelson
Copy link
Member Author

pelson commented Mar 21, 2024

FWIW, I checked with pyqt6 locally and it was fine (as too was pyside and pyqt5).

@anntzer
Copy link
Contributor

anntzer commented Mar 26, 2024

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).

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

Successfully merging this pull request may close these issues.

2 participants