Now that we use PaintOpBuffers instead of SkPictures, it seems we
might be able to get rid of the bounds passed to DrawingRecorder.
These bounds are only used for certain methods in RecordPaintCanvas
(ones that use GetCanvas in record_paint_canvas.cc),
which may not be used by Blink.
The bounds is used to instantiate the SkNoDrawCanvas, which is then used to answer questions later (such as what's the clip, or what's the save count, and what's the CTM).
I could see a few paths here:
(1) Make a RecordPaintCanvas that doesn't use GetCanvas() at all.
I found that SkCanvas had a perf cost to construct (which is why it's lazily constructed). So, it'd probably be a small win to not have one at all. So, one approach would be that all the functions that currently use GetCanvas to return data could be NOTREACHED and return dummy data. I think the PaintCanvas API would be hard to disentangle from everything that uses it in Blink, and so I have a hard time seeing a cleaner design here.
The one weird spot to removing GetCanvas is clipPath which tries to figure out if the clipPath is really a clipRect, but that requires the CTM. If this logic could be pushed up into Blink and verify that no caller is using clipPath when they mean clipRect or clipRRect, then I think it could be plausible to make a RecordPaintCanvas version that does not have an SkNoDrawCanvas internally.
Also a minor weird spot is that we'd probably need to keep track of save count internally instead of asking the canvas to do it.
(2) Pass an empty canvas into RecordPaintCanvas and just don't expect anything useful back from clip-related functions. We could make these assert if recording_bounds_ is empty to make this a more explicit "don't do this". I think this should "just work".
I've an exploratory patch underway pursuing (2) from c#1 above. DrawingDisplayItem::Equals uses bounds to create a bitmap, I think only for under-invalidation checking, presumably we could do it some other way but will need to rework that. Otherwise still sorting through compile error fixes.
PaintArtifact.cpp ComputeChunkBoundsAndOpaqueness also uses DrawingDisplayItem::GetPaintRecordBounds as part of SlimmingPaintV175Enabled. Perhaps we need to keep bounds on DrawingDisplayItem after all.
Actually I have a different plan about VisualRect() and bounds of DrawingDisplayItem for SPv175+ raster invalidation: to remove DisplayItemClient::VisualRect() and use the bounds of DrawingDisplayItem for raster invalidation. The benefit is that raster invalidation will based on actual painting. VisualRect() is a prediction of where we'll paint before painting, and inaccurate prediction causes either over or under invalidation, and the latter corrupts rendering. Raster invalidation based on actual painting will be more accurate and have less chance of under-invalidation bugs.
Can PaintRecord give the bounds of all paint operations?
I don't think PaintRecord can give the bounds currently. I am fine pitching my patch https://chromium-review.googlesource.com/c/chromium/src/+/729322, your logic makes sense, but I am unsure how it fits with enne@ vision for the future of PaintRecord / PaintOpBuffer and friends.
Probably most efficient for you to chat with them briefly.
This issue was filed as output of enne@ and chrishtr@ talking in person.
I saw PaintOp::GetBounds() which returns the bounds of a draw op (except for DrawRecord). I'm not sure if this is enough for our DrawingDisplayItem on SPv175+. I would like to explore this.
Now I think we can go ahead to get rid of the bounds of DrawingRecorder. Actually both the bounds and VisualRect() are predictions before painting, so it's good to get rid of one of them, and better to get rid of both.
PaintRecord can't get the bounds. Any bounds that it currently has is just what it was initialized with. Once Blink is no longer using that (so exciting!), there's really only a few users in ui (who are using that bounds as a way to plumb a rectangle to some bitmap copying code later that wants to know how big the bitmap is) and then it can likely be removed and recordings no longer will need initial sizes.
If you wanted to get the bounds of a recording, you'd have to do all the work you'd expect to do, such as unioning every op that you recorded as well as keeping track of filters and transform matrices and such, which is a good bit of overhead. It seems like something that paint code is more equipped to do than to figure out these bounds later.
We only need to track the bounds of drawing operations in PaintChunks. We calculate bounds affected by transforms, clips and filters in GeometryMapper.
> It seems like something that paint code is more equipped to do than to figure
> out these bounds later.
I think the "something" is just the bounds parameter of DrawingRecorder which we intent to remove :)
Comment 1 by enne@chromium.org
, Oct 3 2017