[Impeller] drawAtlas performance improvements #39865
Conversation
jason-simmons
commented
Feb 24, 2023
jason-simmons
commented
- Remove construction of a temporary vector in TRect::MakePointBounds
- Avoid repeated int-to-float conversion of texture_size in AtlasTextureContents::Render
- Cache the AtlasContents bounding box
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| return ComputeBoundingBox().TransformBounds(entity.GetTransformation()); | ||
| } | ||
|
|
||
| Rect AtlasContents::ComputeBoundingBox() const { |
There was a problem hiding this comment.
I suspect that we already know the exact bounding box in some part of the display list, but I took a shortcut here for simplicity.
This change LGTM, but fyi @flar in case we could plumb something through the dispatcher API if it exists
There was a problem hiding this comment.
It would be odd to pass along a bounds to a draw call, but perhaps we could make an Atlas object that computed its bounds on construction and was passed through the DlCanvas/DL ops vector?
There was a problem hiding this comment.
The object could also vet that the array sizes all matched their common expectations...
There was a problem hiding this comment.
Yeah that change sounds reasonable to me. If we are computing the bounds of these or other objects like paths for display list bounds though I think we should be passing them through somehow, or allowing impeller to query the bounds in the dispatcher somehow. Its wasteful if we end up doing nearly the same work twice.
There was a problem hiding this comment.
First, despite the bad naming, the dispatcher is the Impeller object which is receiving the data and so wouldn't know the answer. The object providing the info is the DisplayList and it is stateless (the Dispatch internal implementation has the state, but there is no "object" to represent its current state of dispatch as that is all inside the method call), so it would have no easy way to know which operation you are querying about. There are some other reasons we might want to provide "additional state" with the Dispatch() process, but there is no architectural mechanism to do so at this time.
The DisplayList doing the dispatching doesn't normally store any info that isn't provided in the method call. The API of the data going in and going out is identical for symmetry. Though we might change that in the future, that would require some re-plumbing. DisplayLists do store the bounds of all ops IF you create one with an RTree, but again, that is only used for culling within the implementation of the Dispatch() method call and there is no place to provide the info to the receiver without a lot of rework.
The easiest place to put the bounds would be in an object like we do with Path (via SkPath, but that feature will likely continue on with a DlPath replacement). I think, for many reasons, a DlAtlas object should be created and providing the bounds across the DL recording and playback process would be a nice side benefit. It would not only implicitly carry the local bounds of the operation along to the receiver, but it would also mean that the data consistency only has to be vetted once when the data is received from Dart rather than every step in the process that receives a bunch of raw arrays.
There was a problem hiding this comment.
Dispatcher should probably be renamed StatefulOpSink or DlReceiver or something like that. At some point DL and Impeller should sit down and decide the best way to convey the info and come up with a mechanism tailored to those needs - should it be push or pull, should it be stateful or stateless (one requires Impeller receiver to remember state, another would just keep the state and make it available on each call, but that means detecting changes in state is harder and the implementation of the state changes is baked into DL rather than controlled by Impeller), etc.
jason-simmons
force-pushed
the
iplr_draw_atlas_perf
branch
from
5c1c815 to
773531b
Compare
chinmaygarde
changed the title
* Remove construction of a temporary vector in TRect::MakePointBounds * Avoid repeated int-to-float conversion of texture_size in AtlasTextureContents::Render * Cache the AtlasContents bounding box
jason-simmons
force-pushed
the
iplr_draw_atlas_perf
branch
from
773531b to
10ad7ba
Compare
jason-simmons
added
the
autosubmit
[Impeller] drawAtlas performance improvements
4 participants
