Skip to content
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

[css-paint-api] Consider making Path2D the only way to render paths with PaintRenderingContext2D. #348

Closed
nical opened this issue Jan 17, 2017 · 5 comments

Comments

@nical
Copy link

nical commented Jan 17, 2017

I am proposing that PaintRenderingContext[1] do not implement CanvasPath[2] (calling moveTo/lineTo/etc. directly on the context). The reason is that there is opportunity for optimization when retaining and reusing path objects that is lost with this CanvasPath API.

Rendering paths on the GPU can be done in several ways, all of them involve at some point transferring potentially large amounts of data to the GPU (if path contains a lot of segments), and potentially requires some non-trivial computation to take place (for example when using tessellation like Direc2D and Skia[3]).
I believe that Chrome now records drawing commands and replays them outside of the content process, and Firefox is moving towards an architecture where the path data will have to be transferred to another process. So there is some copying that can be saved here as well if path objects are retained.
Moving painting to the GPU is a trend that browser implementors are all following at the moment. My examples focused on it in part because Canvas2D exposes a drawing model that works well on the CPU but is quite challenging to do efficiently on the GPU. There are probably other venues for optimizations when reusing path objects. On the other hand I can't think of a reason for Path2D to be less efficient than calling lineTo and friends directly on the context.

We should encourage practices that give the browser a better chance to optimize, and as a new API, houdini paint has the opportunity to do that for paths. This would not prevent web authors from re-creating and throwing away the same path objects every time the element is invalidated, but it would still be a step in the right direction, in my opinion.

[1] https://drafts.css-houdini.org/css-paint-api/#2d-rendering-context
[2] https://html.spec.whatwg.org/multipage/scripting.html#canvaspath
[3] Skia seems to implement several ways to render paths on the GPU, tessellation being one of them.

@bfgeek
Copy link
Contributor

bfgeek commented Apr 6, 2017

We'll talk about this at the upcoming F2F in Tokyo.

My personal opinion here is that we shouldn't do this. When creating the PaintRC2D we tried to keep most of the APIs so that existing canvas rendering libraries can be dropped in and work by default. Almost all of these libraries use the CanvasPath api subset.

There are other ways to solve this:

  1. Provide tooling to encourage authors to re-use Path2D objects.
  2. Internally you could keep a "path operation list" which keeps a cache of paths and their operations. If the exact same operations are listed, you could re-use this path.

Additionally we've said previously that we would love a proposal which has a more display-list style API for canvas-2d rendering more generally for the platform :).

If we have a DisplayListCR2D at some point we could add this to the paint api via:

registerPaint('foo', class {
  static contextType = 'display-list';
  paint(ctx, size, styleMap, args) {
    ctx instanceof DisplayListCanvasRenderingContext2D; // true. :)
  }
});

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed #348.

