[Impeller] partial repaint for Impeller/iOS. #40959
Conversation
impeller/renderer/blit_pass.cc
Outdated
| if (!source_region.has_value()) { | ||
| return true; // Nothing to blit. | ||
| } | ||
| // // Clip the destination image. |
There was a problem hiding this comment.
The existing code here is wrong. Need to figure out why.
There was a problem hiding this comment.
This didn't work as expected if the source region isn't the entire size of the source texture. Since I changed the impeller impl to use a translate instead of just a clip we don't hit this case anymore.
| return current_drawable; | ||
| } | ||
|
|
||
| std::unique_ptr<SurfaceMTL> SurfaceMTL::WrapCurrentMetalLayerDrawable( |
There was a problem hiding this comment.
I split this and the above into two functions, so that we can ge the clip rect for DRM before we construct the render target, which is required for conditionally adjusting the strategy.
|
actually ... 500ns isn't really a long time. We could unconditionally do the blit if it were easier. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
|
image changes seem unrelated from canvaskit changes? |
jonahwilliams
requested review from
chinmaygarde,
dnfield and
bdero
and removed request for
dnfield
| } | ||
|
|
||
| void Reset() { ignore_damage_ = true; } |
There was a problem hiding this comment.
I'm wondering if this could simply be void Reset() { damage_ = std::nullopt; }
There was a problem hiding this comment.
Interestingly if I do this rendering breaks. I wonder if the logic for computing damage requires all frames to have up to date damage calculations? Or if perhaps the std::nullopt signal is incorrect for frame damage and it should instead be treated as if the entire screen were dirty?
There was a problem hiding this comment.
I see. It breaks because in that case frame_damage is not properly reported in submit info which breaks tracking of accumulated damage in other two buffers. So the area subsequently repainted in back buffers is smaller than it should be, causing glitches. This is visible in the material progress indicator example when sliding in/out the menu:
glitch.mov
Possible solution might be something like knopp@30c3d61 . Except this doesn't work because it causes ShouldPerformPartialRepaint always return false in subsequent repaints effectively disabling partial repaint completely.
So I'd keep it as it is, maybe add a comment that we need to preserve frame_damage to correctly track accumulated damage for other buffers while at same time reset buffer_damage to disable partial repaint for current buffer.
There was a problem hiding this comment.
Makes sense, thanks for the investigation here. I added a comment, but I think it makes sense to keep frame damage because the damage info is still correct a subsequent frame could use this and decide to do a partial repaint.
…/engine into partial_repaint_impeller_2
| return true; | ||
| } | ||
|
|
||
| auto blit_command_buffer = context->CreateCommandBuffer(); |
There was a problem hiding this comment.
@bdero , if we are doing a final blit for partial repaint, I think I could just hop on the command buffer above and use that? I don't think it actually needs to be empty right, its just a way to ensure that everything else has been scheduled
There was a problem hiding this comment.
I tried this out a bit but ran into some memory corruption issues. not investigating, just moved the blit before the final command buffer creation for now.
| layer_tree.Paint(*this, ignore_raster_cache); | ||
| } | ||
|
|
||
| constexpr float kImpellerRepaintRatio = 0.7f; |
There was a problem hiding this comment.
Consider adding a comment about why this number and how to figure out if changing it is helping or hurting things.
| const std::shared_ptr<Context>& context, | ||
| CAMetalLayer* layer) { | ||
| TRACE_EVENT0("impeller", "SurfaceMTL::WrapCurrentMetalLayerDrawable"); | ||
| TRACE_EVENT0("impeller", "SurfaceMTL::GetMetalDrawableAndValidate"); |
There was a problem hiding this comment.
Are both of these events (this and the one int he new method) helpful?
There was a problem hiding this comment.
Probably not!, I removed one
jonahwilliams
added
the
autosubmit
|
auto label is removed for flutter/engine, pr: 40959, due to - The status or check suite Mac Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
bot
removed
the
autosubmit
jonahwilliams
added
the
autosubmit
…125593) flutter/engine@cf97541...d4ca524 2023-04-27 skia-flutter-autoroll@skia.org Roll Skia from 20a1c61c5597 to d315ab065af3 (5 revisions) (flutter/engine#41535) 2023-04-26 jonahwilliams@google.com Revert "[Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend." (flutter/engine#41533) 2023-04-26 xilaizhang@google.com [codesign] Add pinned xcode version as property to mac android aot engine (flutter/engine#41518) 2023-04-26 bdero@google.com [Impeller] Coerce opaque ColorSourceContents to Source (flutter/engine#41525) 2023-04-26 skia-flutter-autoroll@skia.org Roll Skia from 3fea88565a83 to 20a1c61c5597 (3 revisions) (flutter/engine#41530) 2023-04-26 jonahwilliams@google.com [Impeller] partial repaint for Impeller/iOS. (flutter/engine#40959) 2023-04-26 30870216+gaaclarke@users.noreply.github.com Updated todo with github issue link (flutter/engine#41517) 2023-04-26 skia-flutter-autoroll@skia.org Roll Clang from 20d06c833d83 to 5344d8e10bb7 (flutter/engine#41524) 2023-04-26 jonahwilliams@google.com [Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. (flutter/engine#41374) 2023-04-26 zanderso@users.noreply.github.com Manual clang roll to 5344d8e10bb7d8672d4bfae8adb010465470d51b (flutter/engine#41520) 2023-04-26 zanderso@users.noreply.github.com Download and use the goma client from cipd (flutter/engine#41488) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Implements partial repaint for Impeller.
Fixes flutter/flutter#124526
The new code that manages the damage regions is more or less a copy paste from the existing Skia implementation. Compared to Skia, there are a few differences:
Normally Impeller wants to use the drawable as the resolve texture for the root MSAA pass. Unfortunately this will unconditonally clear that texture. Thus to do a partial repaint, we have to allocate a separate texture to resolve to and then blit into the drawable.
The blit seems to take about 500ns for a full screen on an iPhone 13. That implies that partial repaint is likely not worth doing if the screen is significantly changed. Thus I've added code in compositor_context.cc that computes the percentage of width or height that is part of the dirty rect. Above a threshold of (abitrarily chosen) 70%, we just render as normal. This should mean there is only a very minor hit from performing the diff on screens that are highly changed.
The other special case, is that sometimes we get damage rects that are empty - that is the drawable is already completely up to date with what we want to render. IN that case I shortcircuit all of the impeller code and just present immediately. I previously tried returning without a present but this resulted in Xcode reporting dropped frames. One caveat here is that if you use the XCode frame debugger and attempt to capture a frame where we early present, then it will claim it couldn't capture any command buffers (because we didn't create any).
To facilitate all of this, I added some additonal plumbing so that the impeller surface can get the clip rect from the submit info. Additionally, rather than using a clip rect impeller will translate and then shrink the root surface texture. This reduces memory usage compared to just clippling.