Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1007623002:
Fix resourceless software draw (Closed)
DescriptionFix resourceless software draw crash
Resourceless software draw can only have a single root layer
render surface. Since decision to create render surface has
been moved to main thread, this condition no longer holds
and the mismatch causes a crash in LayerIterator.
Instead tweak the logic to only *use* the root layer render
surface, while being possible to to have more than one
render surface in the tree. This means only the root render
layer can be a render target.
BUG=466695
Committed: https://crrev.com/13185caf5c14377166ef88f467570c34e37af81b
Cr-Commit-Position: refs/heads/master@{#320821}
Patch Set 1 #
Total comments: 5
Patch Set 2 : tests #
Total comments: 10
Patch Set 3 : review #
Total comments: 2
Patch Set 4 : EXPECT_GE #
Messages
Total messages: 32 (5 generated)
enne@chromium.org changed reviewers: + enne@chromium.org
I'm not 100% sure what the answer is here. There's a copy request that's being set on the compositor thread? Maybe we just need a one-off SetCopyRequestFromCompositorThread function that adds a copy request, creates a render surface, then recurses from the layer, clobbering any children that were pointing to the previous parent to point to itself instead? https://codereview.chromium.org/1007623002/diff/1/cc/trees/layer_tree_host_co... File cc/trees/layer_tree_host_common.h (right): https://codereview.chromium.org/1007623002/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common.h:180: return layer->render_surface() && layer->render_target() == layer && layer->render_target() is going to go away at some point soon, so I don't think this is legit.
Er, sorry, "pointing to the previous parent to point to itself instead" => "pointing to that layer's previous target to point to the layer as the target instead"
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/1007623002/diff/1/cc/trees/layer_tree_host_co... File cc/trees/layer_tree_host_common.h (right): https://codereview.chromium.org/1007623002/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common.h:180: return layer->render_surface() && layer->render_target() == layer && On 2015/03/13 17:18:47, enne wrote: > layer->render_target() is going to go away at some point soon, so I don't think > this is legit. With main thread surface calc, how else can we tell if the surface was actually used?
boliu@chromium.org changed reviewers: + aelias@chromium.org
On 2015/03/13 17:18:47, enne wrote: > I'm not 100% sure what the answer is here. There's a copy request that's being > set on the compositor thread? > > Maybe we just need a one-off SetCopyRequestFromCompositorThread function that > adds a copy request, creates a render surface, then recurses from the layer, > clobbering any children that were pointing to the previous parent to point to > itself instead? I don't think I understand this comment.. If you meant copy request being a trigger for using RenderSurfaces, then it's not the case here. I checked one of the sites, and non-root render surface is used due to a layer having a mask_layer on the main thread. Webview is not adding CopyRequests on the impl thread. If you meant copy request being the longer term solution for resourceless software draw supporting additional render surfaces, then ehh, +aelias to comment on that.. https://codereview.chromium.org/1007623002/diff/1/cc/trees/layer_tree_host_co... File cc/trees/layer_tree_host_common.h (right): https://codereview.chromium.org/1007623002/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common.h:180: return layer->render_surface() && layer->render_target() == layer && On 2015/03/13 17:41:36, danakj wrote: > On 2015/03/13 17:18:47, enne wrote: > > layer->render_target() is going to go away at some point soon, so I don't > think > > this is legit. > > With main thread surface calc, how else can we tell if the surface was actually > used? If it wasn't clear, I was trying to capture CalculateDrawProperties going to the else block of this if-statement for this layer: https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... This change matters when: IsRootLayer(layer) is false globals.can_render_to_separate_surface is false layer->render_surface() is true
Ignore my previous comments entirely. I misunderstood what was going on. I chatted with vollick and ajuma, and have a suggestion. When you do a commit, if you're in resourceless software mode, then do a walk of the tree and clobber all the render target ids to point to the root, and remove all render surfaces. Maybe DCHECK that there aren't copy requests too, I guess? Then when we ever get out of this mode, mark the tree as being unable to draw and force a commit. (We have contents_texture_purged and viewport_size_invalid that do similar things. Maybe we need another bool, or maybe you can combine this and those two into a single boolean if it makes sense. How's that sound?
On 2015/03/13 at 17:50:35, boliu wrote: > If you meant copy request being the longer term solution for resourceless software draw supporting additional render surfaces, then ehh, +aelias to comment on that.. That's not viable, the readback would cause a major perf regression. I think we do need to restore the previous behavior of overriding the usual render surface decision on the impl thread in this mode, the only question is how. Note that we also should have some kind of best-effort rendering and not drop the contents of the render surface entirely, as when we tried that before we noticed some ad WebViews went black.
On 2015/03/13 18:13:11, enne wrote: > Ignore my previous comments entirely. I misunderstood what was going on. > > I chatted with vollick and ajuma, and have a suggestion. > > When you do a commit, if you're in resourceless software mode, then do a walk of > the tree and clobber all the render target ids to point to the root, and remove > all render surfaces. Maybe DCHECK that there aren't copy requests too, I guess? > > Then when we ever get out of this mode, mark the tree as being unable to draw > and force a commit. (We have contents_texture_purged and viewport_size_invalid > that do similar things. Maybe we need another bool, or maybe you can combine > this and those two into a single boolean if it makes sense. > > How's that sound? This won't work because resourceless software draw is not a mode :( Whether a draw is normal or "resourceless software" happens on a per-draw basis. And webview can't get this information before hand. So there is no way to correctly make that decision at commit time.
On 2015/03/13 at 18:13:11, enne wrote: > Ignore my previous comments entirely. I misunderstood what was going on. > > I chatted with vollick and ajuma, and have a suggestion. > > When you do a commit, if you're in resourceless software mode, then do a walk of the tree and clobber all the render target ids to point to the root, and remove all render surfaces. Maybe DCHECK that there aren't copy requests too, I guess? > > Then when we ever get out of this mode, mark the tree as being unable to draw and force a commit. (We have contents_texture_purged and viewport_size_invalid that do similar things. Maybe we need another bool, or maybe you can combine this and those two into a single boolean if it makes sense. > > How's that sound? That won't work because we don't know the mode at commit time, only at draw time.
On Fri, Mar 13, 2015 at 11:13 AM, <enne@chromium.org> wrote: > Ignore my previous comments entirely. I misunderstood what was going on. > > I chatted with vollick and ajuma, and have a suggestion. > > When you do a commit, if you're in resourceless software mode, then do a > walk of > the tree and clobber all the render target ids to point to the root, and > remove > all render surfaces. Maybe DCHECK that there aren't copy requests too, I > guess? > Resourceless software draws arent tied to commits, they can happen for any given impl-side frame regardless of source/main frame. > > Then when we ever get out of this mode, mark the tree as being unable to > draw > and force a commit. (We have contents_texture_purged and > viewport_size_invalid > that do similar things. Maybe we need another bool, or maybe you can > combine > this and those two into a single boolean if it makes sense. > Webview has to be able to draw always (always_draw_and_swap_full_viewport) so I'm not sure if this would be okay? > > How's that sound? > > https://codereview.chromium.org/1007623002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1007623002/diff/1/cc/trees/layer_tree_host_co... File cc/trees/layer_tree_host_common.h (right): https://codereview.chromium.org/1007623002/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common.h:180: return layer->render_surface() && layer->render_target() == layer && On 2015/03/13 17:50:35, boliu wrote: > On 2015/03/13 17:41:36, danakj wrote: > > On 2015/03/13 17:18:47, enne wrote: > > > layer->render_target() is going to go away at some point soon, so I don't > > think > > > this is legit. > > > > With main thread surface calc, how else can we tell if the surface was > actually > > used? > > If it wasn't clear, I was trying to capture CalculateDrawProperties going to the > else block of this if-statement for this layer: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... > > This change matters when: > IsRootLayer(layer) is false > globals.can_render_to_separate_surface is false > layer->render_surface() is true I think this is a reasonable fix, the bug here is that the we are using the layer->render_surface() but not looking at the layer->render_target() which became illegal to do when we moved render surface computation to the main thread. After lunchdiscussion with enne@ I think we're both on board to fix this. In property tree world, layerization decisions will have us generate a layer tree, and the layer tree will hold draw transforms and render targets. So these can be recomputed when software draw is enabled or disabled. So, using the render_target() should make sense going forward in the future too. tl;dr can you just remove the render_surface() check here entirely and replace it with the render_target() one? And is there a test we can write for this?
Thanks all! I'll ping here again once I have a test https://codereview.chromium.org/1007623002/diff/1/cc/trees/layer_tree_host_co... File cc/trees/layer_tree_host_common.h (right): https://codereview.chromium.org/1007623002/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common.h:180: return layer->render_surface() && layer->render_target() == layer && On 2015/03/13 20:02:33, danakj wrote: > On 2015/03/13 17:50:35, boliu wrote: > > On 2015/03/13 17:41:36, danakj wrote: > > > On 2015/03/13 17:18:47, enne wrote: > > > > layer->render_target() is going to go away at some point soon, so I don't > > > think > > > > this is legit. > > > > > > With main thread surface calc, how else can we tell if the surface was > > actually > > > used? > > > > If it wasn't clear, I was trying to capture CalculateDrawProperties going to > the > > else block of this if-statement for this layer: > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... > > > > This change matters when: > > IsRootLayer(layer) is false > > globals.can_render_to_separate_surface is false > > layer->render_surface() is true > > I think this is a reasonable fix, the bug here is that the we are using the > layer->render_surface() but not looking at the layer->render_target() which > became illegal to do when we moved render surface computation to the main > thread. > > After lunchdiscussion with enne@ I think we're both on board to fix this. In > property tree world, layerization decisions will have us generate a layer tree, > and the layer tree will hold draw transforms and render targets. So these can be > recomputed when software draw is enabled or disabled. So, using the > render_target() should make sense going forward in the future too. Don't really understand the "property tree world", but not a pre-requisite for this change :p > > tl;dr can you just remove the render_surface() check here entirely and replace > it with the render_target() one? > > And is there a test we can write for this? Of course!
ptal Two tests, both reproduce the crash in the bug without the fix. One unit test, and one more end to end.
https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:2966: // PictureLayer can only be used with impl side painting enabled. You could use the IMPL flavour of the MULTI_THREAD_TEST macros to do this also. Then you don't have non-implside tests running and duplicating the impl-side ones. https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:2993: scoped_ptr<SoftwareOutputDevice>(new SoftwareOutputDevice), nit: make_scoped_ptr() instead of using the constructor explicitly? https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:2997: void BeginTest() override { PostSetNeedsUpdateLayersToMainThread(); } nit: PostSetNeedsCommit...()? That path has less logic and would be fine for this test? https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:3010: LayerImpl* parent_impl = root_impl->children()[0]; I think it would be better to test the RenderPasses rather than the layers here. Override PrepareToDraw() and verify there's only one RenderPass in the list, and N shared quad states in that pass?
https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:3010: LayerImpl* parent_impl = root_impl->children()[0]; On 2015/03/16 17:32:19, danakj wrote: > I think it would be better to test the RenderPasses rather than the layers here. > Override PrepareToDraw() and verify there's only one RenderPass in the list, and > N shared quad states in that pass? I also want to verify things start has multiple render surfaces. Tried to checking render surfaces on the main layers, but couldn't find the hook at the right place such the render_surface pointers on main layers are still up to date. Seems main layers create a RSLL, which gets destroyed and cleared immediately after UpdateLayers. Also Layer::has_render_surface() is being deprecated, so didn't want to use that. Could do a normal draw once first to verify there are two render passes first. Has any better suggestions?
https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:3010: LayerImpl* parent_impl = root_impl->children()[0]; On 2015/03/16 18:12:22, boliu wrote: > On 2015/03/16 17:32:19, danakj wrote: > > I think it would be better to test the RenderPasses rather than the layers > here. > > Override PrepareToDraw() and verify there's only one RenderPass in the list, > and > > N shared quad states in that pass? > > I also want to verify things start has multiple render surfaces. > > Tried to checking render surfaces on the main layers, but couldn't find the hook > at the right place such the render_surface pointers on main layers are still up > to date. Seems main layers create a RSLL, which gets destroyed and cleared > immediately after UpdateLayers. > > Also Layer::has_render_surface() is being deprecated, so didn't want to use > that. > > Could do a normal draw once first to verify there are two render passes first. > Has any better suggestions? I like that idea. It keeps the testing surface area down (just on the passes/quads) which are not going to totally change from property trees, so that makes the test more stable.
https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:2963: class LayerTreeHostTestResourcelessSoftwareDraw : public LayerTreeHostTest { I don't really think we need this integration test. I think the other one that verifies that only the root has a surface and all 4 layers represent themselves is sufficient. We have plenty of other testing to make sure that the RSLL is turned into passes correctly.
https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:2963: class LayerTreeHostTestResourcelessSoftwareDraw : public LayerTreeHostTest { On 2015/03/16 18:24:36, enne wrote: > I don't really think we need this integration test. I think the other one that > verifies that only the root has a surface and all 4 layers represent themselves > is sufficient. We have plenty of other testing to make sure that the RSLL is > turned into passes correctly. Im also good with that.
https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:2963: class LayerTreeHostTestResourcelessSoftwareDraw : public LayerTreeHostTest { On 2015/03/16 18:26:43, danakj wrote: > On 2015/03/16 18:24:36, enne wrote: > > I don't really think we need this integration test. I think the other one > that > > verifies that only the root has a surface and all 4 layers represent > themselves > > is sufficient. We have plenty of other testing to make sure that the RSLL is > > turned into passes correctly. > > Im also good with that. I think this is not really quad generation test, more like a smoke test that nothing crashes with resourceless draws. I guess the question is if there's anything else not covered by that unit test that has the potential to crash with resourceless software draws. Eg something will be added to the end of LayerTreeImpl::UpdateDrawProperties that makes a bad assumption? If both of you think this is unlikely, then I'll remove this test.
https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1007623002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:2963: class LayerTreeHostTestResourcelessSoftwareDraw : public LayerTreeHostTest { On 2015/03/16 18:35:07, boliu wrote: > On 2015/03/16 18:26:43, danakj wrote: > > On 2015/03/16 18:24:36, enne wrote: > > > I don't really think we need this integration test. I think the other one > > that > > > verifies that only the root has a surface and all 4 layers represent > > themselves > > > is sufficient. We have plenty of other testing to make sure that the RSLL > is > > > turned into passes correctly. > > > > Im also good with that. > > I think this is not really quad generation test, more like a smoke test that > nothing crashes with resourceless draws. > > I guess the question is if there's anything else not covered by that unit test > that has the potential to crash with resourceless software draws. Eg something > will be added to the end of LayerTreeImpl::UpdateDrawProperties that makes a bad > assumption? > > If both of you think this is unlikely, then I'll remove this test. I did recently make changes in CalcRenderPasses that ended up DCHECKing in some webview unit test due to not having correct results when draw_and_swap_full_viewport, so I found such a test useful. I probably would have broken some webview instrumentation test otherwise. But maybe not. All things being equal I think I'd keep this test, but maybe enne@ has a stronger opinion than me here.
New patch set up addressing Dana's review comments. Still easy to delete the test if we decide to do that.
LGTM % enne https://codereview.chromium.org/1007623002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1007623002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:3008: EXPECT_LE(1u, frame_data->render_passes[0]->quad_list.size()); nit: can you write this as GE, ie the same way we'd do it with an operator: foo >= 2. There's not "Expected, Actual" ordering required for these.
https://codereview.chromium.org/1007623002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1007623002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:3008: EXPECT_LE(1u, frame_data->render_passes[0]->quad_list.size()); On 2015/03/16 20:35:41, danakj wrote: > nit: can you write this as GE, ie the same way we'd do it with an operator: foo > >= 2. There's not "Expected, Actual" ordering required for these. Done.
The CQ bit was checked by enne@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1007623002/#ps50004 (title: "EXPECT_GE")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1007623002/50004
Message was sent while issue was closed.
Committed patchset #4 (id:50004)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/13185caf5c14377166ef88f467570c34e37af81b Cr-Commit-Position: refs/heads/master@{#320821} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
