In general this seems fine, but please describe in the docs what a backend implementer needs to do (if anything) when implementing (if they need to do so) flush_rasterizing. (In mplcairohttps://github.com/matplotlib/mplcairo/blob/master/lib/mplcairo/base.py#L149 I found that I could just reuse the agg filter machinery and use a do-nothing filter an rasterizing end; what happens now?)

I've run therun-mpl-test-suite.py --tolerance=inf from mplcairo (git master). This PR does not break any existing tests (I get 2 failures that I also see with matplotlib master).

The new new testtest_backend_svg.py::test_count_bitmaps fails, because the PR does not effect how mplcairo handles rasterization, i.e. the test expects the 5 raster objects to be merged to a single<image> tag in the svg file but finds 5<image> tags.

If the backend plugs intoMixedModeRenderer then everything should just work. If not then they can do nothing, and they just wont get the benefit of bitmap merging.

To support it withoutMixedModeRenderer they should move any finalisation fromstop_rasterizing() toflush_rasterizing(), and have some protection so that the flushing process is not called when there have not been any previous rasterization. InMixedModeRenderer there isself._rasterizing which keeps track of if we are inside of any raster elements, and I addedself._rasterizing_unflushed to keep track of if there is there to merger.

Thanks for the explanation. I wonder if it would be possible to achieve this without even needing a new API, by simplynot calling stop_rasterizing at the end of draw(), but rather storing a_prev_artist_was_rasterized somewhere, and then when the next artist comes, if that flag is set, and this next artist is also rasterized, we simply don't call start_rasterizing(), and if the next artist isn't rasterized, we make the call to stop_rasterizing() that was missing.

Thestart_rasterizing() andstop_rasterizing() already have slightly misleading names (and comments) because they take into account the nesting depth, so if the axes is set to rasterize then all its children will be. So when you get to an element with rasterized set to false, you would still want rasterization on if the parent is rasterized. Maybe enter end exit, or push and pop would be better names.

Another way of doing this would be to pass a rasterized flag through the nested draw calls. But this could be a more invasive change.

I've added a changes entry, and improved the comments on effected functions.

Do I need to add an API change doc? I don't think this should break anything that already exists. Possible exception is that the tests now check for merging, which the external backend mplcairo do not do.

Also I spotted#2816 . Currently user specified group ID are not correctly stored in the SVG for rasterized elements. In the case of merged elements, its less clear how that should work. Maybe once a fix for that lands, I could add a patch to suppress merging if the user has set a gid.

This should definitely get an API note because the output files are changed.

I guess a possible issue is someone may have been counting on separate rasters for some reason?

To reiterate my question above: would something along the lines of the following work (untested, but hopefully you get the idea)?

diff --git i/lib/matplotlib/artist.py w/lib/matplotlib/artist.pyindex 41e890352..c3c746dad 100644--- i/lib/matplotlib/artist.py+++ w/lib/matplotlib/artist.py@@ -33,8 +33,14 @@ def allow_rasterization(draw):     @wraps(draw)     def draw_wrapper(artist, renderer, *args, **kwargs):         try:-            if artist.get_rasterized():-                renderer.start_rasterizing()+            if renderer._owes_stop_rasterizing:+                if artist.get_rasterized():+                    pass  # start_r cancels previously owed stop_r.+                else:+                    renderer.stop_rasterizing()+            else:+                if artist.get_rasterized():+                    renderer.start_rasterizing()             if artist.get_agg_filter() is not None:                 renderer.start_filter()@@ -43,7 +49,7 @@ def allow_rasterization(draw):             if artist.get_agg_filter() is not None:                 renderer.stop_filter(artist.get_agg_filter())             if artist.get_rasterized():-                renderer.stop_rasterizing()+                renderer._owes_stop_rasterizing = True      draw_wrapper._supports_rasterization = True     return draw_wrapperdiff --git i/lib/matplotlib/figure.py w/lib/matplotlib/figure.pyindex 8f31d9f69..db1062a3e 100644--- i/lib/matplotlib/figure.py+++ w/lib/matplotlib/figure.py@@ -1693,6 +1693,7 @@ default: 'top'     def draw(self, renderer):         # docstring inherited         self._cachedRenderer = renderer+        renderer._owes_stop_rasterizing = False          # draw the figure bounding box, perhaps none for white figure         if not self.get_visible():@@ -1740,6 +1741,9 @@ default: 'top'         finally:             self.stale = False+        if renderer._owes_stop_rasterizing:+            renderer.stop_rasterizing()+         self.canvas.draw_event(renderer)      def draw_artist(self, a):

Basically, don't emit stop_rasterizing() immediately, but just record that you "owe" one to the renderer, but only emit it if you know the next artist isn't going to restart rasterizing immediately after.

We also need an API change note that there is a new required API on the renders.

I see the utility of this, but am a bit concerned about growing the API and how brittle this seems. I'm mixed (but mostly ok) about the sensitive dependence on draw order. We do have a deterministic draw order, but something just feels off about it (I agree that is very subjective).

I like@anntzer 's proposal because it does not extend the API and we have to muck withFigure.draw either way.

We already have some prior-art in the library of pre-compositing

def_group_rasters(renderer,list_of_artists):raster_group= []defflush():ifraster_group:renderer.start_rasterizing()try:for_inraster_group:_.draw(renderer)finally:renderer.stop_rasterizing()delraster_group[:]forainlist_of_artists:ifa.get_rasterized():raster_group.append(a)continueelse:flush()a.draw(renderer)flush()

I pushed this to 3.4 as I am not sure we can get this settled in the next week, but if it is ready no problem with it going in for 3.3.

To reiterate my question above: would something along the lines of the following work (untested, but hopefully you get the idea)?

Basically, don't emit stop_rasterizing() immediately, but just record that you "owe" one to the renderer, but only emit it if you know the next artist isn't going to restart rasterizing immediately after.

That does seem a bit neater. My original thoughts were that if theself._rasterizing_unflushed (or_owes_stop_rasterizing) was in the renderer then the logic should live there too. But maybe having it inallow_rasterization() works as well. Need to check that it works across nested artists.

We already have some prior-art in the library of pre-compositing

So this proposal would be to move the logic out ofdraw_wrapper and get rid ofallow_rasterization. Then do the merging in the draw calls in Figure and Axes. To allow merging between different nested artist Figure could flatten the whole hierarchy of Artists into a single list.

There are a few draw() functions in the library that are not wrapped, but its not clear to me why.

Just spotted an issue with the current pull. We can't callrenderer.flush_rasterizing() inFigure.draw(), because it is also wrapped byallow_rasterization. InsteadMixedModeRenderer() would need afinalize to do the flushing.

I'm struggling to get the_owes_stop_rasterizing approach to work.

It needs to preventstop_rasterizing() being called before every non raster artist that follows a raster one. So reset torenderer._owes_stop_rasterizing = False after callingrenderer.stop_rasterizing().

If there are nested raster artists, thenstart_rasterizing() can be called multiple times before_owes_stop_rasterizing gets set true. Previous change meansstop_rasterizing() only gets called once.stop_rasterizing() only flushes after a matched number of starts and stops, so output never flushed.

I'll have a think if this can be solved by moving the depth tracking logic out intoallow_rasterization as well.

The original code keep starts and stops matched by the symmetry around draw. My implementation preserves this by moving the flushing to adraw_image() into its own function, that can be called often and only does work when needed.

Moving out depth tracking seems good to me. (There is only one Figure class but multiple backends, not all of which (e.g., mplcairo) rely on MixedModeRenderer, so moving out depth tracking avoids having to reimplement that logic in each backend.)

Latest version doing the raster depth tracking in allow_rasterization. I think this is a simplification of the original code, because the start and stop functions now actually do what they say without needing internal checks.

I've added an API change note.

I tried again with mplcairo, but it seems thatGraphicsContextRendererCairo is not picking up the new attributes fromRendererBase.__init__(). I guess this would need fixing in the cpp class? I have noted what is required in the API change note.

That's fine, I can handle the fix in mplcairo (which is necessary due to not so nice things done in the constructor).
I think the only question left is whether we want this behavior to be togglable. I personally think it's fine to just not bother until someone actually asks for non-merged rasterizations.

That's fine, I can handle the fix in mplcairo (which is necessary due to not so nice things done in the constructor).

Glad that's fixable. I guess you'd want this held of until 3.4 to avoid compatibility issues in the short term.

I think the only question left is whether we want this behavior to be togglable. I personally think it's fine to just not bother until someone actually asks for non-merged rasterizations.

The only reason to turn it off that I have thought of is if you want to attach javascript to elements in the SVG output. I think that is currently hard to do when rasterization is on because it effects the id attribute,#2816 . One solution could be that if the gid is set for an element then it individually rasterzed and gets the id passed through.

Another option is to use theFigure.suppressComposite attribute.

Suppressing grouping both for figures with suppressComposite = True and for artists with a set gid certainly seems reasonable.
I can fix mplcairo whenever, that shouldn't hold up this PR; if it's ready for 3.3 that's good for me.

I think this is close, thank you@samtygier !

I'm not willing to block merging over either of my comments, but would like them to at least be considered and intentionally dismissed ;)

New version. Staying with the monkey patched_raster_depth and_rasterizing seems to work, and to be easy to handle in mplcairo.

I've moved the finalisation into a new decoratorfinalize_rasterization that wrapsFigure.draw. So no need for to add afinalize method to the renderer.

It now usessuppressComposite to prevent merging. Doing it by gid does not make much sense until#2816 is fixed, which I'll have a go at.

I don't understand why the codecov/project/tests check is failing. Most of the changes it lists seem unrelated to this PR.

@tacaswell, as far as I can see this can be merged, but you still had some thoughts....

I'll merge - if@tacaswell has further thoughts we can open a new PR. There is no need for this to languish.