Implement draw_markers in the cairo backend. #2370

Merged
merged 2 commits into from Jun 18, 2016

Conversation

Projects
None yet
4 participants
Contributor

benzea commented Sep 3, 2013

This commit increases the speed of marker drawing in the caior backend.
It directly uses cairo paths for this. If the marker is only stroked
(and not filled) then only one drawing operation is used. If it is
filled then each marker is filled and stroked separately.

@benzea benzea Implement draw_markers in the cairo backend.
This commit increases the speed of marker drawing in the caior backend.
It directly uses cairo paths for this. If the marker is only stroked
(and not filled) then only one drawing operation is used. If it is
filled then each marker is filled and stroked separately.
0cc8287
Owner

mdboom commented Sep 3, 2013

Thanks for submitting this. I actually took a crack at something similar in cairo in another project. The problem with this approach is that it creates very long paths which can ultimately be even slower than just drawing one at a time.

The magic recipe I arrived at was to create a "similar surface", draw the stamp to that, and the stamp it multiple times. On a raster backend, this boils down to a fast blit operation. On a vector backend, it turns into an XObject that is "used" multiple times.

This is the gist of what I mean (untested pseudocode):

        surface = self.cr.get_target().create_similar(...)
        subctx = cairo.Context(surface)
        # Draw the stamp to subctx
        surface = subctx.get_target()

        try:
            ctx.set_source_surface(
                surface, x, y)
            ctx.paint()
        finally:
            ctx.restore()
Contributor

benzea commented Sep 3, 2013

Hm, if really long paths are an issue, then we could split it up eg. every 1000 marks. But seriously, if it is an issue, we should probably just head over to the cairo people with a small testcase and get them to fix it upstream.
I don't think it should ever slow down with a lot of separate paths (with one large one I understand that calculating intersections and stuff is not linear with path length).

Using a similar surface has the issue that subpixel offsets are not correctly handled, which means the image would change in appearance and will probably look fuzzy. If some hinting/pixel snapping is done, then using a subsurface and only blitting that would be my preferred solution.

Contributor

benzea commented Sep 3, 2013

OK, thought about it some more, and you are right that very long paths will probably slow down at some point.

That said, I just tried with ~28000 points (had to remove that broken assert in draw_path), and things work fine. Not fast, but considering the amount of vertices it seems OK. Batching it up into 1000 points at a time seems slightly faster in a quick trail.

@benzea benzea cairo backend: Flush out mark drawing every 1000 marks.
This is done to prevent the path that is passed to cairo to become
way too large, as very long paths may impact performance.
c52e357
Owner

mdboom commented Sep 3, 2013

The Agg backend currently rounds to pixel centers when it draws the markers and then blits -- so it's at least a "proven" approach in that context -- but I agree it would change the look, which some may prefer (maybe a config option)? We should also test that large paths don't mess up PDF or SVG files in the standard tools for viewing (poppler, Acrobat Reader for PDF; Chrome, Firefox, Inkscape for SVG) -- though if we're chunking them in 1000 points anyway, it's probably fine.

Also, the fact that filling requires each marker to be drawn individually -- I suspect that's because some markers are not closed paths, is that correct? I wonder if we could detect that the path is closed and not have to do that... but maybe that's overoptimizing what's already a good improvement here.

Contributor

benzea commented Sep 4, 2013

Yeah, one could do the same as the Agg backend there. But I don't think there is a point; I think the main bottleneck now is the path iteration in python.

About the fill. The only difference it makes is if markers are overlapping. In that case drawing all of them at the same time changes the on screen appearance slightly as the stroke is on top of any fill. So if a lot of filled triangles overlap with only slight offsets, then the fill for the "topmost" marker could simply disappear.
Nothing too bad, but I didn't want to break it without any thought.

Here is the current code:
sequential

And drawing the markers at the same time:
same-time

It is not a huge difference, but you can see that sometimes the black outline is on top of fills in the second image. In this example it doesn't look too bad actually.

Owner

mdboom commented Sep 4, 2013

Ok -- I think this is reasonable then. We unfortunately don't have any test coverage for the Cairo backend -- it's definitely something we'd like to do, but we already struggle with how long our test suite takes to run. In the meantime, it would be advantageous to just make sure all of the examples involving markers create the same output as the Agg backend (it won't be identical, but they should be logically the same).

Thanks for the clarification about fill. That makes sense.

Contributor

benzea commented Sep 10, 2013

I guess I can look into creating some testcases; that may take some time, as it is not high on my todo list.

The marker code could also be directly checked against the old cairo implementation.

tacaswell added this to the v1.5.x milestone Aug 18, 2014

Owner

tacaswell commented Feb 19, 2015

@benzea Did you make any progress on adding test?

Contributor

benzea commented Feb 20, 2015

No, I never got around to write a testcase.

Owner

tacaswell commented May 2, 2016

@mdboom I am in favor of just merging this as-is. After 2.5 years I do not think we are going to get tests ;)

tacaswell closed this Jun 18, 2016

tacaswell reopened this Jun 18, 2016

Owner

tacaswell commented Jun 18, 2016

'power cycled' to see if this still passes tests against current master.

@WeatherGod WeatherGod merged commit 2ceaa9c into matplotlib:master Jun 18, 2016

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 70.262%
Details
default The Travis CI build passed
Details

mdboom removed the needs_review label Jun 18, 2016

Member

WeatherGod commented Jun 18, 2016

well, it doesn't seem to break anything...

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