The full IRC log of that discussion
<TabAtkins> Topic: https://github.com/w3c/css-houdini-drafts/issues/348
<TabAtkins> GitHub Topic: https://github.com/w3c/css-houdini-drafts/issues/348
<fantasai> iank_: This was about subsetting paint rendering context2d even more and remove canvas part APIs
<fantasai> iank_: This came from someone at Firefox
<fantasai> iank_: Removing this would force devs to remove new createPath API on the canvas and then stash these paths between frames
<fantasai> iank_: This provdes some optimizations for rendering engines
<fantasai> iank_: M sense is that we don't want to do this
<fantasai> iank_: For the big one api
<fantasai> iank_: and instead have a displayList like context, which we've talked about previously, for a V2
<astearns> s/big/v/
<fantasai> dbaron: My understnading of v1 was that you could implement it on top of a displayList-like context
<fantasai> dbaron: i.e. a canvas that is backe d by a display list instead of backed by a bitmap
<fantasai> iank_: What this allows you to do is cache display lists of 2d path elements
<fantasai> dino: What' he's saying is that if everyone use dthe canvasPath interface, then a GPU renderer could cache the tesselated vertexes and store them on the GPU and would not have to recreate them every time
<fantasai> ChrisL: Another advantage of displayList is that it's much easier ot make an a11y tree
<fantasai> dino: Eithe rway there's still a displayList
<fantasai> dino: If we restrict the API, it could make certain drawing implementations faster
<fantasai> iank_: v1 is still impleementable using a display list, this is how our impl works
<fantasai> iank_: the other main thing is that we're trying to make this thing reusable by canvas AIs, and the ones I looked at heavily make use of the path apis
<fantasai> iank_: If we want a more optimized display list rendering context, that should separate API
<fantasai> smfr: so proposal is that we allow both the old style path rendering to context and the new path 2d stuff?
<fantasai> iank_: Currently that's what we have
<fantasai> iank_: This proposal wants to rmeove the context stuff
<fantasai> smfr: We're going to keep old style and new style path drawing functions on context.
<fantasai> jack: Are ppl going to use 3rd party libraries that are already using these ,and this is why we're keeping them?
<fantasai> jack: And we're assuming they aren't goint to break due to missing APIs?
<fantasai> iank_: ...
<fantasai> iank_: It's pretty reasonable to amke it wor
<fantasai> k
<fantasai> iank_: Could re-discuss with nical in Paris, but should rsolve to keep for now
<fantasai> dino: End result should be able to do it with canvas and paint workers
<fantasai> ...
<fantasai> dino: New version isn't very useful, path objects sit by themselves, don't know their live time
<dbaron> s/live time/lifetime/
<fantasai> dino: Best way is SVG where you explicitly say you're going to reuse that object
<fantasai> jack: I can ping relevant webrender ppl on this bug
<fantasai> jack: Saw yo umentioned some workarounds
<fantasai> jack: Seemed reasonable to me
<dbaron> (Paris meeting plan being https://wiki.csswg.org/planning/paris-2017 )
<fantasai> jack: He also said no reason to cut this API, but no reason to wait until august, could ping ppl now
<fantasai> Rossen: So we don't need a resolution now, can just put on agenda for Paris and keep working in github
<fantasai> Rossen: Doens't require any changes to current spec anyway
<dbaron> (The argument that there are a lot of libraries that will just work out of the box with the old path syntax seems pretty reasonable.)

@metajack
Copy link

Pinging @pcwalton @glennw for feedback here. I believe there is a plan to make a v2 API for a context here that is display list based, and the intention is that v1 API is usable with a display list (and Blink's implementation does this). Is keeping this moveTo/lineTo version going to adversely affect WebRender?

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Consider making Path2D the only way to render paths with PaintRenderingContext2D., and agreed to the following resolutions:

  • RESOLVED: No change
The full IRC log of that discussion <nainar> Topic: Consider making Path2D the only way to render paths with PaintRenderingContext2D.
<nainar> Github: https://github.com//issues/348
<fantasai> iank_: This was brought up by a Mozilla engineer
<fantasai> nical: That would be me
<fantasai> nical: Idea is that the way we want to evolve our rendering code involves a lot of expensive preparation that goes into making path into something we can render efficiently under GPU
<fantasai> nical: If we can do as much caching as possible, the better
<fantasai> nical: In light of doing a new API, if we could privilege things that are easier to optimize, like things that are retained, it's better for us
<fantasai> nical: If we don't expect to need to invalidate custom paint stuff might not be that much of a problem
<fantasai> iank_: Proposal was to drop the lineTo APIs and stuff
<fantasai> nical: Force people to use path objects so we can cache them
<fantasai> dino: Cant' cache them between calls
<fantasai> dino: Not going to keep path object between calls
<fantasai> dino: ...
<fantasai> dino: When draw comes, have to build the path every frame anyway
<fantasai> nical: That wasn't my understanding, thought state was preserved
<fantasai> iank_: Can potentially cache things in class, but can't rely on that cache existing
<fantasai> iank_: Could do ... and use as a cache key
<fantasai> dino: At that point API doesn't matter
<fantasai> dino: You would have an implementation cache, is this path the same as I used last time? If so pull it out of the cache
<fantasai> nical: I have a preference for being explicit about when things expire from cachce
<fantasai> dino: There's no way to specify in the API whether your'e going to keep the path
<fantasai> dino: Don't know whether you will reuse the path, whether need to keep the path around
<fantasai> dino: Unless you use SVG for ...
<fantasai> nical: Thought class registered for paint would be stored between invalidations
<fantasai> nical: So if that's the case then the web author could store the path objects in there
<fantasai> nical: in that case, it's very easy for us to know when things should be flushed out of the cache
<fantasai> nical: because it's the lifetime of the object that dictates that
<fantasai> nical: Have pref towards doing this rather than heuristic
<fantasai> till: That would be the case for this. ?? pressure would ..
<fantasai> dbaron: GC would be too slow for that
<dbaron> s/would/may well/
<fantasai> iank_: Other reason why I prefer not to do that is that existing 2D Path libraries rely on the path to the APIs being on the context
<fantasai> iank_: A lot of them work fine with the paint API as is, wouldn't want to put a barrier in front of authors to use the paint API
<fantasai> dino: tesselation dependent on transformation
<fantasai> dino: Just caching paths would be good, but also want to know the scale, tesselation, etc.
<fantasai> dino: No reason to be so accurate if drawing at a tiny scale
<fantasai> nical: My specific impl goal doesn't worry about scale
<fantasai> dino: When you say store on the class, there's no API for storing
<fantasai> iank_: It's .foo
<fantasai> dino: If we were going to store the path, I think we'd have a specific API for "please store this path"
<fantasai> iank_: I think nical was saying that any new 2d path that's created, it's cached as long as there's a strong reference to it
<fantasai> till: Youd' have to say "I still want to use this object", if not it gets discareded?
<fantasai> till: As long as you say "use this thing from previous frame" then keep it
<fantasai> dino: You do know you've used it because you've drawn it
<fantasai> ...
<fantasai> dino: Coudl do this without being explicit
<fantasai> iank_: Could get pretty far with heuristics
<fantasai> till: But that is easier with path2d
<fantasai> iank_: We'd just build up the path again insternally and compare to previous paths
<fantasai> dino: Conclusion is implementations can go a long way down this path
<fantasai> ...
<fantasai> nical: Don't have a strong objection, just if we reboot an API, should do something easier to optimize
<fantasai> dino: Your request was to remove part of the API, ignoring why
<fantasai> dino: I'd say that's pretty strong feedback
<fantasai> dino: For actual reason, we think we could optimize paths without removing that API
<fantasai> RESOLVED: No change

@bfgeek bfgeek closed this as completed Nov 8, 2017
@bfgeek
Copy link
Contributor

bfgeek commented Nov 8, 2017

Closing as no change needed to the spec.

@bfgeek bfgeek reopened this Nov 8, 2017
@bfgeek bfgeek closed this as completed Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants