-
Notifications
You must be signed in to change notification settings - Fork 6k
raster cache Pictures embedded inside other DisplayLists #29155
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,6 +97,12 @@ void DisplayListLayer::Preroll(PrerollContext* context, | |
| TRACE_EVENT0("flutter", "DisplayListLayer::RasterCache (Preroll)"); | ||
| cache->Prepare(context, disp_list, is_complex_, will_change_, matrix, | ||
| offset_); | ||
| if (!matrix.hasPerspective()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this related to this PR?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes and no. Technically we probably shouldn't be doing any caching under perspective transforms. We've been living with it for a while for picture layers and display list layers and I haven't seen any developers note that it is causing problems, mainly because really who does a lot of perspective transforms in UI code. Without a specific test case I don't want to go modifying that code path just yet. For this new code that test is partially "aiming for more correctness" in that regard, but also in a practical matter the setup for a perspective matrix (working under the naive assumption that it might be worthwhile) would be more involved. So, to simplify the new code by avoiding a more complicated setup that would likely end in tears, I added this check to avoid the recursion and bad output that might ensue.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we move this into a separate PR? It seems like it's worth doing, but it should be landed separately to make bisecting easier, and to avoid needing to revert this change for an unrelated reason.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should make a new Issue for it, but if I remove it from this code, this code itself gets more complicated for a reason that will likely prove unfeasible. I can add to the issue something along the lines of "if it turns out we can cache under perspective transforms, then the following code needs to remove its test and adjust". |
||
| DisplayListCacheDispatcher dispatcher(context, cache); | ||
| dispatcher.transform2DAffine(matrix[0], matrix[1], matrix[2], // | ||
| matrix[3], matrix[4], matrix[5]); | ||
| disp_list->Dispatch(dispatcher); | ||
| } | ||
| } | ||
|
|
||
| SkRect bounds = disp_list->bounds().makeOffset(offset_.x(), offset_.y()); | ||
|
|
@@ -121,7 +127,33 @@ void DisplayListLayer::Paint(PaintContext& context) const { | |
| return; | ||
| } | ||
|
|
||
| display_list()->RenderTo(context.leaf_nodes_canvas); | ||
| DisplayListCanvasDispatcher dispatcher(context.leaf_nodes_canvas, | ||
| context.raster_cache); | ||
| display_list()->Dispatch(dispatcher); | ||
| } | ||
|
|
||
| void DisplayListCacheDispatcher::drawPicture(const sk_sp<SkPicture> picture, | ||
| const SkMatrix* picture_matrix, | ||
| bool render_with_attributes) { | ||
| if (render_with_attributes) { | ||
| return; | ||
| } | ||
| TRACE_EVENT0("flutter", | ||
| "DisplayListLayer::EmbeddedPicture::RasterCache (Preroll)"); | ||
| SkMatrix cache_matrix = matrix(); | ||
| if (picture_matrix != nullptr) { | ||
| cache_matrix.preConcat(*picture_matrix); | ||
| } | ||
| cache_->Prepare(context_, picture.get(), false, false, cache_matrix, | ||
| SkPoint::Make(0, 0)); | ||
| } | ||
|
|
||
| void DisplayListCacheDispatcher::drawDisplayList( | ||
| const sk_sp<DisplayList> display_list) { | ||
| TRACE_EVENT0("flutter", | ||
| "DisplayListLayer::EmbeddedDisplayList::RasterCache (Preroll)"); | ||
| cache_->Prepare(context_, display_list.get(), false, false, matrix(), | ||
| SkPoint::Make(0, 0)); | ||
| } | ||
|
|
||
| } // namespace flutter | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I understanding this correctly? It looks like this is trying to raster cache sub-pictures every time. That will be expensive on memory and processing power...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On what basis do you make that claim?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I see now. I was misreading this and thought you were skipping a call to
Prepare. I was confused about which class/method was doing what. I can see now the logic I was looking for is in https://github.com/flutter/engine/pull/29155/files#diff-3854149b0d2e47aedb825a57720ae5dbd70e7b0ab78a460bb8b31d0c4f8211dcR147There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, this is just setting up a new scan of the DL ops with the same logic applied to the primary DL/Picture.