Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873593002:
Force use of and cache render surface. (Closed)
DescriptionForce use of and cache render surface.
Force use of render surface and cache it when there is no damage from
contributing contents. This will allow us to create a caching mechanism
that is simple to maintain and provides the benefits needed to make the
Chrome OS UI fast.
BUG=708513
TEST=manual && new damage_tracker_unittests && surface_aggregator_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2873593002
Cr-Commit-Position: refs/heads/master@{#490671}
Committed: https://chromium.googlesource.com/chromium/src/+/36850738fee2ff9af2f0207353d1d3b00ad3f5df
Patch Set 1 #
Total comments: 17
Patch Set 2 : Fix for patch 1. #
Total comments: 2
Patch Set 3 : Rebase. #Patch Set 4 : Calculate damage of |force_render_surface|. #
Total comments: 24
Patch Set 5 : Rebase mainline. #Patch Set 6 : Try to fix bot_update patch. #Patch Set 7 : Fix for comments in patch 4. #
Total comments: 1
Patch Set 8 : Rebase. #Patch Set 9 : Add cc_unittests. #Patch Set 10 : Fix compile error. #
Total comments: 28
Patch Set 11 : Fix for vmpstr comment in patch 10. #Patch Set 12 : Rebased ToT and merged the conflict. #Patch Set 13 : Rebase again. #Patch Set 14 : Fix test to init has_damage_from_contributing_content_. #
Total comments: 25
Patch Set 15 : Reduce unneeded code in surface aggregator and add more test. #
Total comments: 25
Patch Set 16 : Further reducing the changes in surface aggregator and add more tests. #Patch Set 17 : Rebase to fix compile. #Patch Set 18 : Add more tests to surface_aggregator. #
Total comments: 20
Patch Set 19 : Improve with comment in patch 18. #
Total comments: 5
Patch Set 20 : Preserve cache flag in SwitchToLayer and add tests. #Patch Set 21 : Fix a nit: layer -> Layer. #Patch Set 22 : Rebased to resolve conflict. #Messages
Total messages: 192 (131 generated)
Description was changed from ========== Force use of render surface. Force use of render surface. This will allow us to create a caching mechanism that is simple to maintain and provides the benefits needed to make the Chrome OS UI fast. BUG=708513 TEST=manual ========== to ========== Force use of render surface. Force use of render surface. This will allow us to create a caching mechanism that is simple to maintain and provides the benefits needed to make the Chrome OS UI fast. BUG=708513 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
wutao@chromium.org changed reviewers: + reveman@chromium.org
Hi reveman@, Would you please take a look if I miss any thing. Thanks. Tao
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h#newco... ui/compositor/layer.h:412: void SetForceRenderSurface(bool force); This seems like something the Layer should set when it is animating, not something the client should set permanently. wdyt?
https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h#newco... ui/compositor/layer.h:412: void SetForceRenderSurface(bool force); On 2017/05/09 at 14:56:55, danakj wrote: > This seems like something the Layer should set when it is animating, not something the client should set permanently. wdyt? The main use-case is to enable this when animating but I think we also want manual control. The new background blur UI that is being worked on is one case where I think we want to use this without animating to improve performance. If we want to start with just for animations then that's fine with me.
https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h#newco... ui/compositor/layer.h:412: void SetForceRenderSurface(bool force); On 2017/05/09 15:49:25, reveman wrote: > On 2017/05/09 at 14:56:55, danakj wrote: > > This seems like something the Layer should set when it is animating, not > something the client should set permanently. wdyt? > > The main use-case is to enable this when animating but I think we also want > manual control. The new background blur UI that is being worked on is one case > where I think we want to use this without animating to improve performance. If > we want to start with just for animations then that's fine with me. Background filters are implemented via a separate render pass, so I'd expect that to implicitly get one or the feature wouldn't work. I'd prefer it tied to animations until we have a more clear picture how that'd work, and maybe that can be tied to the feature instead of a bit that might be set an inappropriate times, which would only hurt performance.
On 2017/05/09 15:51:51, danakj wrote: > https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h > File ui/compositor/layer.h (right): > > https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h#newco... > ui/compositor/layer.h:412: void SetForceRenderSurface(bool force); > On 2017/05/09 15:49:25, reveman wrote: > > On 2017/05/09 at 14:56:55, danakj wrote: > > > This seems like something the Layer should set when it is animating, not > > something the client should set permanently. wdyt? > > > > The main use-case is to enable this when animating but I think we also want > > manual control. The new background blur UI that is being worked on is one case > > where I think we want to use this without animating to improve performance. If > > we want to start with just for animations then that's fine with me. > > Background filters are implemented via a separate render pass, so I'd expect > that to implicitly get one or the feature wouldn't work. I'd prefer it tied to > animations until we have a more clear picture how that'd work, and maybe that > can be tied to the feature instead of a bit that might be set an inappropriate > times, which would only hurt performance. Could you please also check it is correct to pass the force render surface flag from ui::Layer to the renderer, especially in surface_aggregator.cc. Thanks.
https://codereview.chromium.org/2873593002/diff/1/cc/output/direct_renderer.cc File cc/output/direct_renderer.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/output/direct_renderer.c... cc/output/direct_renderer.cc:622: if (render_pass->force_render_surface) I'm not sure we can do this without also checking that render pass doesn't have damage. We need to update the texture if it's damaged. If it's not damaged then I think we can now return early here as it's not clipped when this flag is set. We'll need comment here to explain this. https://codereview.chromium.org/2873593002/diff/1/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/output/gl_renderer.cc#ne... cc/output/gl_renderer.cc:992: if (!pass->copy_requests.empty() || pass->force_render_surface) I'm not sure we want to avoid this optimization. It's still useful to skip the intermediate texture when not needed. I think it would be better to leave this as is. Wdyt? https://codereview.chromium.org/2873593002/diff/1/cc/surfaces/display.cc File cc/surfaces/display.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/surfaces/display.cc#newc... cc/surfaces/display.cc:304: bool should_draw = force_render_surface || have_copy_requests || hm, I don't think this should force a draw. copy requests force a draw as that basically a request to take a screenshot but there's still no need to redraw unless something is damaged when using forced render surfaces. https://codereview.chromium.org/2873593002/diff/1/cc/trees/draw_property_util... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:562: is_clipped = false; Please add a comment here to explain why we do this. https://codereview.chromium.org/2873593002/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:840: render_surface->ForceRenderSurface(); is this needed? https://codereview.chromium.org/2873593002/diff/1/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/trees/property_tree.cc#n... cc/trees/property_tree.cc:802: if (node->has_copy_request || node->force_render_surface) I don't think we need this. Do we? https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h#newco... ui/compositor/layer.h:412: void SetForceRenderSurface(bool force); On 2017/05/09 at 15:51:51, danakj wrote: > On 2017/05/09 15:49:25, reveman wrote: > > On 2017/05/09 at 14:56:55, danakj wrote: > > > This seems like something the Layer should set when it is animating, not > > something the client should set permanently. wdyt? > > > > The main use-case is to enable this when animating but I think we also want > > manual control. The new background blur UI that is being worked on is one case > > where I think we want to use this without animating to improve performance. If > > we want to start with just for animations then that's fine with me. > > Background filters are implemented via a separate render pass, so I'd expect that to implicitly get one or the feature wouldn't work. I'd prefer it tied to animations until we have a more clear picture how that'd work, and maybe that can be tied to the feature instead of a bit that might be set an inappropriate times, which would only hurt performance. Animations for now sounds good. There might be some animations where we want to avoid this though (e.g.transform only) so some control for outside ui::Layer might still be needed.
Hi reveman@, PTAL. I removed the early return for the caching so that there is no effect to current code. When we know how to improve the caching we can add it back. Thanks, Tao https://codereview.chromium.org/2873593002/diff/1/cc/output/direct_renderer.cc File cc/output/direct_renderer.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/output/direct_renderer.c... cc/output/direct_renderer.cc:622: if (render_pass->force_render_surface) On 2017/05/10 13:06:17, reveman wrote: > I'm not sure we can do this without also checking that render pass doesn't have > damage. We need to update the texture if it's damaged. If it's not damaged then > I think we can now return early here as it's not clipped when this flag is set. > > We'll need comment here to explain this. Tested in some animation and the render_pass->damage_rect get damaged for most of time. Remove this early return for now. Improve late. https://codereview.chromium.org/2873593002/diff/1/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/output/gl_renderer.cc#ne... cc/output/gl_renderer.cc:992: if (!pass->copy_requests.empty() || pass->force_render_surface) On 2017/05/10 13:06:17, reveman wrote: > I'm not sure we want to avoid this optimization. It's still useful to skip the > intermediate texture when not needed. I think it would be better to leave this > as is. Wdyt? Agree. Removed. https://codereview.chromium.org/2873593002/diff/1/cc/surfaces/display.cc File cc/surfaces/display.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/surfaces/display.cc#newc... cc/surfaces/display.cc:304: bool should_draw = force_render_surface || have_copy_requests || On 2017/05/10 13:06:17, reveman wrote: > hm, I don't think this should force a draw. copy requests force a draw as that > basically a request to take a screenshot but there's still no need to redraw > unless something is damaged when using forced render surfaces. Right. https://codereview.chromium.org/2873593002/diff/1/cc/trees/draw_property_util... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:562: is_clipped = false; On 2017/05/10 13:06:17, reveman wrote: > Please add a comment here to explain why we do this. Done. https://codereview.chromium.org/2873593002/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:840: render_surface->ForceRenderSurface(); On 2017/05/10 13:06:17, reveman wrote: > is this needed? Removed. https://codereview.chromium.org/2873593002/diff/1/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/2873593002/diff/1/cc/trees/property_tree.cc#n... cc/trees/property_tree.cc:802: if (node->has_copy_request || node->force_render_surface) On 2017/05/10 13:06:17, reveman wrote: > I don't think we need this. Do we? Ok. https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/2873593002/diff/1/ui/compositor/layer.h#newco... ui/compositor/layer.h:412: void SetForceRenderSurface(bool force); On 2017/05/10 13:06:17, reveman wrote: > On 2017/05/09 at 15:51:51, danakj wrote: > > On 2017/05/09 15:49:25, reveman wrote: > > > On 2017/05/09 at 14:56:55, danakj wrote: > > > > This seems like something the Layer should set when it is animating, not > > > something the client should set permanently. wdyt? > > > > > > The main use-case is to enable this when animating but I think we also want > > > manual control. The new background blur UI that is being worked on is one > case > > > where I think we want to use this without animating to improve performance. > If > > > we want to start with just for animations then that's fine with me. > > > > Background filters are implemented via a separate render pass, so I'd expect > that to implicitly get one or the feature wouldn't work. I'd prefer it tied to > animations until we have a more clear picture how that'd work, and maybe that > can be tied to the feature instead of a bit that might be set an inappropriate > times, which would only hurt performance. > > Animations for now sounds good. There might be some animations where we want to > avoid this though (e.g.transform only) so some control for outside ui::Layer > might still be needed. Acknowledged.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I would personally like to see a design proposal for how to reuse render pass textures before landing code to force render passes all the way thru the display compositor. https://codereview.chromium.org/2873593002/diff/20001/cc/quads/render_pass.h File cc/quads/render_pass.h (right): https://codereview.chromium.org/2873593002/diff/20001/cc/quads/render_pass.h#... cc/quads/render_pass.h:131: bool force_render_surface = false; Please add documentation for new fields added to structs like this
On 2017/05/11 19:56:50, danakj wrote: > I would personally like to see a design proposal for how to reuse render pass > textures before landing code to force render passes all the way thru the display > compositor. Sent out a doc about how to implement reusing render pass texture. Will write a doc how to apply it in Background blur.
Hi reveman@, Please take another look. Thank you, Tao https://codereview.chromium.org/2873593002/diff/20001/cc/quads/render_pass.h File cc/quads/render_pass.h (right): https://codereview.chromium.org/2873593002/diff/20001/cc/quads/render_pass.h#... cc/quads/render_pass.h:131: bool force_render_surface = false; On 2017/05/11 19:56:50, danakj wrote: > Please add documentation for new fields added to structs like this Done.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
wutao@chromium.org changed reviewers: + jbauman@chromium.org
+jbauman@, could you please check the changes in surface_aggregator and damage_tracker. danakj@, could you please have a look of overall changes. Thanks, Tao
https://codereview.chromium.org/2873593002/diff/60001/cc/output/direct_render... File cc/output/direct_renderer.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/output/direct_render... cc/output/direct_renderer.cc:630: render_pass->has_damage_on_surface_quad)) { What exactly does it mean if there's damage on a surface quad here? There shouldn't be any surface quads given to the direct renderer. https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... File cc/surfaces/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... cc/surfaces/surface_aggregator.cc:92: if (!pass_quad) DCHECK instead of if https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... cc/surfaces/surface_aggregator.cc:102: ref_pass, passes_id_map, updated_passes_id); Could you check the performance on SurfaceAggregatorPerfTest* in cc_perftests? This requires every quad be walked an extra time that we didn't need to do before.
https://codereview.chromium.org/2873593002/diff/60001/cc/output/direct_render... File cc/output/direct_renderer.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/output/direct_render... cc/output/direct_renderer.cc:630: render_pass->has_damage_on_surface_quad)) { On 2017/05/30 22:04:07, jbauman wrote: > What exactly does it mean if there's damage on a surface quad here? > There shouldn't be any surface quads given to the direct renderer. The surface_quad damage is accumulated during surface_aggregation and pass here to indicate if we can reuse the texture. Since we do not have the concept of surface quad (all should be flattened) in the renderer, we might give it different name or grouped with other attribute/damage we will use at last. https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... File cc/surfaces/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... cc/surfaces/surface_aggregator.cc:92: if (!pass_quad) On 2017/05/30 22:04:07, jbauman wrote: > DCHECK instead of if Will do. https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... cc/surfaces/surface_aggregator.cc:102: ref_pass, passes_id_map, updated_passes_id); On 2017/05/30 22:04:07, jbauman wrote: > Could you check the performance on SurfaceAggregatorPerfTest* in cc_perftests? > This requires every quad be walked an extra time that we didn't need to do > before. Comparing the changes with ToT: It is very similar (some times even faster) with +-2% variations when I run with release build: out/Release/cc_perftests --gtest_filter="SurfaceAggregatorPerfTest.*" There is about 10% slower in debug build.
On Tue, May 30, 2017 at 2:24 PM, <wutao@chromium.org> wrote: > +jbauman@, could you please check the changes in surface_aggregator and > damage_tracker. > > danakj@, could you please have a look of overall changes. > Please have weiliangc@ review. > > Thanks, > Tao > > https://codereview.chromium.org/2873593002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
wutao@chromium.org changed reviewers: + weiliangc@chromium.org
+weiliangc@, please review the changes.
On 2017/05/31 15:48:32, danakj wrote: > On Tue, May 30, 2017 at 2:24 PM, <mailto:wutao@chromium.org> wrote: > > > +jbauman@, could you please check the changes in surface_aggregator and > > damage_tracker. > > > > danakj@, could you please have a look of overall changes. > > > > Please have weiliangc@ review. She is OOO this week.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
What's the status here? Are you waiting for danakj@'s review?
On 2017/06/07 17:47:08, reveman wrote: > What's the status here? Are you waiting for danakj@'s review? danakj@ asked weiliangc@ to review this. weiliangc@, would you please have a look. Thanks.
I have more comments in code but two main things: 1) Clip Calculation: - Contributing contents shouldn't be clipped from outside forced render surface. This would look like copy request. This is to make sure texture has full contributing content. - The surface itself should still be clipped by ancestor, so we are clipping the texture when we draw. - Make sure content_rect and drawable_content_rect are not clipped by the clip rect, so a big enough buffer is allocated. 2) Damage Tracking: - No need to track property change on render surface itself and its ancestor. - Optimize to use one bool for tracking damage from contributing content. https://codereview.chromium.org/2873593002/diff/60001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/2873593002/diff/60001/cc/layers/layer.h#newco... cc/layers/layer.h:252: void SetForceRenderSurface(bool force_render_surface); Preferably a different name to distinguish old force_render_surface_for_testing_ behavior with current behavior. Since force_render_surface_for_testing_ doesn't has same assumption about reuse. Also add a comment saying what ui's force_render_surface does. https://codereview.chromium.org/2873593002/diff/60001/cc/quads/render_pass.h File cc/quads/render_pass.h (right): https://codereview.chromium.org/2873593002/diff/60001/cc/quads/render_pass.h#... cc/quads/render_pass.h:137: bool has_property_change_on_contributing_render_surface = false; We should be able to just combine this and has_damage_on_surface_quad bools together, since they both indicate has_damage_from_contributing_content. https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... File cc/surfaces/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... cc/surfaces/surface_aggregator.cc:77: bool UpdateHasDamageOnSurfaceQuadRecursive( Since UpdateHasDamageOnSurfaceQuad is called at the end of CopyPasses, we could push the information about has_damage bottom up, instead of add recursive walk here? https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... cc/surfaces/surface_aggregator.cc:115: UpdateHasDamageOnSurfaceQuadRecursive( We only need the bool has_damage_in_on_surface_quad update when we are in a render pass that is in force_render_pass state, right? https://codereview.chromium.org/2873593002/diff/60001/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:426: has_property_change_on_contributing_render_surface_ |= The flag for not able to use the texture should be damage that comes from contributing layers or contributing surfaces. The damage coming from contributing layers and contributing render surfaces should just be the damage_rect on the render surface. Since the damage rect is later set to output_rect, we should keep track of whether that is empty after damage calculating and before it is set to output rect. https://codereview.chromium.org/2873593002/diff/60001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:427: render_surface->SurfacePropertyChanged() || SurfacePropertyChanged() seems to be ok for reusing texture, since it's the surface's own property and ancestor property change. SUrfacePropertyChanged() is true if there are changes in clip_rect (should be able to apply to texture), content_rect (shouldn't change in the force_render_surface state), or changes from ancestor (should be able to handle in direct renderer when drawing the texture). https://codereview.chromium.org/2873593002/diff/60001/cc/trees/draw_property_... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:568: } else if (render_surface->ForceRenderSurface()) { I don't think this is where "force no clip" should happen. My understanding for the wanted behavior is that: 1. Everything contributing to the render surface shouldn't be clipped by anything outside the render surface to be fully visible/fully rastered. This would be similar to how copy request is handled in the LayerVisibleRect function. Another part of this work is to make sure when we force render surface the drawable_content_space ends up sum of all contributing content without being clipped. 2. The render surface itself, when drawing to the screen, would still be clipped. So for render surface ForceRenderSurface we shouldn't set is_clipped false here. https://codereview.chromium.org/2873593002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:676: const EffectNode* effect_node = Force render surface will look like copy request: we should find the closest ancestor with force render surface, and ignore all clips above that while calculating the visible rect.
https://codereview.chromium.org/2873593002/diff/60001/cc/output/direct_render... File cc/output/direct_renderer.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/output/direct_render... cc/output/direct_renderer.cc:628: if (render_pass->force_render_surface && Could we instead set the damage rectangle to empty in the surface aggregator, and in the direct renderer avoid setting up the renderpass at all if the damage rect is empty?
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Hi weiliangc@ and jbauman@, PTAL. It looks much better now. I want to confirm, should I do the similar thing for the cache_render_surface (new name) to copy_request in the following functions: 1. SkipForInvertibility in layer_tree_host_common.cc 2. Ignore_undamaged in SurfaceAggregator::CopyQuadsToPass in surface_aggregator.cc 3. RenderSurfaceImpl::CalculateClippedAccumulatedContentRect in render_surface_impl.cc Thanks, Tao https://codereview.chromium.org/2873593002/diff/60001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/2873593002/diff/60001/cc/layers/layer.h#newco... cc/layers/layer.h:252: void SetForceRenderSurface(bool force_render_surface); On 2017/06/07 21:19:00, weiliangc wrote: > Preferably a different name to distinguish old force_render_surface_for_testing_ > behavior with current behavior. Since force_render_surface_for_testing_ doesn't > has same assumption about reuse. > > Also add a comment saying what ui's force_render_surface does. As discussed offline, will call the API cache_render_surface for now. There is a comment in the ui::Layer. https://codereview.chromium.org/2873593002/diff/60001/cc/output/direct_render... File cc/output/direct_renderer.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/output/direct_render... cc/output/direct_renderer.cc:628: if (render_pass->force_render_surface && On 2017/06/07 22:05:34, jbauman wrote: > Could we instead set the damage rectangle to empty in the surface aggregator, > and in the direct renderer avoid setting up the renderpass at all if the damage > rect is empty? Sounds good and thanks for your sample cl. The damage_rect also includes damage from the render surface itself. When damage_rect is not empty, the render_pass may not have damage from contributing render surfaces/layers/surface quads. Therefore, in both situations: damage_rect.IsEmpty() or !(render_pass->has_damage_from_contributing_content()), we can reuse the texture. https://codereview.chromium.org/2873593002/diff/60001/cc/quads/render_pass.h File cc/quads/render_pass.h (right): https://codereview.chromium.org/2873593002/diff/60001/cc/quads/render_pass.h#... cc/quads/render_pass.h:137: bool has_property_change_on_contributing_render_surface = false; On 2017/06/07 21:19:00, weiliangc wrote: > We should be able to just combine this and has_damage_on_surface_quad bools > together, since they both indicate has_damage_from_contributing_content. Nice! https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... File cc/surfaces/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... cc/surfaces/surface_aggregator.cc:77: bool UpdateHasDamageOnSurfaceQuadRecursive( On 2017/06/07 21:19:01, weiliangc wrote: > Since UpdateHasDamageOnSurfaceQuad is called at the end of CopyPasses, we could > push the information about has_damage bottom up, instead of add recursive walk > here? Will push the info up in CopyQuadsToPass and HandleSurfaceQuad. https://codereview.chromium.org/2873593002/diff/60001/cc/surfaces/surface_agg... cc/surfaces/surface_aggregator.cc:115: UpdateHasDamageOnSurfaceQuadRecursive( On 2017/06/07 21:19:00, weiliangc wrote: > We only need the bool has_damage_in_on_surface_quad update when we are in a > render pass that is in force_render_pass state, right? Yes. https://codereview.chromium.org/2873593002/diff/60001/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:426: has_property_change_on_contributing_render_surface_ |= On 2017/06/07 21:19:01, weiliangc wrote: > The flag for not able to use the texture should be damage that comes from > contributing layers or contributing surfaces. The damage coming from > contributing layers and contributing render surfaces should just be the > damage_rect on the render surface. Since the damage rect is later set to > output_rect, we should keep track of whether that is empty after damage > calculating and before it is set to output rect. This is a very good point. Do you mean damage_for_this_update_? I can update has_damage_from_contributing_render_surface_or_layer at last in AccumulateDamageFromLayer and AccumulateDamageFromRenderSurface, where all damage from contributing render surfaces or layers must already have been added to damage_for_this_update_. https://codereview.chromium.org/2873593002/diff/60001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:427: render_surface->SurfacePropertyChanged() || On 2017/06/07 21:19:01, weiliangc wrote: > SurfacePropertyChanged() seems to be ok for reusing texture, since it's the > surface's own property and ancestor property change. > > SUrfacePropertyChanged() is true if there are changes in clip_rect (should be > able to apply to texture), content_rect (shouldn't change in the > force_render_surface state), or changes from ancestor (should be able to handle > in direct renderer when drawing the texture). Acknowledged. https://codereview.chromium.org/2873593002/diff/60001/cc/trees/draw_property_... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/2873593002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:568: } else if (render_surface->ForceRenderSurface()) { On 2017/06/07 21:19:01, weiliangc wrote: > I don't think this is where "force no clip" should happen. > > My understanding for the wanted behavior is that: > 1. Everything contributing to the render surface shouldn't be clipped by > anything outside the render surface to be fully visible/fully rastered. This > would be similar to how copy request is handled in the LayerVisibleRect > function. Another part of this work is to make sure when we force render surface > the drawable_content_space ends up sum of all contributing content without being > clipped. > > 2. The render surface itself, when drawing to the screen, would still be > clipped. So for render surface ForceRenderSurface we shouldn't set is_clipped > false here. Done. https://codereview.chromium.org/2873593002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:676: const EffectNode* effect_node = On 2017/06/07 21:19:01, weiliangc wrote: > Force render surface will look like copy request: we should find the closest > ancestor with force render surface, and ignore all clips above that while > calculating the visible rect. As discussed offline, will cache closest_ancestor_with_cache_render_surface_id as we do for closest_ancestor_with_copy_request_id. Then in LayerVisibleRect, choose smaller id to calculate the visible rect.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Please also advise there is the best place to add tests. I will add tests in the next a few patches. For the AccumulateDamageFromLayer, we might need more checks: If the layer is the target of itself, then we did not include this layer's property change. However, if the layer is subtree of the target surface, we should include the layer's property change. Thanks. Tao https://codereview.chromium.org/2873593002/diff/120001/cc/trees/damage_tracke... File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/2873593002/diff/120001/cc/trees/damage_tracke... cc/trees/damage_tracker.cc:394: if (layer_is_new || !layer->LayerPropertyChanged()) This might need special check: If the layer is the target of itself, then we did not include this layer's property change. However, if the layer is subtree of the target surface, we should include the layer's property change.
Hi weiliangc@, friendly ping. Thanks, Tao
Still looking at the code. It's probably best to have unittests close by. If you think this CL is already too big, you can create a new CL that depended on this CL that has unittests. There are three things we want to do in unittests: 1. Make sure draw properties are calculated correctly, such as all layers contributing are fully visible, and the surface is still clipped. This would call functions from draw_properties_util, and will look similar to layer_tree_host_common_unittests. 2. Make sure if we have contributing damage, we don't reuse texture. This will look similar to damage_tracker_unittests. 3. Make sure surface aggregation updates the damage correctly. This should probably be in surface_aggregator_unittests. Look at the files I suggested and let me know if you have questions.
On 2017/06/13 18:00:42, weiliangc wrote: > Still looking at the code. It's probably best to have unittests close by. If you > think this CL is already too big, you can create a new CL that depended on this > CL that has unittests. > > There are three things we want to do in unittests: > 1. Make sure draw properties are calculated correctly, such as all layers > contributing are fully visible, and the surface is still clipped. This would > call functions from draw_properties_util, and will look similar to > layer_tree_host_common_unittests. > > 2. Make sure if we have contributing damage, we don't reuse texture. This will > look similar to damage_tracker_unittests. > > 3. Make sure surface aggregation updates the damage correctly. This should > probably be in surface_aggregator_unittests. > > Look at the files I suggested and let me know if you have questions. Thanks for the suggestion where to add the tests. I will have a look of the files. Might upload another cl for the tests only.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Weiliang, I added cc unittests in this cl: cc/surfaces/surface_aggregator_unittest.cc cc/trees/layer_tree_host_common_unittest.cc cc/trees/damage_tracker_unittest.cc PTAL. Thanks, Tao
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Overall looks good. My biggest question would be whether we want to force render surface if we set cache render surface on something that doesn't need a render surface. https://codereview.chromium.org/2873593002/diff/180001/cc/output/direct_rende... File cc/output/direct_renderer.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/output/direct_rende... cc/output/direct_renderer.cc:635: if (current_frame()->ComputeScissorRectForRenderPass().IsEmpty()) Does this still hold when we are not using partial_swap? https://codereview.chromium.org/2873593002/diff/180001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:10284: // since it owns a surface, and one for the cache_layer. This is saying if we set cache_render_surface we would force a render surface even if we won't normally create a render surface? Creating extra render surface might not be that cheap. Could you provide some metrics of why creating extra render surface for caching improves performance?
wutao@chromium.org changed reviewers: + ochang@chromium.org
Hi ochang@, please review as owner for changes in cc/ipc/* Hi jbauman@, please review as owner for changes in cc/surfaces/* Hi danakj@, please review as owner for changes in cc/layers/*, cc/quads/*, cc/output/*, ui/compositor/* Thanks. Tao
On 2017/07/10 19:19:46, weiliangc wrote: > Overall looks good. My biggest question would be whether we want to force render > surface if we set cache render surface on something that doesn't need a render > surface. I will discuss with reveman@ about this.
On Mon, Jul 10, 2017 at 5:04 PM, <wutao@chromium.org> wrote: > Hi ochang@, please review as owner for changes in cc/ipc/* > > Hi jbauman@, please review as owner for changes in cc/surfaces/* > > Hi danakj@, please review as owner for changes in cc/layers/*, cc/quads/*, > cc/output/*, ui/compositor/* > I won't be able to look this week. +vmpstr to review. > > Thanks. > Tao > > > https://codereview.chromium.org/2873593002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2873593002/diff/180001/cc/output/direct_rende... File cc/output/direct_renderer.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/output/direct_rende... cc/output/direct_renderer.cc:635: if (current_frame()->ComputeScissorRectForRenderPass().IsEmpty()) On 2017/07/10 19:19:46, weiliangc wrote: > Does this still hold when we are not using partial_swap? If the scissor rect is empty then nothing's being drawn to the texture, whether or not partial swap is being used. Whether the scissor rect is empty depends, for the root render pass, on whether partial swap is enabled. https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator.cc:310: dest_pass->has_damage_from_contributing_content |= Would this work if dest pass contains a surface A that contains a surface B that has damage, but surface A has no damage (and is not marked as cached)?
wutao@chromium.org changed reviewers: + vmpstr@chromium.org
+vmpstr@, In an email, danakj@ would like you take a look. weiliangc@ already reviewed the code. Would you please review as owner for changes in cc/layers/*, cc/quads/*, cc/output/*, ui/compositor/* Thanks, Tao https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator.cc:310: dest_pass->has_damage_from_contributing_content |= On 2017/07/11 02:09:44, jbauman wrote: > Would this work if dest pass contains a surface A that contains a surface B that > has damage, but surface A has no damage (and is not marked as cached)? Yes, it should work since SurfaceAggregator::HandleSurfaceQuad is recursive call. Any descendant surface damage should be aggregated. As in the modified test SurfaceAggregatorValidSurfaceTest.AggregateDamageRect, the setup is root_render_pass > parent_frame (surface A) > child_frame (surface B). In the first test in {} block starting from line 1491, the child_frame changed but parent_frame did not change. And on line 1514, the aggregated_pass_list should have damaged surface. https://codereview.chromium.org/2873593002/diff/180001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:10284: // since it owns a surface, and one for the cache_layer. On 2017/07/10 19:19:46, weiliangc wrote: > This is saying if we set cache_render_surface we would force a render surface > even if we won't normally create a render surface? Creating extra render surface > might not be that cheap. Could you provide some metrics of why creating extra > render surface for caching improves performance? We have limited use cases for this caching and have UMA for animation smoothness measurement. We can revisit if this will decrease the performance. reveman@ replied on the bug with this cl: we should be careful to only use this in the UI during animations or in cases where we understand what the side-effects are and have concluded that it's the appropriate solution. The about:flags we added to highlight render surface usage can be used to detect when this is used incorrectly.
Hi Vlad and Oliver, friendly ping. Since this cl is big, please let me know if you have time to review it. Thanks, Tao
cc/ipc lgtm
Mostly naming concerns, I'll defer for actual logic review to weiliangc and jbauman https://codereview.chromium.org/2873593002/diff/180001/cc/ipc/cc_param_traits... File cc/ipc/cc_param_traits_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/ipc/cc_param_traits... cc/ipc/cc_param_traits_unittest.cc:305: arbitrary_bool2, arbitrary_bool2, arbitrary_bool2); There's up to 6 arbitrary bools! maybe use 3 and 4? https://codereview.chromium.org/2873593002/diff/180001/cc/ipc/struct_traits_u... File cc/ipc/struct_traits_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/ipc/struct_traits_u... cc/ipc/struct_traits_unittest.cc:844: has_transparent_background, false, false); Can you please add bools for this right above, so that it's clear what each parameter presents? https://codereview.chromium.org/2873593002/diff/180001/cc/ipc/struct_traits_u... cc/ipc/struct_traits_unittest.cc:961: has_transparent_background, false, false); same here. https://codereview.chromium.org/2873593002/diff/180001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/2873593002/diff/180001/cc/layers/layer_impl.h... cc/layers/layer_impl.h:347: bool LayerPropertyChangedOnly() const; This looks a bit awkward. Maybe LayerPropertyChanged should be renamed to something else first? My concern is that LayerPropertyChanged vs LayerPropertyChangedOnly looks like it would return the same value. Maybe LayerPropertyChanged should be something like LayerNeedsUpdate or LayerIsDamaged. I think it depends on where we use it, but these two functions definitely need to be named more distinctly. Also, could you add comments to both of them; maybe writing the comment would help come up with a better name. https://codereview.chromium.org/2873593002/diff/180001/cc/layers/render_surfa... File cc/layers/render_surface_impl.h (right): https://codereview.chromium.org/2873593002/diff/180001/cc/layers/render_surfa... cc/layers/render_surface_impl.h:157: bool CacheRenderSurface() const; nit: ShouldCacheRenderSurface? Normally bool getters should have a verb leading the name, and the verb here is "Cache" implying that this is an instruction to render surface impl to cache the surface (which isn't true) https://codereview.chromium.org/2873593002/diff/180001/cc/quads/render_pass.h File cc/quads/render_pass.h (right): https://codereview.chromium.org/2873593002/diff/180001/cc/quads/render_pass.h... cc/quads/render_pass.h:136: // A cumulated damage from contributing render surface or layer or surface nit: Accumulated. Also, this comment isn't really saying what the bool holds. Maybe say "Indicates whether there is accumulated damage ..." https://codereview.chromium.org/2873593002/diff/180001/cc/quads/render_pass_u... File cc/quads/render_pass_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/quads/render_pass_u... cc/quads/render_pass_unittest.cc:86: has_transparent_background, false, false); nit: same comments as before, please make named variables for these https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator.h (right): https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator.h:190: base::flat_map<RenderPassId, RenderPass*> passes_id_map; nit: id_to_render_pass_map_ https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator.h:203: base::flat_set<RenderPassId> cache_render_surface_passes_; nit: cached https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator.h:219: bool has_cache_render_surfaces_; nit: cached https://codereview.chromium.org/2873593002/diff/180001/cc/trees/effect_node.h File cc/trees/effect_node.h (right): https://codereview.chromium.org/2873593002/diff/180001/cc/trees/effect_node.h... cc/trees/effect_node.h:87: int closest_ancestor_with_cache_render_surface_id; nit: s/cache/cached/?
Hi vmpstr@, thanks. I renamed based on your suggestions. Hi weiliangc@, would be nice if you can have a final review. Hi reveman@, could you please also have another look before we land this. Thanks, Tao https://codereview.chromium.org/2873593002/diff/180001/cc/ipc/cc_param_traits... File cc/ipc/cc_param_traits_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/ipc/cc_param_traits... cc/ipc/cc_param_traits_unittest.cc:305: arbitrary_bool2, arbitrary_bool2, arbitrary_bool2); On 2017/07/13 18:01:01, vmpstr wrote: > There's up to 6 arbitrary bools! maybe use 3 and 4? Done. https://codereview.chromium.org/2873593002/diff/180001/cc/ipc/struct_traits_u... File cc/ipc/struct_traits_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/ipc/struct_traits_u... cc/ipc/struct_traits_unittest.cc:844: has_transparent_background, false, false); On 2017/07/13 18:01:02, vmpstr wrote: > Can you please add bools for this right above, so that it's clear what each > parameter presents? Done. https://codereview.chromium.org/2873593002/diff/180001/cc/ipc/struct_traits_u... cc/ipc/struct_traits_unittest.cc:961: has_transparent_background, false, false); On 2017/07/13 18:01:02, vmpstr wrote: > same here. Done. https://codereview.chromium.org/2873593002/diff/180001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/2873593002/diff/180001/cc/layers/layer_impl.h... cc/layers/layer_impl.h:347: bool LayerPropertyChangedOnly() const; On 2017/07/13 18:01:02, vmpstr wrote: > This looks a bit awkward. Maybe LayerPropertyChanged should be renamed to > something else first? > > My concern is that LayerPropertyChanged vs LayerPropertyChangedOnly looks like > it would return the same value. > > Maybe LayerPropertyChanged should be something like LayerNeedsUpdate or > LayerIsDamaged. I think it depends on where we use it, but these two functions > definitely need to be named more distinctly. > > Also, could you add comments to both of them; maybe writing the comment would > help come up with a better name. I am a little worry that there are 45 references to LayerPropertyChanged(). Rename it will make even more changes to this patch. I could do it in separate cl though. I would like to add comments and rename my new function, such as LayerPropertyChangedNotFromPropertyTrees() for now? https://codereview.chromium.org/2873593002/diff/180001/cc/layers/render_surfa... File cc/layers/render_surface_impl.h (right): https://codereview.chromium.org/2873593002/diff/180001/cc/layers/render_surfa... cc/layers/render_surface_impl.h:157: bool CacheRenderSurface() const; On 2017/07/13 18:01:02, vmpstr wrote: > nit: ShouldCacheRenderSurface? Normally bool getters should have a verb leading > the name, and the verb here is "Cache" implying that this is an instruction to > render surface impl to cache the surface (which isn't true) Done. https://codereview.chromium.org/2873593002/diff/180001/cc/quads/render_pass.h File cc/quads/render_pass.h (right): https://codereview.chromium.org/2873593002/diff/180001/cc/quads/render_pass.h... cc/quads/render_pass.h:136: // A cumulated damage from contributing render surface or layer or surface On 2017/07/13 18:01:03, vmpstr wrote: > nit: Accumulated. Also, this comment isn't really saying what the bool holds. > Maybe say "Indicates whether there is accumulated damage ..." Done. https://codereview.chromium.org/2873593002/diff/180001/cc/quads/render_pass_u... File cc/quads/render_pass_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/180001/cc/quads/render_pass_u... cc/quads/render_pass_unittest.cc:86: has_transparent_background, false, false); On 2017/07/13 18:01:03, vmpstr wrote: > nit: same comments as before, please make named variables for these Done. https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... File cc/surfaces/surface_aggregator.h (right): https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator.h:190: base::flat_map<RenderPassId, RenderPass*> passes_id_map; On 2017/07/13 18:01:03, vmpstr wrote: > nit: id_to_render_pass_map_ Done. https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator.h:203: base::flat_set<RenderPassId> cache_render_surface_passes_; On 2017/07/13 18:01:03, vmpstr wrote: > nit: cached Done. https://codereview.chromium.org/2873593002/diff/180001/cc/surfaces/surface_ag... cc/surfaces/surface_aggregator.h:219: bool has_cache_render_surfaces_; On 2017/07/13 18:01:03, vmpstr wrote: > nit: cached Done. https://codereview.chromium.org/2873593002/diff/180001/cc/trees/effect_node.h File cc/trees/effect_node.h (right): https://codereview.chromium.org/2873593002/diff/180001/cc/trees/effect_node.h... cc/trees/effect_node.h:87: int closest_ancestor_with_cache_render_surface_id; On 2017/07/13 18:01:03, vmpstr wrote: > nit: s/cache/cached/? Done.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nice! this lgtm from a high level pov. please go ahead and land this after getting appropriate lgtms. https://codereview.chromium.org/2873593002/diff/260001/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/2873593002/diff/260001/ui/compositor/layer.h#... ui/compositor/layer.h:417: // surface even if layer is invisible is not a problem. This explains it perfectly. Thanks!
Description was changed from ========== Force use of render surface. Force use of render surface. This will allow us to create a caching mechanism that is simple to maintain and provides the benefits needed to make the Chrome OS UI fast. BUG=708513 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Force use of and cache render surface. Force use of render surface and cache it when there is no damage from contributing contents. This will allow us to create a caching mechanism that is simple to maintain and provides the benefits needed to make the Chrome OS UI fast. BUG=708513 TEST=manual && new cc_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Thanks, waiting for lgtms from the following owners: weiliangc@ for overall review of cc/*. jbauman@ for surface_aggregator.* danakj@ for ui/compositor/layer.* jbauman@ is ooo till 7-19, so maybe danakj@ could help to review the surface_aggregator before that? On 2017/07/14 19:36:27, reveman wrote: > nice! this lgtm from a high level pov. please go ahead and land this after > getting appropriate lgtms.
Sorry, I'll review this tomorrow.
Can you verify that you've covered all the branches and corner cases added in surface aggregator in tests? Because the test changes look very minimal, considering the number of interactions going on with damage tracking, saving damage between frames, damage from children, etc... It's very hard to understand changes in SurfaceAggregator for me, sorry. So I have a lot of questions. If there's a way to break the changes in there up into smaller bits somehow by staging this CL in a few pieces, that might be helpful too. https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... File components/viz/service/display/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:270: source.has_damage_from_contributing_content); Are you expecting CompositorFrames from clients to report has_damage_from_contributing_content on renderpasses, or is it only meant to be computed here? Which side is responsible for what parts of the computation? https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:287: if (!copy_request_passes_.count(remapped_pass_id) && Can you help me understand why in CopyQuadsToPass we check if there exists /any/ (copy requests|cached output) to ignore damage, but here we compute damage anyway unless /this/ renderpass is (being copied|being cached). https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:288: !cached_render_surface_passes_.count(remapped_pass_id) && How is this different than checking source.cache_render_surface ? https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:309: // Check if the surface_quad has damage. Can this comment explain why this damage check is different from the above computation of the copy_pass->damage_rect? https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:312: !DamageRectForSurface(surface, last_pass, last_pass.output_rect) This is different from the use of this function in PrewalkTree, it also looks at child surfaces. Is that intentional? Why doesn't this use a damage result computed there so we don't have to do it again? https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:427: // render surface may be ignored. I think you mean to say that you want what's in the cache to be the full content here? https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:471: if (aggregate_only_damaged_ && !has_copy_requests_ && Can you help me understand why this doesn't compute damage_rect_in_quad_space when there is any cached renderpass, but we do when there's moved pixel passes, even tho in both cases we're going to aggregate all the quads anyway? https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:495: iter->second->has_damage_from_contributing_content; Is this just what HandleSurfaceDrawQuad() computed above on L463? Should it just get that from there? Or is this including other information, from where? https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:567: source.has_damage_from_contributing_content); Same questions here as in HandleSurfaceDrawQuad https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:587: } It doesn't compute the damage from contributing things here, is that intentional? Do copied currently-offscreen surfaces not need to compute that? Why? I think the root doesn't need to because we can't cache that one, is that right? https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:885: PropagatePasses(©_request_passes_); granted the name comes from what was there before but what is this propogating? I don't think it describes what it's doing at all.. it's more like collecting or something. Can you improve this? https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... File components/viz/service/display/surface_aggregator.h (right): https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.h:140: void PropagatePasses(base::flat_set<cc::RenderPassId>* passes); Please give the variable more context in the name than just the type
Hi Dana, PTAL. As you pointed out, some codes are redundant, so I removed them. Thanks, Tao https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... File components/viz/service/display/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:270: source.has_damage_from_contributing_content); On 2017/07/19 22:52:55, danakj wrote: > Are you expecting CompositorFrames from clients to report > has_damage_from_contributing_content on renderpasses, or is it only meant to be > computed here? Which side is responsible for what parts of the computation? There are 3 contributing_contents: 1. contributing render surface, 2. contributing layer, 3. surface qaud. 1 and 2 are handled by damage_tracker. Clients should report has_damage_from_contributing_content of RenderPasses. Here HandleSurfaceQuad is to calculate damage from surface qaud. https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:287: if (!copy_request_passes_.count(remapped_pass_id) && On 2017/07/19 22:52:55, danakj wrote: > Can you help me understand why in CopyQuadsToPass we check if there exists /any/ > (copy requests|cached output) to ignore damage, but here we compute damage > anyway unless /this/ renderpass is (being copied|being cached). Here we also ignore damage and do not recalculate damage if there is a copy request. It is if (!copy_request...) jbauman@ might be the best person to answer this. IIRC, non-root-renderpass' damage_rect can be used in partial-swap. However if there is a copy request, the copy_pass' damage_rect will be the output_rect. As indicated by the comment in line 425, "If the current frame has copy requests, then aggregate the entire thing, as otherwise parts of the copy requests may be ignored." https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:288: !cached_render_surface_passes_.count(remapped_pass_id) && On 2017/07/19 22:52:54, danakj wrote: > How is this different than checking source.cache_render_surface ? I think you are right. So I do not need to modify the function "PropogatePass" to collecting this info. That would remove many changes in this cl. https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:309: // Check if the surface_quad has damage. On 2017/07/19 22:52:54, danakj wrote: > Can this comment explain why this damage check is different from the above > computation of the copy_pass->damage_rect? Will change the comment. The damage here is only from surface quads. The copy_pass->damage_rect has all the damages on the top level surface including all the damage from damage_tracker. https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:312: !DamageRectForSurface(surface, last_pass, last_pass.output_rect) On 2017/07/19 22:52:54, danakj wrote: > This is different from the use of this function in PrewalkTree, it also looks at > child surfaces. Is that intentional? > > Why doesn't this use a damage result computed there so we don't have to do it > again? This will check if all the surface_quads (including child surfaces) has damage because HandleSurfaceQuad is a recursive call by calling CopyQuadsToPass in it. This is different than the damage_rect calculated in PrewalkTree, which also includes all the damage in the top level surface, containing damage from damage_tracker. https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:427: // render surface may be ignored. On 2017/07/19 22:52:55, danakj wrote: > I think you mean to say that you want what's in the cache to be the full content > here? Yes. Changed accordingly. https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:471: if (aggregate_only_damaged_ && !has_copy_requests_ && On 2017/07/19 22:52:54, danakj wrote: > Can you help me understand why this doesn't compute damage_rect_in_quad_space > when there is any cached renderpass, but we do when there's moved pixel passes, > even tho in both cases we're going to aggregate all the quads anyway? Looks like the final result is same, but we might do extra calculate if there is moved pixel in the pass in the current code. When there's moved pixel in the pass, the ignore_undamaged if false, then the damage_rect_in_quad_space_valid is not used. If there is no moved pixel in the pass, then we can ignore the out-side quad damage. https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:495: iter->second->has_damage_from_contributing_content; On 2017/07/19 22:52:54, danakj wrote: > Is this just what HandleSurfaceDrawQuad() computed above on L463? Should it just > get that from there? Or is this including other information, from where? They are in different code path. Here is to handle cc::DrawQuad::RENDER_PASS. I should have already checked this damage either in HandleSurfaceDrawQuad or render surface in damage_tracker. Removed related code. https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:567: source.has_damage_from_contributing_content); On 2017/07/19 22:52:54, danakj wrote: > Same questions here as in HandleSurfaceDrawQuad Please see the reply to the other comment. https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:587: } On 2017/07/19 22:52:54, danakj wrote: > It doesn't compute the damage from contributing things here, is that > intentional? Do copied currently-offscreen surfaces not need to compute that? > Why? > > I think the root doesn't need to because we can't cache that one, is that right? All damage from contributing contents should be handled: 1. copy_pass->SetAll(...) 2. HandleSurfaceQuad. Are offscreen surfaces handled here? https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.cc:885: PropagatePasses(©_request_passes_); On 2017/07/19 22:52:55, danakj wrote: > granted the name comes from what was there before but what is this propogating? > I don't think it describes what it's doing at all.. it's more like collecting or > something. Can you improve this? 1. I do not need to modify this function. 2. I think the name could be fine. It is propagating all the dependent passes to the /copy_request_passes_/. https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... File components/viz/service/display/surface_aggregator.h (right): https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... components/viz/service/display/surface_aggregator.h:140: void PropagatePasses(base::flat_set<cc::RenderPassId>* passes); On 2017/07/19 22:52:55, danakj wrote: > Please give the variable more context in the name than just the type Related to the other comments. Do not need this modification any way.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... File components/viz/service/display/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:288: !copy_pass->cache_render_surface && I feel that each of these 3 conditions needs a comment explaining why it is here. I tried to understand why this one is here below.. lmk. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:309: // CopyQuadsToPass in it. This is different than the damage_rect calculated in I'm not clear why this is talking about Prewalk tree, maybe there's some indirection here which isn't clear. We're computing damage_rect in this function too, on line 295. So I was wondering what's different from here to there. IIUC the difference is: - Above we are reducing (setting?) the damage on the copy_pass to be the computed damage for the root aggregated frame being swapped. Since offscreen copy requests are not being swapped, they are not part of the root aggregated damage, so in that case we don't know what is damaged there and leave it fully damaged. You added cache_render_surface there also for unrelated reasons, which are that you want to ensure the cached render surface is fully drawn even if not all of it is damaged? I don't follow the logic for why that's happening - it seems to imply that we have a render surface already that is now partially damaged, but we want to redraw the pixels that are already present.. Or can you have a render surface appear the first time and not all of it is damaged? If so, how would that happen? - Below here, we are determining (recursively) if any aggregated frames for SurfaceDrawQuads have damage. RenderPassDrawQuads are not checked because we expect the client submitting the RenderPass to do that check. If this is write can you turn more of this into comments? https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:311: // containing damage from damage_tracker. "containing damage specified with the CompositorFrame" or something. damage tracker is part of layer compositor which this code does not know about and could change arbitrarily. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:428: // aggregate the entire thing, because we want full content. I think this comment is saying what we want as the outcome but not why we need to do it. For copy requests, it's because of how we aggregate damage for only surfaces that will be swapped, right? For cached render surfaces, is it possible to have a surface not fully damaged when it first appears, or is it something else? DamageRectForSurface() seems to make a surface fully damaged the first time it appears... do you have a unit test showing why we need has_cached_render_surfaces_ here, and that we get an incorrect cache result without it? Put another way, for each if-condition added in this patch like this, if you remove it, does some test fail? I wanted to just mention also that it's unfortunate IMO that this patch is phrasing things in terms of render surfaces (which are a mostly obsolete term from layer compositors, they don't even exist anymore except or RenderSurfaceImpl), instead of render targets and render passes. Especially for types and variables that are used from SurfaceAggregator for which "Surface" has a completely different meaning causing confusion, and "RenderSurface" is not something it could every know about. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:567: !copy_pass->cache_render_surface && I also think each of these needs the same comments explaining why the condition is here, matching HandleSurfaceQuad() comments if that's the case. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... File components/viz/service/display/surface_aggregator_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2462: TEST_F(SurfaceAggregatorValidSurfaceTest, HasDamageFromSurfaces) { Can you split this into smaller test cases? https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2499: ->has_damage_from_contributing_content); This is related to my questions in the aggregator, but why does this need to be true on the first frame? Should the new object already be covered by the root aggregated damage? If this was false, what would end up in the cache? Is it possible to build expectations around that? https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2564: ->has_damage_from_contributing_content); This is related to my questions in the aggregator, but why does this need to be true on the first frame? Should the new object already be covered by the root aggregated damage? https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2614: ->has_damage_from_contributing_content); This is related to my questions in the aggregator, but why does this need to be true on the first frame? Should the removed object already be covered by the root aggregated damage?
https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... File components/viz/service/display/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:313: dest_pass->has_damage_from_contributing_content |= Suppose we have a Surface 1 with damage, and a surface 2: Pass A) SurfaceDrawQuad referencing 1 Pass B) RenderPassDrawQuad referencing A If B has cache_render_surface, shouldn't has_damage_from_contributing_content be set on it? Also, what do we do with has_damage_from_contributing_content? I don't see it used anywhere https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:428: // aggregate the entire thing, because we want full content. On 2017/07/25 18:55:02, danakj wrote: > I think this comment is saying what we want as the outcome but not why we need > to do it. For copy requests, it's because of how we aggregate damage for only > surfaces that will be swapped, right? For cached render surfaces, is it possible > to have a surface not fully damaged when it first appears, or is it something > else? DamageRectForSurface() seems to make a surface fully damaged the first > time it appears... do you have a unit test showing why we need > has_cached_render_surfaces_ here, and that we get an incorrect cache result > without it? > I think if the renderpass is partially offscreen initially then it might not necessarily be completely damaged then, because we start with global screen-space damage rect and work from there to find the damage rect for the renderpass.
Hi danakj@ and jbauman@, Thanks for the comments. I removed/bring back some codes and add a new test for jbauman@'s corner case. PTAL. Tao https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... File components/viz/service/display/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:288: !copy_pass->cache_render_surface && On 2017/07/25 18:55:02, danakj wrote: > I feel that each of these 3 conditions needs a comment explaining why it is > here. I tried to understand why this one is here below.. lmk. Do not need this change anymore. But I try to explain why it is here IIUC. copy_pass's damage_rect by default is the source.output_rect. See line 265: copy_pass->SetAll(...source.output_rect...). When there is no copy_request or moved pixel_pass (I am guessing moved-pixel-pass also needs full output damage because it can read pixel outside the render pass?), we try to get the clipped damage rect on the render pass space by line 295: copy_pass->damage_rect.Intersect(damage_rect_in_render_pass_space); This will be used in DirectRenderer::DrawingFrame::ComputeScissorRectForRenderPass. If there are partial_swap, then we allow non-root-render-pass damage_rect to be smaller than the output_rect. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:309: // CopyQuadsToPass in it. This is different than the damage_rect calculated in On 2017/07/25 18:55:03, danakj wrote: > I'm not clear why this is talking about Prewalk tree, maybe there's some > indirection here which isn't clear. We're computing damage_rect in this function > too, on line 295. So I was wondering what's different from here to there. > I should mention Prewalk here. I did just because try to address your comments in previous patch. > IIUC the difference is: > - Above we are reducing (setting?) the damage on the copy_pass to be the > computed damage for the root aggregated frame being swapped. Since offscreen > copy requests are not being swapped, they are not part of the root aggregated > damage, so in that case we don't know what is damaged there and leave it fully > damaged. Make sense to me. > You added cache_render_surface there also for unrelated reasons, which are that > you want to ensure the cached render surface is fully drawn even if not all of > it is damaged? I don't follow the logic for why that's happening - it seems to > imply that we have a render surface already that is now partially damaged, but > we want to redraw the pixels that are already present.. Or can you have a render > surface appear the first time and not all of it is damaged? If so, how would > that happen? You could be totally right here. Thanks! I will test on removing this. Sounds like the damage_rect only affect drawing/swap, not affect caching. > > - Below here, we are determining (recursively) if any aggregated frames for > SurfaceDrawQuads have damage. RenderPassDrawQuads are not checked because we > expect the client submitting the RenderPass to do that check. > > If this is write can you turn more of this into comments? Yes, we are expecting the client submitting the RenderPass to do that check. But thinking more with jbauman@'s corner case example below, I think we still need this code in previous patch: https://codereview.chromium.org/2873593002/diff/260001/components/viz/service... https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:311: // containing damage from damage_tracker. On 2017/07/25 18:55:03, danakj wrote: > "containing damage specified with the CompositorFrame" or something. damage > tracker is part of layer compositor which this code does not know about and > could change arbitrarily. Deleted comments related to PrewalkTree. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:313: dest_pass->has_damage_from_contributing_content |= On 2017/07/25 20:40:34, jbauman wrote: > Suppose we have a > Surface 1 with damage, and a surface 2: > Pass A) SurfaceDrawQuad referencing 1 > Pass B) RenderPassDrawQuad referencing A > > If B has cache_render_surface, shouldn't has_damage_from_contributing_content be > set on it? Yes. we should set the flag. You example makes me think I do need this code in previous patch: https://codereview.chromium.org/2873593002/diff/280001/cc/output/direct_rende... Explained as this: When we process Pass A, we insert to dest_pass_list_ a render_pass_1 with damage and flag (has_damage_from_contributing_content) set on it. When we process Pass B, we have the remapped_pass_id in the id_to_render_pass_map_, and it is referring to render_pass_1, which has the flag set, then we will set the render_pass_2 as damaged as well. I am adding a new test for this. > > Also, what do we do with has_damage_from_contributing_content? I don't see it > used anywhere If has_damage_from_contributing_content is set, then in DirectRenderer::UseRenderPass, we will not reuse the texture of the render pass: https://codereview.chromium.org/2873593002/diff/280001/cc/output/direct_rende... https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:428: // aggregate the entire thing, because we want full content. On 2017/07/25 18:55:02, danakj wrote: > I think this comment is saying what we want as the outcome but not why we need > to do it. For copy requests, it's because of how we aggregate damage for only > surfaces that will be swapped, right? For cached render surfaces, is it possible > to have a surface not fully damaged when it first appears, or is it something > else? DamageRectForSurface() seems to make a surface fully damaged the first > time it appears... do you have a unit test showing why we need > has_cached_render_surfaces_ here, and that we get an incorrect cache result > without it? > > Put another way, for each if-condition added in this patch like this, if you > remove it, does some test fail? > > I wanted to just mention also that it's unfortunate IMO that this patch is > phrasing things in terms of render surfaces (which are a mostly obsolete term > from layer compositors, they don't even exist anymore except or > RenderSurfaceImpl), instead of render targets and render passes. Especially for > types and variables that are used from SurfaceAggregator for which "Surface" has > a completely different meaning causing confusion, and "RenderSurface" is not > something it could every know about. We do not need this change any more for cache render pass. I renamed the cache render surface to cache render pass in render_pass.cc, surface aggregator.cc and direct_renderer.cc. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:567: !copy_pass->cache_render_surface && On 2017/07/25 18:55:03, danakj wrote: > I also think each of these needs the same comments explaining why the condition > is here, matching HandleSurfaceQuad() comments if that's the case. Do not need this any more. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... File components/viz/service/display/surface_aggregator_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2462: TEST_F(SurfaceAggregatorValidSurfaceTest, HasDamageFromSurfaces) { On 2017/07/25 18:55:03, danakj wrote: > Can you split this into smaller test cases? I added a new test for jbauman@'s corner case. I also separated this test into two tests: HasDamageByChangingChildSurface and HasDamageByChangingGrandChildSurface. The other test by adding new child_frame, new grand_child_frame and remove grand_child_frame are redundant since child_support_->SubmitCompositorFrame will always damage the surface, which are the same as the test of changing child_frame. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2499: ->has_damage_from_contributing_content); On 2017/07/25 18:55:03, danakj wrote: > This is related to my questions in the aggregator, but why does this need to be > true on the first frame? DamageRectForSurface() makes a surface fully damaged the first time it appears. Just like the root aggregated damage, "Damage rect for first aggregation should contain entire root surface" [1] and then we start to track partial damage from there? [1] https://cs.chromium.org/chromium/src/components/viz/service/display/surface_a... > Should the new object already be covered by the root > aggregated damage? Yes, it is, but has root aggregated damage does not mean we can or cannot cache the render pass. > > If this was false, what would end up in the cache? Is it possible to build > expectations around that? If this is false for the first frame, we do not have the texture of the render pass, we need to always create new texture for the new surface, so we have nothing to cache at this point? https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2564: ->has_damage_from_contributing_content); On 2017/07/25 18:55:03, danakj wrote: > This is related to my questions in the aggregator, but why does this need to be > true on the first frame? Should the new object already be covered by the root > aggregated damage? Similar to previous comments, root aggregated damage does not know where the damage come from, so it cannot help to set has_damage_from_contributing_content. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2614: ->has_damage_from_contributing_content); On 2017/07/25 18:55:03, danakj wrote: > This is related to my questions in the aggregator, but why does this need to be > true on the first frame? Should the removed object already be covered by the > root aggregated damage? Actually, it seems no point for this test, just as previous test, when there is a new frame for the child_frame (every time: child_support_->SubmitCompositorFrame), this expectation is always true. Removed this.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Rebased to fix compile error.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/07/26 07:48:53, wutao wrote: > > > You added cache_render_surface there also for unrelated reasons, which are > that > > you want to ensure the cached render surface is fully drawn even if not all of > > it is damaged? I don't follow the logic for why that's happening - it seems to > > imply that we have a render surface already that is now partially damaged, but > > we want to redraw the pixels that are already present.. Or can you have a > render > > surface appear the first time and not all of it is damaged? If so, how would > > that happen? > > You could be totally right here. Thanks! I will test on removing this. Sounds > like the damage_rect only affect drawing/swap, not affect caching. Could you add a test with the cached RenderPass being initially partially off-screen, then moved onscreen? I would expect the first draw might not damage the entire RenderPass because it would only affect the area of the RenderPass that's onscreen. Then when it's moved onscreen it won't be updated because no contributing content changed. > https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... > components/viz/service/display/surface_aggregator.cc:313: > dest_pass->has_damage_from_contributing_content |= > On 2017/07/25 20:40:34, jbauman wrote: > > Suppose we have a > > Surface 1 with damage, and a surface 2: > > Pass A) SurfaceDrawQuad referencing 1 > > Pass B) RenderPassDrawQuad referencing A > > > > If B has cache_render_surface, shouldn't has_damage_from_contributing_content > be > > set on it? > Yes. we should set the flag. You example makes me think I do need this code in > previous patch: > https://codereview.chromium.org/2873593002/diff/280001/cc/output/direct_rende... > Explained as this: > When we process Pass A, we insert to dest_pass_list_ a render_pass_1 with damage > and flag (has_damage_from_contributing_content) set on it. > When we process Pass B, we have the remapped_pass_id in the > id_to_render_pass_map_, and it is referring to render_pass_1, which has the flag > set, then we will set the render_pass_2 as damaged as well. > > I am adding a new test for this. > Ok, thanks, I think this new code should work.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi danakj@ and jbauman@, I added some more tests to cover the changes in surface_aggregator. PTAL. Thanks, Tao https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... File components/viz/service/display/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:288: !copy_pass->cache_render_surface && On 2017/07/25 18:55:02, danakj wrote: > I feel that each of these 3 conditions needs a comment explaining why it is > here. I tried to understand why this one is here below.. lmk. Added comments. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:428: // aggregate the entire thing, because we want full content. On 2017/07/25 18:55:02, danakj wrote: > For cached render surfaces, is it possible > to have a surface not fully damaged when it first appears, or is it something > else? Yes, if the render pass is off screen, then it is not fully damaged. We should avoid this happening. > DamageRectForSurface() seems to make a surface fully damaged the first > time it appears... do you have a unit test showing why we need > has_cached_render_surfaces_ here, and that we get an incorrect cache result > without it? Added some tests, please see below. > > Put another way, for each if-condition added in this patch like this, if you > remove it, does some test fail? Added several tests to cover all the changes in this patch: 1. HasDamageByChangingChildSurface for why we need DamageRectForSurface in HandleSurfaceQuad to get child surface changes. 2. HasDamageByChangingGrandChildSurface for why we need DamageRectForSurface in HandleSurfaceQuad to get grand child surface changes. 3. HasDamageFromRenderPassQuads for the change in if block (from line 499) if (quad->material == cc::DrawQuad::RENDER_PASS) in CopyQuadsToPass. To get damage from RenderPassQuads. 4. DamageRectOfCachedRenderPass for why we need the change (line 585) !copy_pass->cache_render_pass && in CopyPasses to make the off screen render pass fully damaged. 5. DamageRectOfCachedRenderPassInChildSurface for why we need the change (line 293) !copy_pass->cache_render_pass && in HandleSurfaceQuad to make the off screen render pass fully damaged. 6. NotIgnoreOutsideForCachedRenderPass: for why we need ignore_undamaged = ... && !has_cached_render_surfaces && ..., so that we will not ignore outside render pass. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:567: !copy_pass->cache_render_surface && On 2017/07/25 18:55:03, danakj wrote: > I also think each of these needs the same comments explaining why the condition > is here, matching HandleSurfaceQuad() comments if that's the case. Added comments.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks for the new tests!
Thanks, I have a few small suggestions but I understand the change now thanks to your work here. https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... File components/viz/service/display/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/280001/components/viz/service... components/viz/service/display/surface_aggregator.cc:428: // aggregate the entire thing, because we want full content. On 2017/07/27 02:30:13, wutao wrote: > On 2017/07/25 18:55:02, danakj wrote: > > For cached render surfaces, is it possible > > to have a surface not fully damaged when it first appears, or is it something > > else? > Yes, if the render pass is off screen, then it is not fully damaged. We should > avoid this happening. > > > DamageRectForSurface() seems to make a surface fully damaged the first > > time it appears... do you have a unit test showing why we need > > has_cached_render_surfaces_ here, and that we get an incorrect cache result > > without it? > Added some tests, please see below. > > > > > Put another way, for each if-condition added in this patch like this, if you > > remove it, does some test fail? > > Added several tests to cover all the changes in this patch: > 1. HasDamageByChangingChildSurface > for why we need DamageRectForSurface in HandleSurfaceQuad to get child surface > changes. > 2. HasDamageByChangingGrandChildSurface > for why we need DamageRectForSurface in HandleSurfaceQuad to get grand child > surface changes. > > 3. HasDamageFromRenderPassQuads > for the change in if block (from line 499) if (quad->material == > cc::DrawQuad::RENDER_PASS) in CopyQuadsToPass. > To get damage from RenderPassQuads. > > 4. DamageRectOfCachedRenderPass > for why we need the change (line 585) !copy_pass->cache_render_pass && in > CopyPasses to make the off screen render pass fully damaged. > > 5. DamageRectOfCachedRenderPassInChildSurface > for why we need the change (line 293) !copy_pass->cache_render_pass && in > HandleSurfaceQuad to make the off screen render pass fully damaged. > > 6. NotIgnoreOutsideForCachedRenderPass: > for why we need ignore_undamaged = ... && !has_cached_render_surfaces && ..., so > that we will not ignore outside render pass. Thanks! https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... File components/viz/service/display/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator.cc:291: // . this period should be attached to a word :) so wrap the word instead https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator.cc:304: id_to_render_pass_map_[copy_pass->id] = copy_pass.get(); one thought here: it seems like this map is only used to check for the bool has_damage_from_contributing_content. So instead you could make this a set of RenderPassIds, and only insert if has_damage_from_contributing_content was true, then code just checks to see if the id is present or not later? Or can that bool change after it was inserted into the map here? If we can, then that'd eliminate any map inserts here in the common case. https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator.cc:429: // If the current frame has copy requests or cahced render passes, then cached https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator.cc:585: // . this one also https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator.cc:814: cached_render_passes_.insert(remapped_pass_id); it seems like we're building this structure just to ask if it's empty, so we could just use a bool? https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... File components/viz/service/display/surface_aggregator_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2540: // First aggregation shold be true. DamageRectForSurface() makes a surface "should" Explain why it should, why this bool matters to correct caching in this case, rather than what code will make it so. (This comment appears multiple times.) https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2556: AddPasses(&child_surface_frame.render_pass_list, gfx::Rect(SurfaceSize()), What happens if a new frame comes but has no damage on it? https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2782: // For off screen render pass, only the visible area is damaged. nit: usually write offscreen, or off-screen works too https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2802: // Only the visible area is damaged. Good test. You could also show that the root pass (pass[1]) has damage for only what changed, in both this case and the one below? https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2901: // Only the visible area is damaged. same here?
Hi danakj@ Please take another look. Thanks, Tao https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... File components/viz/service/display/surface_aggregator.cc (right): https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator.cc:291: // . On 2017/07/27 21:24:33, danakj wrote: > this period should be attached to a word :) so wrap the word instead Done https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator.cc:304: id_to_render_pass_map_[copy_pass->id] = copy_pass.get(); On 2017/07/27 21:24:33, danakj wrote: > one thought here: it seems like this map is only used to check for the bool > has_damage_from_contributing_content. So instead you could make this a set of > RenderPassIds, and only insert if has_damage_from_contributing_content was true, > then code just checks to see if the id is present or not later? Or can that bool > change after it was inserted into the map here? > > If we can, then that'd eliminate any map inserts here in the common case. Very nice, thank you. The bool should not change after it becomes true and be inserted. https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator.cc:429: // If the current frame has copy requests or cahced render passes, then On 2017/07/27 21:24:33, danakj wrote: > cached Done. https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator.cc:585: // . On 2017/07/27 21:24:33, danakj wrote: > this one also Done. https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator.cc:814: cached_render_passes_.insert(remapped_pass_id); On 2017/07/27 21:24:33, danakj wrote: > it seems like we're building this structure just to ask if it's empty, so we > could just use a bool? Good point, done. https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... File components/viz/service/display/surface_aggregator_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2540: // First aggregation shold be true. DamageRectForSurface() makes a surface On 2017/07/27 21:24:34, danakj wrote: > "should" > > Explain why it should, why this bool matters to correct caching in this case, > rather than what code will make it so. (This comment appears multiple times.) Talked offline to remove this check and add comment: On first frame there is no existing cache texture to worry about re-using, so we don't worry what this bool is set to. https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2556: AddPasses(&child_surface_frame.render_pass_list, gfx::Rect(SurfaceSize()), On 2017/07/27 21:24:34, danakj wrote: > What happens if a new frame comes but has no damage on it? Adding a new test case for this. https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2782: // For off screen render pass, only the visible area is damaged. On 2017/07/27 21:24:34, danakj wrote: > nit: usually write offscreen, or off-screen works too Done. https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2802: // Only the visible area is damaged. On 2017/07/27 21:24:34, danakj wrote: > Good test. You could also show that the root pass (pass[1]) has damage for only > what changed, in both this case and the one below? Done. https://codereview.chromium.org/2873593002/diff/340001/components/viz/service... components/viz/service/display/surface_aggregator_unittest.cc:2901: // Only the visible area is damaged. On 2017/07/27 21:24:34, danakj wrote: > same here? Done.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for working through this. LGTM https://codereview.chromium.org/2873593002/diff/360001/cc/trees/damage_tracke... File cc/trees/damage_tracker_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/360001/cc/trees/damage_tracke... cc/trees/damage_tracker_unittest.cc:600: // layer's layer_property_changed_ should be considered as damage to render nit: Layer's
Thanks, surface_aggregator stuff LGTM
https://codereview.chromium.org/2873593002/diff/360001/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/2873593002/diff/360001/ui/compositor/layer.cc... ui/compositor/layer.cc:658: cc_layer_->SetCacheRenderSurface(cache_render_surface); You should leave a comment explaining why Clone() and SwitchToLayer() are not preserving this, if it is intentional.
https://codereview.chromium.org/2873593002/diff/360001/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/2873593002/diff/360001/ui/compositor/layer.cc... ui/compositor/layer.cc:658: cc_layer_->SetCacheRenderSurface(cache_render_surface); On 2017/07/28 22:22:58, danakj wrote: > You should leave a comment explaining why Clone() and SwitchToLayer() are not > preserving this, if it is intentional. Adding a comment to explain why not propogated to the new layer in Clone, and making it preserve it in SwitchToLayer() would LGTM
Thank you everyone reviewing this cl. Will land it soon. https://codereview.chromium.org/2873593002/diff/360001/cc/trees/damage_tracke... File cc/trees/damage_tracker_unittest.cc (right): https://codereview.chromium.org/2873593002/diff/360001/cc/trees/damage_tracke... cc/trees/damage_tracker_unittest.cc:600: // layer's layer_property_changed_ should be considered as damage to render On 2017/07/28 21:40:44, weiliangc wrote: > nit: Layer's Done. https://codereview.chromium.org/2873593002/diff/360001/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/2873593002/diff/360001/ui/compositor/layer.cc... ui/compositor/layer.cc:658: cc_layer_->SetCacheRenderSurface(cache_render_surface); On 2017/07/28 22:32:45, danakj wrote: > On 2017/07/28 22:22:58, danakj wrote: > > You should leave a comment explaining why Clone() and SwitchToLayer() are not > > preserving this, if it is intentional. > > Adding a comment to explain why not propogated to the new layer in Clone, and > making it preserve it in SwitchToLayer() would LGTM Added a comment and two tests for cloning and switching cc_layer.
Description was changed from ========== Force use of and cache render surface. Force use of render surface and cache it when there is no damage from contributing contents. This will allow us to create a caching mechanism that is simple to maintain and provides the benefits needed to make the Chrome OS UI fast. BUG=708513 TEST=manual && new cc_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Force use of and cache render surface. Force use of render surface and cache it when there is no damage from contributing contents. This will allow us to create a caching mechanism that is simple to maintain and provides the benefits needed to make the Chrome OS UI fast. BUG=708513 TEST=manual && new damage_tracker_unittests && surface_aggregator_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by wutao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ochang@chromium.org, reveman@chromium.org, jbauman@chromium.org, weiliangc@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2873593002/#ps400001 (title: "Fix a nit: layer -> Layer.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by wutao@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Force use of and cache render surface. Force use of render surface and cache it when there is no damage from contributing contents. This will allow us to create a caching mechanism that is simple to maintain and provides the benefits needed to make the Chrome OS UI fast. BUG=708513 TEST=manual && new damage_tracker_unittests && surface_aggregator_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Force use of and cache render surface. Force use of render surface and cache it when there is no damage from contributing contents. This will allow us to create a caching mechanism that is simple to maintain and provides the benefits needed to make the Chrome OS UI fast. BUG=708513 TEST=manual && new damage_tracker_unittests && surface_aggregator_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wutao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ochang@chromium.org, weiliangc@chromium.org, reveman@chromium.org, danakj@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2873593002/#ps420001 (title: "Rebased to resolve conflict.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1501359593572090,
"parent_rev": "56e6b2ef3eaeaf1e315eab0fc500f0616b148842", "commit_rev":
"36850738fee2ff9af2f0207353d1d3b00ad3f5df"}
Message was sent while issue was closed.
Description was changed from ========== Force use of and cache render surface. Force use of render surface and cache it when there is no damage from contributing contents. This will allow us to create a caching mechanism that is simple to maintain and provides the benefits needed to make the Chrome OS UI fast. BUG=708513 TEST=manual && new damage_tracker_unittests && surface_aggregator_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Force use of and cache render surface. Force use of render surface and cache it when there is no damage from contributing contents. This will allow us to create a caching mechanism that is simple to maintain and provides the benefits needed to make the Chrome OS UI fast. BUG=708513 TEST=manual && new damage_tracker_unittests && surface_aggregator_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2873593002 Cr-Commit-Position: refs/heads/master@{#490671} Committed: https://chromium.googlesource.com/chromium/src/+/36850738fee2ff9af2f0207353d1... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001) as https://chromium.googlesource.com/chromium/src/+/36850738fee2ff9af2f0207353d1... |
