Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
151093005:
cc: Update Main RendererCapabilities on DeferredInitialize (Closed)
|
Created:
6 years, 10 months ago by boliu Modified:
6 years, 10 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc: Update Main RendererCapabilities on DeferredInitialize
Also update caps on ReleaseGL. The updated RendererCapabilities
are posted back to the main thread. Main thread layers will
receive OnOutputSurfaceCreated (should be renamed to
OnRendererCapabilitiesChanged) when this happens. Note this
is also called on first renderer initialization.
BUG=332616
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251853
Patch Set 1 #Patch Set 2 : cancel commits with stale caps, no tests yet #
Total comments: 1
Patch Set 3 : with tests #
Total comments: 26
Patch Set 4 : rebase + clang format #Patch Set 5 : Address some comments #Patch Set 6 : Move staleness check to ReadyToFinalizeTextureUpdates. Add last ref check to TextureLayer::OnOutput… #Patch Set 7 : rebase #Patch Set 8 : Remove DCHECK in TextureLayer::OnOutputSurfaceCreated #
Total comments: 3
Patch Set 9 : rebase #Patch Set 10 : Release Mailbox if UsingSharedMemoryResources changed in TextureLayer::Update #
Total comments: 19
Patch Set 11 : remove cancel commit logic #Patch Set 12 : UpdateRendererCapabilitiesOnImplThread in CreateAndSetRenderer #Patch Set 13 : UpdateRendererCapabilitiesOnImplThread in CreateAndSetRenderer #
Total comments: 3
Patch Set 14 : Simplify a lot #Patch Set 15 : Add back a DCHECK that was accdentially deleted #
Messages
Total messages: 46 (0 generated)
PTAL. HUDLayer and PaintedScrollbarLayer only uses the max texture size, so already does the right thing on the next layer update. https://codereview.chromium.org/151093005/diff/30001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/30001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:1021: "cc", "EarlyOut_StaleRendererCaps", TRACE_EVENT_SCOPE_THREAD); TEST! https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.... cc/layers/texture_layer.cc:281: holder_ref_.reset(); So for mailboxes the contract is maintained implicitly in the client, that if the previous mailbox is returned, then the layer will need another one in the next PrepareTextureMailbox. Can easily follow up to make that explicit by passing an arg in PrepareTextureMailbox. I'm not sure what the current contract is when uses_mailbox_ is false though. Webview doesn't care at the moment. I think the explicit arg will work as well? https://codereview.chromium.org/151093005/diff/60001/cc/trees/single_thread_p... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/trees/single_thread_p... cc/trees/single_thread_proxy.cc:376: layer_tree_host_->RendererCapabilitiesChanged(); This will never actually be called by LayerTreeHostImpl. I just made these changes in anticipation of the OutputSurfaceProvider work and to keep some functions more consistent with thread_proxy. I can make this NOTREACHED if that's preferred.
Friendly ping?
On 2014/02/10 21:02:56, boliu wrote: > Friendly ping? Would be great if I could get this in before m34 branch point :)
On Tue, Feb 11, 2014 at 2:39 PM, <boliu@chromium.org> wrote: > On 2014/02/10 21:02:56, boliu wrote: > >> Friendly ping? >> > > Would be great if I could get this in before m34 branch point :) > Sorry am mid-review > > https://codereview.chromium.org/151093005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.... cc/layers/texture_layer.cc:280: if (uses_mailbox_) { If we release the mailbox here, we should be calling SetNeedsCommit()? How does this relate to the content on the impl thread? We can't just go using that content now right? I'm not sure how we'd try prevent it tho.. https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.... cc/layers/texture_layer.cc:281: holder_ref_.reset(); On 2014/02/07 04:29:55, boliu wrote: > So for mailboxes the contract is maintained implicitly in the client, that if > the previous mailbox is returned, then the layer will need another one in the > next PrepareTextureMailbox. Can easily follow up to make that explicit by > passing an arg in PrepareTextureMailbox. > > I'm not sure what the current contract is when uses_mailbox_ is false though. > Webview doesn't care at the moment. I think the explicit arg will work as well? It'd be nice to just mark the layer as "bad" until it gets a new resource, and prevent drawing until no layers are "bad". That'd let us avoid drawing black layers without forcing the client to block in PrepareTextureMailbox. For non-mailbox, the client watches its context and gets told when the context is lost. Consumers of it are aura reflectors and android ui, neither of which webview cares about. We should just delete the code path rather than adding complexity to it. https://codereview.chromium.org/151093005/diff/60001/cc/output/renderer.cc File cc/output/renderer.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/output/renderer.cc#ne... cc/output/renderer.cc:39: bool RendererCapabilitiesImpl::Equals( This should move to RendererCapabilities (main thread class) I think? It's weird to Equals(some other type). https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3113: scoped_ptr<TestWebGraphicsContext3D> context3d( unused? https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3135: EXPECT_EQ(static_cast<size_t>(1), "1u" instead of static_cast<size_t>(1) https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3139: // Make sure first main frame attemp gets aborted due to stale renderer attempt https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3152: EXPECT_EQ(static_cast<size_t>(1), 1u https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3170: aborted_frame_ = true; use an int instead so you can test the number of times https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3180: // First attempted should have been aborted. attempt https://codereview.chromium.org/151093005/diff/60001/cc/trees/single_thread_p... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/trees/single_thread_p... cc/trees/single_thread_proxy.cc:376: layer_tree_host_->RendererCapabilitiesChanged(); On 2014/02/07 04:29:55, boliu wrote: > This will never actually be called by LayerTreeHostImpl. I just made these > changes in anticipation of the OutputSurfaceProvider work and to keep some > functions more consistent with thread_proxy. > > I can make this NOTREACHED if that's preferred. I don't see how this won't be reached, it's called from UpdateRendererCapabilitiesOnImplThread(). You mean to move this out of the method to the callsite? I think it's good here as is.
https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.... cc/layers/texture_layer.cc:280: if (uses_mailbox_) { On 2014/02/11 19:58:59, danakj wrote: > If we release the mailbox here, we should be calling SetNeedsCommit()? That's already handled on the impl side (DeferredInitialize and ReleaseGL already calls SetNeedsCommit). Don't need and probably don't want main layers to set that racily too. > How does > this relate to the content on the impl thread? We can't just go using that > content now right? I'm not sure how we'd try prevent it tho.. s/content/context/ ? So the context used by accelerated canvas is *not* lost or destroyed when ReleaseGL happens. There are a few cases: If mailbox has not been committed to impl side, then we just release it back to client here. If the mailbox has already been committed to impl side, then impl layer will have already released it by this point, so this reset will probably do nothing. The edge case is the impl release races with an update here, so it's possible impl side released mailbox n, and we are already holding mailbox n+1. And we'll release n+1 also. https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.... cc/layers/texture_layer.cc:281: holder_ref_.reset(); On 2014/02/11 19:58:59, danakj wrote: > On 2014/02/07 04:29:55, boliu wrote: > > So for mailboxes the contract is maintained implicitly in the client, that if > > the previous mailbox is returned, then the layer will need another one in the > > next PrepareTextureMailbox. Can easily follow up to make that explicit by > > passing an arg in PrepareTextureMailbox. > > > > I'm not sure what the current contract is when uses_mailbox_ is false though. > > Webview doesn't care at the moment. I think the explicit arg will work as > well? > > It'd be nice to just mark the layer as "bad" until it gets a new resource, and > prevent drawing until no layers are "bad". That'd let us avoid drawing black > layers without forcing the client to block in PrepareTextureMailbox. Could I do this in a follow up? :p Or do you think I'm breaking something with this change already? > For non-mailbox, the client watches its context and gets told when the context > is lost. Consumers of it are aura reflectors and android ui, neither of which > webview cares about. We should just delete the code path rather than adding > complexity to it. And more follow up... https://codereview.chromium.org/151093005/diff/60001/cc/output/renderer.cc File cc/output/renderer.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/output/renderer.cc#ne... cc/output/renderer.cc:39: bool RendererCapabilitiesImpl::Equals( On 2014/02/11 19:58:59, danakj wrote: > This should move to RendererCapabilities (main thread class) I think? It's weird > to Equals(some other type). Done. https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3113: scoped_ptr<TestWebGraphicsContext3D> context3d( On 2014/02/11 19:58:59, danakj wrote: > unused? Done. https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3135: EXPECT_EQ(static_cast<size_t>(1), On 2014/02/11 19:58:59, danakj wrote: > "1u" instead of static_cast<size_t>(1) Done. https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3139: // Make sure first main frame attemp gets aborted due to stale renderer On 2014/02/11 19:58:59, danakj wrote: > attempt Done. https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3152: EXPECT_EQ(static_cast<size_t>(1), On 2014/02/11 19:58:59, danakj wrote: > 1u Done. https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3170: aborted_frame_ = true; On 2014/02/11 19:58:59, danakj wrote: > use an int instead so you can test the number of times Done. https://codereview.chromium.org/151093005/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:3180: // First attempted should have been aborted. On 2014/02/11 19:58:59, danakj wrote: > attempt Done.
Oops, got a bit trigger happy in last reply... https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.... cc/layers/texture_layer.cc:280: if (uses_mailbox_) { On 2014/02/11 19:58:59, danakj wrote: > How does > this relate to the content on the impl thread? We can't just go using that > content now right? I'm not sure how we'd try prevent it tho.. Actually I'm not clear what your concern is about the impl context here. For accelerated canvas in webview case, the canvas context will still be alive even if webview goes back to software mode. And I think in general, we shouldn't assume compositor impl context loss also means main thread context lost? I don't really know how context loss happens on desktop though :/ Also this should be called only after a new context/OS has been recreated as the name implies. https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.... cc/layers/texture_layer.cc:281: holder_ref_.reset(); On 2014/02/11 19:58:59, danakj wrote: > It'd be nice to just mark the layer as "bad" until it gets a new resource, and > prevent drawing until no layers are "bad". That'd let us avoid drawing black > layers without forcing the client to block in PrepareTextureMailbox. This should just work right now? Without a valid mailbox, and if PrepareTextureMailbox/PrepareTexture returning false, TextureLayer::Update will return false as well. So client doesn't need to block, just need to call SetNeedsUpdate when it's ready. And in TextureLayerImpl::WillDraw, if layer doesn't have a valid Mailbox/Texture, it will return false, and AppendQuads will be skipped, so will not draw black box.
https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.... cc/layers/texture_layer.cc:280: if (uses_mailbox_) { On 2014/02/12 01:48:57, boliu wrote: > On 2014/02/11 19:58:59, danakj wrote: > > How does > > this relate to the content on the impl thread? We can't just go using that > > content now right? I'm not sure how we'd try prevent it tho.. > > Actually I'm not clear what your concern is about the impl context here. > > For accelerated canvas in webview case, the canvas context will still be alive > even if webview goes back to software mode. > > And I think in general, we shouldn't assume compositor impl context loss also > means main thread context lost? I don't really know how context loss happens on > desktop though :/ > > Also this should be called only after a new context/OS has been recreated as the > name implies. I did mean content, the impl side texture. But then as you said, the impl side is already released first when we get here. Can we DCHECK here that this is the last reference left on the mailbox holder? The release post-task should have been run first right? https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.... cc/layers/texture_layer.cc:281: holder_ref_.reset(); On 2014/02/12 01:48:57, boliu wrote: > On 2014/02/11 19:58:59, danakj wrote: > > It'd be nice to just mark the layer as "bad" until it gets a new resource, and > > prevent drawing until no layers are "bad". That'd let us avoid drawing black > > layers without forcing the client to block in PrepareTextureMailbox. > > This should just work right now? > > Without a valid mailbox, and if PrepareTextureMailbox/PrepareTexture returning > false, TextureLayer::Update will return false as well. So client doesn't need to > block, just need to call SetNeedsUpdate when it's ready. > > And in TextureLayerImpl::WillDraw, if layer doesn't have a valid > Mailbox/Texture, it will return false, and AppendQuads will be skipped, so will > not draw black box. Oh, we'll draw a frame without the layer there, which is still visually weird though. I didn't mean that you need to do that in this CL, but we're setting ourselves up for it, so it'd be nice to do.
https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/151093005/diff/60001/cc/layers/texture_layer.... cc/layers/texture_layer.cc:280: if (uses_mailbox_) { On 2014/02/12 15:40:01, danakj wrote: > I did mean content, the impl side texture. But then as you said, the impl side > is already released first when we get here. Can we DCHECK here that this is the > last reference left on the mailbox holder? The release post-task should have > been run first right? > Good suggestion. Caught an edge case that I haven't thought of. If DeferredInitialize or ReleaseGL comes after ThreadProxy::StartCommitOnImplThread but before ThreadProxy::ScheduledActionCommit, then the commit will go through with stale renderer caps, and this check will fail. I think can solve this by moving the staleness check to ThreadProxy::UpdateRendererCapabilitiesOnImplThread. Coding it up now and seeing if it works.
PTAL Moved the staleness check to ThreadProxy::ReadyToFinalizeTextureUpdates, the last point before commit happens. No changes needed in test. And finally added the last ref check in TextureLayer::OnOutputSurfaceCreated, which should hold now.
https://codereview.chromium.org/151093005/diff/650001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/151093005/diff/650001/cc/layers/texture_layer... cc/layers/texture_layer.cc:281: holder_ref_.reset(); I removed the DCHECK for now. This broke context_lost test on the gpu bots. The issue here is that in a chrome context loss, it recreates another renderer with the same RendererCapabilities. So there is nothing stopping a new commit going through, even if the context is lost. I still think it's correct here to drop the ref here. But to make the DCHECK work, we have to pass a bool param saying whether the renderer caps actually changed with the new OS, and only assert single ref if it did change. Thought I'd get your opinion first before implementing this though.
ping again? :)
https://codereview.chromium.org/151093005/diff/650001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/151093005/diff/650001/cc/layers/texture_layer... cc/layers/texture_layer.cc:281: holder_ref_.reset(); On 2014/02/13 20:14:39, boliu wrote: > I removed the DCHECK for now. This broke context_lost test on the gpu bots. > > The issue here is that in a chrome context loss, it recreates another renderer > with the same RendererCapabilities. So there is nothing stopping a new commit > going through, even if the context is lost. > > I still think it's correct here to drop the ref here. But to make the DCHECK > work, we have to pass a bool param saying whether the renderer caps actually > changed with the new OS, and only assert single ref if it did change. Thought > I'd get your opinion first before implementing this though. Maybe we can do this without adding more LayerAPI.. would it work to just SetNeedsUpdate on the tree when renderer caps change, and then in Layer::Update() it could decide to drop things as needed. This layer does not even use the RendererCapabilities, so maybe I'm not sure why it cares that they change even?
https://codereview.chromium.org/151093005/diff/650001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/151093005/diff/650001/cc/layers/texture_layer... cc/layers/texture_layer.cc:281: holder_ref_.reset(); On 2014/02/14 17:59:11, danakj wrote: > On 2014/02/13 20:14:39, boliu wrote: > > I removed the DCHECK for now. This broke context_lost test on the gpu bots. > > > > The issue here is that in a chrome context loss, it recreates another renderer > > with the same RendererCapabilities. So there is nothing stopping a new commit > > going through, even if the context is lost. > > > > I still think it's correct here to drop the ref here. But to make the DCHECK > > work, we have to pass a bool param saying whether the renderer caps actually > > changed with the new OS, and only assert single ref if it did change. Thought > > I'd get your opinion first before implementing this though. > > Maybe we can do this without adding more LayerAPI.. would it work to just > SetNeedsUpdate on the tree when renderer caps change, and then in > Layer::Update() it could decide to drop things as needed. This layer does not > even use the RendererCapabilities, so maybe I'm not sure why it cares that they > change even? RendererCapabilities is used in the layer_tree_host()->UsingSharedMemoryResources(). I think checking if that changed in Update works as well. Trying to add a test for it now.
PTAL Now TextureLayer::Update checks if UsingSharedMemoryResources changed, and release old mailbox if it did.
https://codereview.chromium.org/151093005/diff/740001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/layers/texture_layer... cc/layers/texture_layer.cc:228: holder_ref_.reset(); Now that we're here, I wonder.. the call to PrepareTextureMailbox will tell the client about UsingSharedMemoryResources. If they set a new (or empty) TM as a result, it'll reset the holder ref. So what is this buying us? Is it saving us redundant code in other places? https://codereview.chromium.org/151093005/diff/740001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:162: void SingleThreadProxy::OnOutputSurfaceInitializeAttempted(bool success) { The "main vs impl" side stuff in here is kinda messed up. It should match the thread proxy a bit more. I think you were right about the UpdateRendererCapabilitiesOnImplThread() method in your original question about NOTREACHED(). This method is on the main thread side so it won't call an OnImplThread method directly. Two ways to solve this: 1) Pass the caps in to this method here, collecting them in the scoped impl code above (line ~155). Call the LTH directly from this method instead of calling UpdateRCapsOnImplThread(). or 2) DebugScopedImpl yourself here to call UpdateRCapsOnImplThread(). Then inside that method DebugScopedMain yourself back to the main side to call into the LTH. 1 seems a bit simpler if you don't care about supporting the UpdateRCapsOnImplThread() method here, which I think you don't. So maybe just a NOTREACHED in there is best afterall. https://codereview.chromium.org/151093005/diff/740001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:165: if (success) { nit: no {} https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1022: I think if we're going to abort the commit we need to do it before we start setting state on the impl side. Otherwise we aborting half way thru a commit and we are going to put ourselves into some very weird situations. Doing it after we've done anything in this method sounds scary to me. WDYT?
https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1022: On 2014/02/14 19:38:50, danakj wrote: > I think if we're going to abort the commit we need to do it before we start > setting state on the impl side. Otherwise we aborting half way thru a commit and > we are going to put ourselves into some very weird situations. Doing it after > we've done anything in this method sounds scary to me. WDYT? Unfortunately putting the check here is not going to work :( Commit is not synchronous on impl thread, so if RendererCap changes in the middle of a commit (ie after StartCommitOnImplThread but before ScheduledActionCommit), then the commit will incorrectly go through. I don't really have a solution other than being really careful with the state here.
https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1022: On 2014/02/14 19:48:53, boliu wrote: > On 2014/02/14 19:38:50, danakj wrote: > > I think if we're going to abort the commit we need to do it before we start > > setting state on the impl side. Otherwise we aborting half way thru a commit > and > > we are going to put ourselves into some very weird situations. Doing it after > > we've done anything in this method sounds scary to me. WDYT? > > Unfortunately putting the check here is not going to work :( > > Commit is not synchronous on impl thread, so if RendererCap changes in the > middle of a commit (ie after StartCommitOnImplThread but before > ScheduledActionCommit), then the commit will incorrectly go through. Is there still a good reason to be not synchronous with impl side painting? We dont have to upload textures here anymore since we do that in the pending tree. So maybe we could change this? > I don't really have a solution other than being really careful with the state > here. I think I'd prefer we commit stuff that doesn't match the renderer and degrade that gracefully (max texture too large? can't draw it sorry) than commit half of the data and leave the impl side in a weird state. WDYT?
https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1022: On 2014/02/14 19:53:25, danakj wrote: > On 2014/02/14 19:48:53, boliu wrote: > > On 2014/02/14 19:38:50, danakj wrote: > > > I think if we're going to abort the commit we need to do it before we start > > > setting state on the impl side. Otherwise we aborting half way thru a commit > > and > > > we are going to put ourselves into some very weird situations. Doing it > after > > > we've done anything in this method sounds scary to me. WDYT? > > > > Unfortunately putting the check here is not going to work :( > > > > Commit is not synchronous on impl thread, so if RendererCap changes in the > > middle of a commit (ie after StartCommitOnImplThread but before > > ScheduledActionCommit), then the commit will incorrectly go through. > > Is there still a good reason to be not synchronous with impl side painting? We > dont have to upload textures here anymore since we do that in the pending tree. > So maybe we could change this? I was wondering about that. So we can skip this whole ResourceUpdateController thing for impl-side painting? Then I can try make that change and see if anything breaks. > > > I don't really have a solution other than being really careful with the state > > here. > > I think I'd prefer we commit stuff that doesn't match the renderer and degrade > that gracefully (max texture too large? can't draw it sorry) than commit half of > the data and leave the impl side in a weird state. WDYT? Actually I don't know anything that will break in practice. Webview did launch with never updating the renderer caps from SoftwareRenderer, but then again, we didn't have accelerated canvas. Let me try removing ResourceUpdateController for impl-side painting first. If that blows up in my face, then I can cross my fingers and hope webview will be ok with bad render caps for one commit, and change the cancel commit to set commit instead.
https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1878: client_->UpdateRendererCapabilitiesOnImplThread(); This seems like the wrong place to do this. Can you do this in CreateAndSetRenderer? If you move it there, then you don't need to do anything in SingleThreadProxy either. https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1414: const RendererCapabilitiesImpl& impl_caps = I don't quite follow what sorts of caps aren't supported by the commit, or what problems come from the main thread preparing a frame using the wrong set of caps. What problems occur when you don't abort the commit? In general, I agree with danakj that it'd be better to just fail gracefully if that's possible. If there are problems, what happens if the caps change between commit and activation? Isn't the successfully committed pending tree also equally wrong?
So just tried calling Scheduler::FinishCommit from ThreadProxy::StartCommitOnImplThread if impl-side painting is on. Clank *seems* to work fine, but ~40 cc_unittests failed. And I don't really have high confidence such a change won't break other things. And since both of you suggested just letting the commit through, and webview has been working fine all this time doing exactly that, I'll just remove all the cancel commit code. Expect a much smaller patch soon :) https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1414: const RendererCapabilitiesImpl& impl_caps = On 2014/02/14 20:21:48, enne wrote: > I don't quite follow what sorts of caps aren't supported by the commit, or what > problems come from the main thread preparing a frame using the wrong set of > caps. What problems occur when you don't abort the commit? In general, I agree > with danakj that it'd be better to just fail gracefully if that's possible. I thought it might be a problem. But I don't know any in practice. > > If there are problems, what happens if the caps change between commit and > activation? Isn't the successfully committed pending tree also equally wrong? Both pending and active trees are on impl side, and we already clean them up synchronously. All this dance is just to cover the edge case that a commit is already pending when render caps change.
Removed the cancel commit code and addressed other comments. PTAL https://codereview.chromium.org/151093005/diff/740001/cc/layers/texture_layer.cc File cc/layers/texture_layer.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/layers/texture_layer... cc/layers/texture_layer.cc:228: holder_ref_.reset(); On 2014/02/14 19:38:50, danakj wrote: > Now that we're here, I wonder.. the call to PrepareTextureMailbox will tell the > client about UsingSharedMemoryResources. If they set a new (or empty) TM as a > result, it'll reset the holder ref. So what is this buying us? Is it saving us > redundant code in other places? Good point...removed all this code and just make the client a little bit smarter. https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1878: client_->UpdateRendererCapabilitiesOnImplThread(); On 2014/02/14 20:21:48, enne wrote: > This seems like the wrong place to do this. Can you do this in > CreateAndSetRenderer? If you move it there, then you don't need to do anything > in SingleThreadProxy either. UpdateRendererCapabilitiesOnImplThread is only used for DeferredInitialize and ReleaseGL (ie the webview methods). It's updated synchronously for normal compositor init. Yes, this is a second path just for webview. :( Dana proposed adding OutputSurfaceProvider, move the logic of creating the OS to the embedder, and converge more on this path. But not there yet. https://codereview.chromium.org/151093005/diff/740001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:162: void SingleThreadProxy::OnOutputSurfaceInitializeAttempted(bool success) { On 2014/02/14 19:38:50, danakj wrote: > The "main vs impl" side stuff in here is kinda messed up. It should match the > thread proxy a bit more. I think you were right about the > UpdateRendererCapabilitiesOnImplThread() method in your original question about > NOTREACHED(). This method is on the main thread side so it won't call an > OnImplThread method directly. > > Two ways to solve this: > > 1) Pass the caps in to this method here, collecting them in the scoped impl code > above (line ~155). Call the LTH directly from this method instead of calling > UpdateRCapsOnImplThread(). > > or > > 2) DebugScopedImpl yourself here to call UpdateRCapsOnImplThread(). Then inside > that method DebugScopedMain yourself back to the main side to call into the LTH. > > > 1 seems a bit simpler if you don't care about supporting the > UpdateRCapsOnImplThread() method here, which I think you don't. So maybe just a > NOTREACHED in there is best afterall. 1) it is. Done https://codereview.chromium.org/151093005/diff/740001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:165: if (success) { On 2014/02/14 19:38:50, danakj wrote: > nit: no {} Done.
https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1878: client_->UpdateRendererCapabilitiesOnImplThread(); On 2014/02/14 22:03:08, boliu wrote: > On 2014/02/14 20:21:48, enne wrote: > > This seems like the wrong place to do this. Can you do this in > > CreateAndSetRenderer? If you move it there, then you don't need to do anything > > in SingleThreadProxy either. > > UpdateRendererCapabilitiesOnImplThread is only used for DeferredInitialize and > ReleaseGL (ie the webview methods). It's updated synchronously for normal > compositor init. > > Yes, this is a second path just for webview. :( Can there be only one?
https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1878: client_->UpdateRendererCapabilitiesOnImplThread(); On 2014/02/14 22:08:49, enne wrote: > On 2014/02/14 22:03:08, boliu wrote: > > On 2014/02/14 20:21:48, enne wrote: > > > This seems like the wrong place to do this. Can you do this in > > > CreateAndSetRenderer? If you move it there, then you don't need to do > anything > > > in SingleThreadProxy either. > > > > UpdateRendererCapabilitiesOnImplThread is only used for DeferredInitialize and > > ReleaseGL (ie the webview methods). It's updated synchronously for normal > > compositor init. > > > > Yes, this is a second path just for webview. :( > > Can there be only one? No...webview has the crazy requirement to synchronously initialize/tear down GL on compositor thread.
On 2014/02/14 22:14:49, boliu wrote: > https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:1878: > client_->UpdateRendererCapabilitiesOnImplThread(); > On 2014/02/14 22:08:49, enne wrote: > > On 2014/02/14 22:03:08, boliu wrote: > > > On 2014/02/14 20:21:48, enne wrote: > > > > This seems like the wrong place to do this. Can you do this in > > > > CreateAndSetRenderer? If you move it there, then you don't need to do > > anything > > > > in SingleThreadProxy either. > > > > > > UpdateRendererCapabilitiesOnImplThread is only used for DeferredInitialize > and > > > ReleaseGL (ie the webview methods). It's updated synchronously for normal > > > compositor init. > > > > > > Yes, this is a second path just for webview. :( > > > > Can there be only one? > > No...webview has the crazy requirement to synchronously initialize/tear down GL > on compositor thread. Well, there can be if we replace the OutputSurface instead of letting it change modes on webview.
lgtm https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1878: client_->UpdateRendererCapabilitiesOnImplThread(); On 2014/02/14 22:14:49, boliu wrote: > On 2014/02/14 22:08:49, enne wrote: > > On 2014/02/14 22:03:08, boliu wrote: > > > On 2014/02/14 20:21:48, enne wrote: > > > > This seems like the wrong place to do this. Can you do this in > > > > CreateAndSetRenderer? If you move it there, then you don't need to do > > anything > > > > in SingleThreadProxy either. > > > > > > UpdateRendererCapabilitiesOnImplThread is only used for DeferredInitialize > and > > > ReleaseGL (ie the webview methods). It's updated synchronously for normal > > > compositor init. > > > > > > Yes, this is a second path just for webview. :( > > > > Can there be only one? > > No...webview has the crazy requirement to synchronously initialize/tear down GL > on compositor thread. Sure, but the caps can still be sent back the same way in both paths?
Er, did not mean to lgtm that. Mis-mouseclick. :P
The CQ bit was unchecked by enne@chromium.org
https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1878: client_->UpdateRendererCapabilitiesOnImplThread(); On 2014/02/14 22:19:39, enne wrote: > Sure, but the caps can still be sent back the same way in both paths? So InitializeRenderer has main thread blocked and can update caps synchronously. I don't want to change that behavior to post back to main thread in this patch. That being said, we could make it look a bit more consistent in LTHI, and decide whether to post task or copy in the proxy, depending on if main thread is blocked. But honestly that feels worse than the two explicit paths... WDYT?
https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/151093005/diff/740001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1878: client_->UpdateRendererCapabilitiesOnImplThread(); On 2014/02/14 22:27:30, boliu wrote: > On 2014/02/14 22:19:39, enne wrote: > > Sure, but the caps can still be sent back the same way in both paths? > > So InitializeRenderer has main thread blocked and can update caps synchronously. > I don't want to change that behavior to post back to main thread in this patch. Maybe it's just me, but I would rather have CreateAndSetRenderer always send back caps. If the main thread is synchronously doing an InitializeRenderer, then it can also cache the caps itself locally and will get a redundant copy later. You could put a comment on the main thread side about this, since it's my opinion that this is the deprecated path and in some shiny future the compositor thread will be the initiating party for creating the output surface.
Ok, did the change to call UpdateRendererCapabilitiesOnImplThread in CreateAndSetRenderer. Please take a glance to see if it's ok. One thing fell out: that s/OutputSurfaceCreated/RendererCapabilitiesChanged/ doesn't make sense, at least for now. Currently it really is use to drop resources on the main tree, which really should only happen once per OS change, so need to early out of the posted update if nothing changed. But in chrome, recreating the OS recreates the same renderer, so does not change the renderer caps, so can't do the same early out in the synchronous path... What we really need in the future is is something like OutputSurfaceCreated(bool renderer_capabilities_changed)
Thanks. lgtm % danakj
https://codereview.chromium.org/151093005/diff/940002/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/940002/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:374: layer_tree_host_->RendererCapabilitiesChanged(); Should this check the previous ones and only call if it has changed?
https://codereview.chromium.org/151093005/diff/940002/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/940002/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:374: layer_tree_host_->RendererCapabilitiesChanged(); On 2014/02/14 23:57:32, enne wrote: > Should this check the previous ones and only call if it has changed? This was explicit choice too. Webview doesn't run in single threaded mode, so this is only used in the synchronous path, where we definitely don't want the check.
https://codereview.chromium.org/151093005/diff/940002/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/151093005/diff/940002/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:374: layer_tree_host_->RendererCapabilitiesChanged(); On 2014/02/14 23:59:14, boliu wrote: > On 2014/02/14 23:57:32, enne wrote: > > Should this check the previous ones and only call if it has changed? > > This was explicit choice too. Webview doesn't run in single threaded mode, so > this is only used in the synchronous path, where we definitely don't want the > check. Why don't we want the check?
On 2014/02/15 00:00:14, enne wrote: > Why don't we want the check? See the chunk of text in #31. Don't want to check in synchronous path (in thread_proxy.cc too) since that's used by chrome which recreates the same renderer with the same caps, but main thread still need to drop resources since it's probably a context loss.
On 2014/02/15 00:04:53, boliu wrote: > On 2014/02/15 00:00:14, enne wrote: > > Why don't we want the check? > > See the chunk of text in #31. > > Don't want to check in synchronous path (in thread_proxy.cc too) since that's > used by chrome which recreates the same renderer with the same caps, but main > thread still need to drop resources since it's probably a context loss. Can you clean that up? It shouldn't be called RenderCapabilitiesChanged on the main thread if you need to call it every time the output surface was changed.
On 2014/02/15 00:07:22, enne wrote: > Can you clean that up? It shouldn't be called RenderCapabilitiesChanged on the > main thread if you need to call it every time the output surface was changed. Simplified a lot of a smaller patch. Not changing anything in the sync path or LTH anymore. There is no main layer cares about RenderCapChanged callback at the moment. So the RendererCapabilitiesUpdated call never needs to leave the proxy. Also no need to compare capabilities anymore. Interesting this got so small at the end, removed everything I thought that were going to be problems.
On 2014/02/15 00:27:42, boliu wrote: > Interesting this got so small at the end, removed everything I thought that were > going to be problems. I guess that's fail on my part :/
Thanks for the cleanup! This looks way better. :)
On 2014/02/15 00:36:07, enne wrote: > Thanks for the cleanup! This looks way better. :) Waiting on Dana :)
On 2014/02/15 00:43:41, boliu wrote: > On 2014/02/15 00:36:07, enne wrote: > > Thanks for the cleanup! This looks way better. :) > > Waiting on Dana :) Eager branch day ping :)
LGTM
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/boliu@chromium.org/151093005/1150001
Message was sent while issue was closed.
Change committed as 251853 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
