Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14772021:
cc::OutputSurfaceClient::InitializeForGL (Closed)
|
Created:
7 years, 7 months ago by boliu Modified:
7 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, jam, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc::OutputSurfaceClient::DeferredInitialize
SynchronousCompositorOutputSurface first starts in software only
mode, then cc::OutputSurfaceClient::InitializeForGL is called to
initialize the GL parts.
BUG=230197
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204771
Patch Set 1 #Patch Set 2 : Start in dummy software mode. Loads of code dup in cc. #Patch Set 3 : rebase #Patch Set 4 : Dedup and refactor code #
Total comments: 6
Patch Set 5 : Rebased onto r201804 #Patch Set 6 : Rebase onto r201857 (in the middle of refactor, do not review) #Patch Set 7 : cc refactors to reduce duplication #
Total comments: 4
Patch Set 8 : Reduce scope of change in cc #Patch Set 9 : cleanups #
Total comments: 4
Patch Set 10 : rebase, minor fix ups #Patch Set 11 : only InitializeForGL #Patch Set 12 : Renamed to DeferredInitialize. Added smoke tests. #
Total comments: 2
Patch Set 13 : Do not drop resources since coming from resourceless software mode #
Total comments: 9
Patch Set 14 : comments #
Total comments: 7
Patch Set 15 : fix test, revert scheduler change, DCHECK(!inside_draw_) #
Total comments: 1
Patch Set 16 : Do not update caps in DidTryInitializeRendererOnImplThread #
Total comments: 8
Patch Set 17 : Add swap callback dcheck, fix test #
Total comments: 1
Patch Set 18 : fix last nit #Patch Set 19 : rebase #Patch Set 20 : fix win compile by including ContextProvider from output_surface_client.h #Messages
Total messages: 66 (0 generated)
Currently, LTHI::InitilizeForGL is very similar to calling InitializeRenderer but with the same output surface. Then it passes the new capabilities to proxy and LTH. Are there things like ClearRenderSurfaces don't need to called in InitilizeForGL? Added a context provider param, but I'll leave actually using it in a follow up patch.
couple comments on the not-cc-internals parts of the change https://codereview.chromium.org/14772021/diff/8001/android_webview/browser/in... File android_webview/browser/in_process_renderer/in_process_view_renderer.cc (right): https://codereview.chromium.org/14772021/diff/8001/android_webview/browser/in... android_webview/browser/in_process_renderer/in_process_view_renderer.cc:201: hardware_initialized_ = true; lets rename these two memebers: hardware_initialized_ => hardware_initialization_attempted_ hardware_failed_ => hardware_initialized_ hmmm that makes the logic in PrepareDrawGL more complicated. How about a tristate? HwInitState { HWINIT_UNATTEMPTED, HWINIT_SUCCEEDED, HWINIT_FAILED } https://codereview.chromium.org/14772021/diff/8001/android_webview/browser/in... android_webview/browser/in_process_renderer/in_process_view_renderer.cc:207: guess we could DCHECK(!hardware_failed_) here https://codereview.chromium.org/14772021/diff/8001/cc/output/output_surface_c... File cc/output/output_surface_client.h (right): https://codereview.chromium.org/14772021/diff/8001/cc/output/output_surface_c... cc/output/output_surface_client.h:22: // committed. // Will only be called by OutputSurfaces that hold the deferred_gl_initialization capability ? https://codereview.chromium.org/14772021/diff/8001/content/renderer/android/s... File content/renderer/android/synchronous_compositor_output_surface.cc (right): https://codereview.chromium.org/14772021/diff/8001/content/renderer/android/s... content/renderer/android/synchronous_compositor_output_surface.cc:7: #include "base/command_line.h" remove me?
I'm not so sure about making a new "DidUpdateCaps" path like this. When the caps change, the max texture size changes, and maybe we need to destroy some textures etc. We already have the context loss path that deals with changing caps correctly. Maybe you want to hook into that? If you delete/recreate the resource provider, you are going to destroy all the software resources we had already anyway. This sounds a lot like lost output surface. Alternatively, when we recreate the context, we have "updated caps" and we should use the same code path to set up everything. If we have both a hardware and software renderer, we actually have 2 sets of caps, do we not? Which set of caps is the one being updated and made available on the main thread? Does the GL one completely replace the software one? That's more or less how things will behave if you make a new resource provider. https://codereview.chromium.org/14772021/diff/8001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/14772021/diff/8001/cc/trees/layer_tree_host.c... cc/trees/layer_tree_host.cc:193: // renderer. weird indenting going on here https://codereview.chromium.org/14772021/diff/8001/cc/trees/layer_tree_host.c... cc/trees/layer_tree_host.cc:206: if (!contents_texture_manager_) { i don't really get why this would belong here, this happens when we create and init the first output surface, right?
On 21 May 2013 07:00, <danakj@chromium.org> wrote: > I'm not so sure about making a new "DidUpdateCaps" path like this. When > the caps > change, the max texture size changes, and maybe we need to destroy some > textures > etc. > > We already have the context loss path that deals with changing caps > correctly. > Maybe you want to hook into that? > > If you delete/recreate the resource provider, you are going to destroy all > the > software resources we had already anyway. This sounds a lot like lost > output > surface. > > Alternatively, when we recreate the context, we have "updated caps" and we > should use the same code path to set up everything. > > If we have both a hardware and software renderer, we actually have 2 sets > of > caps, do we not? Which set of caps is the one being updated and made > available > on the main thread? > The intent is that add GL will cause a transition from 0 renderers to 1 renderer. So, from "no" caps to "some" caps. When we're in the 0 renderer mode, we have no resource_provider, no tile_manager, and SW rendering occurs using tile-free rendering using a temporary SW renderer:- https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... > Does the GL one completely replace the software one? That's more or less > how > things will behave if you make a new resource provider. > > https://codereview.chromium.**org/14772021/diff/8001/cc/** > trees/layer_tree_host.cc<https://codereview.chromium.org/14772021/diff/8001/cc/trees/layer_tree_host.cc> > File cc/trees/layer_tree_host.cc (right): > > https://codereview.chromium.**org/14772021/diff/8001/cc/** > trees/layer_tree_host.cc#**newcode193<https://codereview.chromium.org/14772021/diff/8001/cc/trees/layer_tree_host.cc#newcode193> > cc/trees/layer_tree_host.cc:**193: // renderer. > weird indenting going on here > > https://codereview.chromium.**org/14772021/diff/8001/cc/** > trees/layer_tree_host.cc#**newcode206<https://codereview.chromium.org/14772021/diff/8001/cc/trees/layer_tree_host.cc#newcode206> > cc/trees/layer_tree_host.cc:**206: if (!contents_texture_manager_) { > i don't really get why this would belong here, this happens when we > create and init the first output surface, right? > > https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >
On Tue, May 21, 2013 at 1:23 PM, Jonathan Dixon <joth@chromium.org> wrote: > > > > On 21 May 2013 07:00, <danakj@chromium.org> wrote: > >> I'm not so sure about making a new "DidUpdateCaps" path like this. When >> the caps >> change, the max texture size changes, and maybe we need to destroy some >> textures >> etc. >> >> We already have the context loss path that deals with changing caps >> correctly. >> Maybe you want to hook into that? >> >> If you delete/recreate the resource provider, you are going to destroy >> all the >> software resources we had already anyway. This sounds a lot like lost >> output >> surface. >> >> Alternatively, when we recreate the context, we have "updated caps" and we >> should use the same code path to set up everything. >> >> If we have both a hardware and software renderer, we actually have 2 sets >> of >> caps, do we not? Which set of caps is the one being updated and made >> available >> on the main thread? >> > > The intent is that add GL will cause a transition from 0 renderers to 1 > renderer. So, from "no" caps to "some" caps. > When we're in the 0 renderer mode, we have no resource_provider, no > tile_manager, and SW rendering occurs using tile-free rendering using a > temporary SW renderer:- > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... > Ah okay thanks. This still sounds a lot like "InitializedFirstContext" then, and it would be nice to reuse existing code paths. In other words, defer current initialization paths as needed, then run them when we have an output surface we can use for rendering. Rather than adding a new 2nd set of later initialization paths? WDYT? > > > > >> Does the GL one completely replace the software one? That's more or less >> how >> things will behave if you make a new resource provider. >> > >> https://codereview.chromium.**org/14772021/diff/8001/cc/** >> trees/layer_tree_host.cc<https://codereview.chromium.org/14772021/diff/8001/cc/trees/layer_tree_host.cc> >> File cc/trees/layer_tree_host.cc (right): >> >> https://codereview.chromium.**org/14772021/diff/8001/cc/** >> trees/layer_tree_host.cc#**newcode193<https://codereview.chromium.org/14772021/diff/8001/cc/trees/layer_tree_host.cc#newcode193> >> cc/trees/layer_tree_host.cc:**193: // renderer. >> weird indenting going on here >> >> https://codereview.chromium.**org/14772021/diff/8001/cc/** >> trees/layer_tree_host.cc#**newcode206<https://codereview.chromium.org/14772021/diff/8001/cc/trees/layer_tree_host.cc#newcode206> >> cc/trees/layer_tree_host.cc:**206: if (!contents_texture_manager_) { >> i don't really get why this would belong here, this happens when we >> create and init the first output surface, right? >> >> https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >> > >
On 2013/05/21 17:34:20, danakj wrote: > Ah okay thanks. This still sounds a lot like "InitializedFirstContext" > then, and it would be nice to reuse existing code paths. In other words, > defer current initialization paths as needed, then run them when we have an > output surface we can use for rendering. Rather than adding a new 2nd set > of later initialization paths? WDYT? The major difference with normal initialization is that "InitializeForGL" is triggered on the impl thread and needs to be synchronous. Also for the forced draw to sw, it still needs normal initialization path to set the output surface.
On Tue, May 21, 2013 at 2:11 PM, <boliu@chromium.org> wrote: > On 2013/05/21 17:34:20, danakj wrote: > >> Ah okay thanks. This still sounds a lot like "InitializedFirstContext" >> then, and it would be nice to reuse existing code paths. In other words, >> defer current initialization paths as needed, then run them when we have >> an >> output surface we can use for rendering. Rather than adding a new 2nd set >> of later initialization paths? WDYT? >> > > The major difference with normal initialization is that "InitializeForGL" > is > triggered on the impl thread and needs to be synchronous. > > Also for the forced draw to sw, it still needs normal initialization path > to set > the output surface. > Do we need to split up "initialized surface" and "initialized renderer" a little better then? After a context loss, when we've got a renderer again, we're going to want to do all the things you'll also want to do when this InitializeForGL happens I think, right? As we both know initialization code is super tricky and often source of bugs, so if we can reduce (or not increase) the number of potential paths that'd be a huge win IMO > > https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >
On 2013/05/21 18:31:05, danakj wrote: > Do we need to split up "initialized surface" and "initialized renderer" a > little better then? > > After a context loss, when we've got a renderer again, we're going to want > to do all the things you'll also want to do when this InitializeForGL > happens I think, right? Yep > As we both know initialization code is super tricky > and often source of bugs, so if we can reduce (or not increase) the number > of potential paths that'd be a huge win IMO So I can invert the call stack, basically have LTHI::InitializeRenderer call up to proxy to do all these things that is currently after LHTI::InitializeRenderer, then LTHI have the choice to call this later in the case of webview. Sounds better? One weird thing is path can directly call into LTH in normal initialization since main thread is blocked, but needs to be a post task for webview/delayed init.
On 2013/05/21 19:00:33, boliu wrote: > On 2013/05/21 18:31:05, danakj wrote: > > Do we need to split up "initialized surface" and "initialized renderer" a > > little better then? > > > > After a context loss, when we've got a renderer again, we're going to want > > to do all the things you'll also want to do when this InitializeForGL > > happens I think, right? > > Yep > > > As we both know initialization code is super tricky > > and often source of bugs, so if we can reduce (or not increase) the number > > of potential paths that'd be a huge win IMO > > So I can invert the call stack, basically have LTHI::InitializeRenderer call up > to proxy to do all these things that is currently after > LHTI::InitializeRenderer, then LTHI have the choice to call this later in the > case of webview. Sounds better? I think that sounds about right. But the call to InitializeRenderer is coming from the proxy anyways, so maybe you don't need to have a cyclical call Proxy->LTHI->Proxy to do this. > One weird thing is path can directly call into LTH in normal initialization > since main thread is blocked, but needs to be a post task for webview/delayed > init. Can it first block the main thread before it gets this far into the init process?
Back to looking at this one now. On 2013/05/21 20:22:06, danakj wrote: > > One weird thing is path can directly call into LTH in normal initialization > > since main thread is blocked, but needs to be a post task for webview/delayed > > init. > > Can it first block the main thread before it gets this far into the init > process? Not possible I think. The plan is to call this InitializeForGL when webview is attached to the hardware view tree and we finally can get a real GL context, and this can happen at any time and is out of webview's control. It also needs to be synchronous because we could call hw draw immediately after. So I can't see how this is possible since blocking ui/compositor on main thread is almost certainly immediate deadlock. Good thing is LTHI::DeleteContentsTexturesOnImplThread is the only thing that needs both threads blocked, and it doesn't need to be called for InitializeForGL. This does mean InitializeForGL can't be re-used for the gl context lost path. But losing draws here is less bad, so my plan for that is to actually go through the normal context loss path, and just draw white before we get the new context initialized.
Dana: PTAL Tried invert the call after LTHI::InitializeRenderer, but couldn't get all the way. There are code related to setting output_surface_lost state, deciding on whether to retry context creation, etc, that are not relevant for InitializeForGL. I could rewrite them to make sure they don't break when called randomly, but there are nice properties like OnCreateAndInitializeOutputSurfaceAttempted is only called when output_surface_lost that will be lost. Thoughts? I ran this through clang-format, so there might be spurious format changes. Also only made cc changes since last review, no need to look at other parts.
On 2013/05/24 02:36:36, boliu wrote: > Dana: PTAL > > Tried invert the call after LTHI::InitializeRenderer, but couldn't get all the > way. There are code related to setting output_surface_lost state, deciding on > whether to retry context creation, etc, that are not relevant for > InitializeForGL. I could rewrite them to make sure they don't break when called > randomly, but there are nice properties like > OnCreateAndInitializeOutputSurfaceAttempted is only called when > output_surface_lost that will be lost. Thoughts? I feel like this CL needs to decide what it is trying to implement/support. Or I need more clarification and understanding. For instance, it's making changes to single thread proxy and LTH in order to support non-impl-side painting. But if it doesn't go through all the lost output surface code, it won't work anyways. It's like it updates some state on the main thread, but doesn't cause anything to actually use the new state. I think that this should work without doing the lost-context stuff for impl-side painting (for picture layers!) because reinit the renderer does reinit the tile manager. But what about layer types that aren't picture layers? Maybe the video layer is producing software bitmaps because we're in software mode. Now you recreate the renderer but don't tell it "output surface lost", it could decide to keep sending the same software bitmap quads which would be the wrong behaviour, it should drop them and make GL backed quads instead. I think right now in the "pre-GL software mode" you don't have a resource provider set up for software mode or anything, so basically only picture layers will work at all. Is that the end goal? You're right that "lost" isn't really the right term here. There are really two ways we could tell layers that they need to remake their resources for a new output mode: (1) Lost output surface -> your resources have all been destroyed. (2) Switching to new output mode/have new renderer -> you need to remake all your resources for the current output surface's behaviour + renderer caps. If you want more than picture layers to work, you'll want to signal (2) here to all layers, main thread or impl, but not (1) cuz they actually need to clean up after themselves. Do you want to eventually support non-picture layers? If not, what should happen when they exist? WDYT? > I ran this through clang-format, so there might be spurious format changes. > > Also only made cc changes since last review, no need to look at other parts.
Venturing into things beyond my knowledge On 2013/05/24 16:04:23, danakj wrote: > I feel like this CL needs to decide what it is trying to implement/support. Or I > need more clarification and understanding. > > For instance, it's making changes to single thread proxy and LTH in order to > support non-impl-side painting. NOTREACHED in single thread proxy then? > But if it doesn't go through all the lost output > surface code, it won't work anyways. Which bit of output surface lost code are you referring to? ThreadProxy::CheckOutputSurfaceStatusOnImplThread looks safe to leave out since there is no offscreen context in software mode. Must be something else I missed? > It's like it updates some state on the main > thread, but doesn't cause anything to actually use the new state. My understanding is the new settings will be picked up in the next commit/draw. Is this not the case? > > I think that this should work without doing the lost-context stuff for impl-side > painting (for picture layers!) because reinit the renderer does reinit the tile > manager. > > But what about layer types that aren't picture layers? Maybe the video layer is > producing software bitmaps because we're in software mode. Now you recreate the > renderer but don't tell it "output surface lost", it could decide to keep > sending the same software bitmap quads which would be the wrong behaviour, it > should drop them and make GL backed quads instead. I think right now in the > "pre-GL software mode" you don't have a resource provider set up for software > mode or anything, so basically only picture layers will work at all. Is that the > end goal? Alex is better person to answer about software mode. This does sound scary though. My understanding is that software mode is supposed to be stateless; even after hardware init, we can still request software draws interleaves with regular hardware draws. Also I think there are things expected not to work software and video is one of them (I assume video layer == <video> tag?) > > You're right that "lost" isn't really the right term here. There are really two > ways we could tell layers that they need to remake their resources for a new > output mode: > (1) Lost output surface -> your resources have all been destroyed. > (2) Switching to new output mode/have new renderer -> you need to remake all > your resources for the current output surface's behaviour + renderer caps. > > If you want more than picture layers to work, you'll want to signal (2) here to > all layers, main thread or impl, but not (1) cuz they actually need to clean up > after themselves. > So I'm guessing (1) is LHTI::SendDidLoseOutputSurfaceRecursive? It is called on InitializeForGL. But sounds like this maybe destroying too much, 2? I didn't find something equivalent for the layers on main thread though. > Do you want to eventually support non-picture layers? If not, what should happen > when they exist? WDYT?
On Fri, May 24, 2013 at 1:19 PM, <boliu@chromium.org> wrote: > Venturing into things beyond my knowledge > > > On 2013/05/24 16:04:23, danakj wrote: > >> I feel like this CL needs to decide what it is trying to >> implement/support. Or >> > I > >> need more clarification and understanding. >> > > For instance, it's making changes to single thread proxy and LTH in order >> to >> support non-impl-side painting. >> > > NOTREACHED in single thread proxy then? I think that'd be appropriate if it's only for impl painting. Also some DCHECKs that impl painting is on in thread proxy? > > > But if it doesn't go through all the lost output >> surface code, it won't work anyways. >> > > Which bit of output surface lost code are you referring to? > ThreadProxy::**CheckOutputSurfaceStatusOnImpl**Thread looks safe to leave > out since > there is no offscreen context in software mode. Must be something else I > missed? > > > It's like it updates some state on the main >> thread, but doesn't cause anything to actually use the new state. >> > > My understanding is the new settings will be picked up in the next > commit/draw. > Is this not the case? See thread proxy's call to layer_tree_host_->DeleteContentsTexturesOnImplThread(layer_tree_host_impl_->resource_provider()); in ThreadProxy::InitializeOutputSurfaceOnImplThread. This drops resources for main thread content layers. > > > > I think that this should work without doing the lost-context stuff for >> > impl-side > >> painting (for picture layers!) because reinit the renderer does reinit the >> > tile > >> manager. >> > > But what about layer types that aren't picture layers? Maybe the video >> layer >> > is > >> producing software bitmaps because we're in software mode. Now you >> recreate >> > the > >> renderer but don't tell it "output surface lost", it could decide to keep >> sending the same software bitmap quads which would be the wrong >> behaviour, it >> should drop them and make GL backed quads instead. I think right now in >> the >> "pre-GL software mode" you don't have a resource provider set up for >> software >> mode or anything, so basically only picture layers will work at all. Is >> that >> > the > >> end goal? >> > > Alex is better person to answer about software mode. This does sound scary > though. My understanding is that software mode is supposed to be > stateless; even > after hardware init, we can still request software draws interleaves with > regular hardware draws. > > Also I think there are things expected not to work software and video is > one of > them (I assume video layer == <video> tag?) Yeh. Also to consider are texture layers (flash etc), scrollbar layers (on non-android these have resources on the main thread still), the HUD layer, delegated renderer layers (OOP iframes, <webview>). I think the HUD will just not work right now before there's a resource provider, and probably will crash. Even not working sounds like a non-ideal situation. > > You're right that "lost" isn't really the right term here. There are >> really >> > two > >> ways we could tell layers that they need to remake their resources for a >> new >> output mode: >> (1) Lost output surface -> your resources have all been destroyed. >> (2) Switching to new output mode/have new renderer -> you need to remake >> all >> your resources for the current output surface's behaviour + renderer caps. >> > > If you want more than picture layers to work, you'll want to signal (2) >> here >> > to > >> all layers, main thread or impl, but not (1) cuz they actually need to >> clean >> > up > >> after themselves. >> > > > So I'm guessing (1) is LHTI::**SendDidLoseOutputSurfaceRecurs**ive? It is > called on > InitializeForGL. But sounds like this maybe destroying too much, 2? > Ah, right okay so we're doing the SendDidLose part to the impl layers, that's good. But they need to clean up things not just drop them on the floor/leak then.. I think picture layers are fine here again. Video and HUD look okay atm. TextureLayerImpl would not be though right now, it just sets things to 0, for example. It's also a little inconsistent to tell the layers "Lost context" but not the proxy. It stays reasonably sane at first, but I worry about things getting more weird in the future.. > > I didn't find something equivalent for the layers on main thread though. > > > Do you want to eventually support non-picture layers? If not, what should >> > happen > >> when they exist? WDYT? >> > > https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >
On 2013/05/24 17:36:33, danakj wrote: > > See thread proxy's call to > layer_tree_host_->DeleteContentsTexturesOnImplThread(layer_tree_host_impl_->resource_provider()); > in ThreadProxy::InitializeOutputSurfaceOnImplThread. This drops resources > for main thread content layers. > I was totally under the impression that this was not needed for InitializeForGL, given there was no resource provider before, and PrioritizedResourceManager::ClearAllMemory has an early return for null resource provider. What did I miss? I'm actually wondering if we need to create LTH::contents_texture_manager_ for this pre-gl software mode... > Ah, right okay so we're doing the SendDidLose part to the impl layers, > that's good. But they need to clean up things not just drop them on the > floor/leak then.. I think picture layers are fine here again. Video and HUD > look okay atm. TextureLayerImpl would not be though right now, it just sets > things to 0, for example. > > It's also a little inconsistent to tell the layers "Lost context" but not > the proxy. It stays reasonably sane at first, but I worry about things > getting more weird in the future.. > So add LayerImpl::DidUpdateCapbilities and go through each layer subclass. Might need a better name though :/
On Fri, May 24, 2013 at 2:00 PM, <boliu@chromium.org> wrote: > On 2013/05/24 17:36:33, danakj wrote: > > See thread proxy's call to >> > > layer_tree_host_->**DeleteContentsTexturesOnImplTh** > read(layer_tree_host_impl_->**resource_provider()); > >> in ThreadProxy::**InitializeOutputSurfaceOnImplT**hread. This drops >> resources >> for main thread content layers. >> > > > I was totally under the impression that this was not needed for > InitializeForGL, > given there was no resource provider before, and > PrioritizedResourceManager::**ClearAllMemory has an early return for null > resource > provider. What did I miss? > I guess that's true. Maybe I need to wrap my head around this concept of "the compositor is producing frames but has no resource provider etc". It seems like the output for this state will only work for a very limited set of things, I'm not super thrilled about it. I'd like this path to also be more-or-less the same way we fall back from GL mode to Software mode when the GL context can't be reinitialized, because they are going to be super similar. And in that case, you will have a RP, and you will have to drop resources on the main thread to make it recreate them in software mode. Do you think it's going too far to try build this in a way that makes sense for the GL->Software transition as well? It would definitely be easier to think about this only for the webview "software without RP -> hardware with RP" case.. but then we'll have to rewrite it later to merge these paths, I think? > I'm actually wondering if we need to create LTH::contents_texture_manager_ > for > this pre-gl software mode... There's definitely some things that won't work, which may be okay for android but it's not a very general solution at all. > > > Ah, right okay so we're doing the SendDidLose part to the impl layers, >> that's good. But they need to clean up things not just drop them on the >> floor/leak then.. I think picture layers are fine here again. Video and >> HUD >> look okay atm. TextureLayerImpl would not be though right now, it just >> sets >> things to 0, for example. >> > > It's also a little inconsistent to tell the layers "Lost context" but not >> the proxy. It stays reasonably sane at first, but I worry about things >> getting more weird in the future.. >> > > > So add LayerImpl::**DidUpdateCapbilities and go through each layer > subclass. Might > need a better name though :/ We could also just have the DidLoseOS() call on each layer actually clean up its resources instead of just setting them to 0, and rename this method. (aside, I'd rather DropAllResources() or DropResourcesForOutputSurfaceMode() something.. It has more to do with the software/hardware change than to do with the capabilities - well.. I guess that is like a capability but its not part of the RendererCaps struct. I feel like "DidUpdateCapabilities" could want to check "am i still less than max texture size? early out!". Anyway, naming is kinda nitty at this point.) > > https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >
I think a bit of thought needs to go into the "we have no ResourceProvider in this particular software mode" decision. Currently a lot of places are going to crash if you have more than just PictureLayers in the tree. And a lot of assumptions are being broken. I wish there was a way to do the synchronous init to GL and still involve the main thread in some way, maybe requiring some different threading primitives like locks or cond vars to accomplish this, I'm not sure. Maybe it's not possible. If we're going to go ahead without a RP in this mode, maybe we can have checks and prevent other layer types from appearing in the tree? That sounds weird though too, so I'm not sure. I worry a bit that there's going to have to be a lot of early outs all over the place, that won't ever be false 98% of the time, and are going to be really hard to test correct behaviour when each of them are hit. Sorry to be a downer on this CL so far, I'm just not totally convinced that things are going to work reliably. I hope we can resolve this stuff. As I told you, I'm off on vacation, so I'll have to pass this review over to James. Maybe he'll have a very different POV of this stuff! https://codereview.chromium.org/14772021/diff/28001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/14772021/diff/28001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:1494: bool LayerTreeHostImpl::InitializeRenderer( Move this function up by the deferred init method? https://codereview.chromium.org/14772021/diff/28001/cc/trees/single_thread_pr... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/14772021/diff/28001/cc/trees/single_thread_pr... cc/trees/single_thread_proxy.cc:142: initializing_new_output_surface_ = true; You can use base::AutoReset<bool> for a bit more clarity/safety with this code pattern. See TiledLayer::Update for an example. However, I'd rather this was a parameter, passing through the is_deferred_init = true from LTHI, or false from here. https://codereview.chromium.org/14772021/diff/28001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/14772021/diff/28001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1175: void ThreadProxy::DidUpdateCapabilitiesOnImplThread( nit: DidInitializeRendererOnImplThread? This function does a bit more than deal with caps (sets up the RP for the offscreen context provider for instance). https://codereview.chromium.org/14772021/diff/28001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1214: void ThreadProxy::DidUpdateCapabilities() { This needs to take and do something with the caps, to have them show up changed on the main thread. But this is a race for sure. We could be in the middle of BeginFrame when this is posted, and then the impl thread has a new renderer with new caps, but the frame is generated with the old caps. A silver lining may be: http://crbug.com/234233 may help make the dependencies on caps on the main thread more clear.. if there is only picture layers on the main thread then, again, maybe we're okay I think as they aren't using settings derived from caps. If there's a HUD layer, it uses the caps' max texture size. If the GL mode has a smaller size than the software mode did (likely) and you're on a big enough display, you get something bad until the next commit. A race by design seems problematic to me :/ I'm not sure what the solution here is though, other than allowing this code path to block the main thread somehow. Or, block here until BeginFrame is done, then block future BeginFrame until this initialization is done, or something. And maybe that lends itself to allowing more code reuse for the non-synchronous case too? Maybe you or James will have some better insight or decide this raciness isn't a big deal.
Why would you not want to have a ResourceProvider when using the compositor?
On 2013/05/24 19:11:13, jamesr wrote: > Why would you not want to have a ResourceProvider when using the compositor? The mental model we have is that ResourceProvider is only ever going to create GL resources. When a software draw suddenly comes in, we need to not use those GL resources for anything so we have to be able to do most things without touching the ResourceProvider. Secondly, on startup (and in some cases throughout the entire lifetime of the WebView), we don't have a GL context at all. In this configuration it makes sense to simply set the ResourceProvider to NULL. That will provide a nice sanity check since we'll have an easily identifiable NULL pointer crash if we ever try to use the resource provider for anything non-GL. > If we're going to go ahead without a RP in this mode, maybe we can have checks and prevent other layer types from appearing in the tree? In our software mode, we only care about displaying PictureLayer and SolidColorLayer. We need to let other things like VideoLayer exist in the tree, but prevent them from drawing anything. When the GL context is later created, we would then bring up a ResourceProvider and use the lost context path to start creating contents for the VideoLayer, and in addition create a TileManager and start creating tiles for the PictureLayer.
> A race by design seems problematic to me :/ I'm not sure what the solution here is though, other than allowing this code path to block the main thread somehow. One thing we're discussing now is that we have the option of requesting an additional feature from the Android system to provide us the GL extensions strings, max texture size etc even if we have no GL context yet. Then we can just set those everywhere on startup and the problem goes away. We'll see where that goes.
On 2013/05/24 19:32:13, aelias wrote: > On 2013/05/24 19:11:13, jamesr wrote: > > Why would you not want to have a ResourceProvider when using the compositor? > > The mental model we have is that ResourceProvider is only ever going to create > GL resources. When a software draw suddenly comes in, we need to not use those > GL resources for anything so we have to be able to do most things without > touching the ResourceProvider. In CC in general a ResourceProvider can provide a resource of any type the compositor understands (GL, software, possibly more in the future), so this is not a good mental model.
This is too complex. I can't review it.
On 2013/05/24 19:37:49, jamesr wrote: > On 2013/05/24 19:32:13, aelias wrote: > > On 2013/05/24 19:11:13, jamesr wrote: > > > Why would you not want to have a ResourceProvider when using the compositor? > > > > The mental model we have is that ResourceProvider is only ever going to create > > GL resources. When a software draw suddenly comes in, we need to not use > those > > GL resources for anything so we have to be able to do most things without > > touching the ResourceProvider. > > In CC in general a ResourceProvider can provide a resource of any type the > compositor understands (GL, software, possibly more in the future), so this is > not a good mental model. I realize that since I added the ResourceProvider's capability to have software resources :). In AndroidWebView's use case though, it's a mistake to ever allocate a software resource. We could also verify this with a DCHECK I suppose.
James: PTAL Reduced scope. Correctness depends on: 1) tile-free software mode does not create any resources 2) the capabilities used on main thread are initialized correctly for hardware on first init already, and are not be used in software mode
I still think this is really complicated. I'm not going to be able to review it.
On 2013/05/24 22:59:22, jamesr wrote: > I still think this is really complicated. I'm not going to be able to review it. Which part do you think is complicated? I can upload just the cc/ changes but I think you are referring to something there?
Note this patch is one of the last blockers to switching over WebView fully to the new path and deleting the old stuff, so we'd all benefit from this landing soon. Are you saying you'd prefer someone else to review it? I can take a close look at it (requesting tests etc) and use my CC OWNERS power if you would prefer, although I've usually let you guys do it in order to avoid bias. Or could you suggest another appropriate reviewer?
You need a reviewer who understands cc, the ResourceProvider system, and context startup. This has deep implications for the compositor so it must be reviewed carefully. Dana is capable of reviewing this, but she's unavailable. This is too complex and and I have too much other high-priority work to look at this any time soon. You need to find someone who understands this and can review it or wait.
OK. We'll leave this pending for now and keep working on the other blockers for WebView bringup. As Dana is only out for a week, we will hopefully not get delayed.
https://codereview.chromium.org/14772021/diff/48001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/14772021/diff/48001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:1436: renderer_ = SoftwareRenderer::Create(this, output_surface.get(), NULL); I wasn't involved in the original CL, but like Dana I have serious reservation wrt not having a resource_provider_ in the "deferred" mode. It will crash on many things, e.g. if you need a render pass, if you have any quad type beyond Picture or SolidColor etc. OTOH you have no way of ensuring that invariant at this level - I'm not even sure what your plan is wrt render pass limitation. So it will result in many types of run-time checks, and it will be very very fragile. I also see no test in this CL. They will be very much needed. https://codereview.chromium.org/14772021/diff/48001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/14772021/diff/48001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:405: bool is_deffered_init); nit: spelling and no abbreviation (is_deferred_initialization).
Recall that we're required to support software rendering at any time, in particular in the case that the GL context actually exists and the ResourceProvider owns nothing but GL resources. That is the primary case that we're interested in, and our starting point for the software-only mode is that it should behave as a subset of that to ensure identical performance+correctness properties and reduce the complexity of our implementation. Therefore, any software resource that we allocate would be only a waste of memory and a leak risk, and we would generally consider such an allocation a bug. We also are unable to block on the main thread to clean up resources when the context is lost, and would like to avoid that can of worms by not allocating anything in the first place. Our thinking was to catch these bugs more easily by crashing when they occur. However, it's true this has a risk of exposing real users to any remaining crashes. One alternative I'm equally fine with is to DCHECK within the ResourceProvider allocate-resource method.
On Tue, May 28, 2013 at 5:37 PM, <aelias@chromium.org> wrote: > Recall that we're required to support software rendering at any time, in > particular in the case that the GL context actually exists and the > ResourceProvider owns nothing but GL resources. I don't know how the current code (with or without this CL) achieves that. After you recreate the renderer with a context, you'll always go to the GLRenderer > That is the primary case that > we're interested in, and our starting point for the software-only mode is > that > it should behave as a subset of that to ensure identical > performance+correctness > properties and reduce the complexity of our implementation. > How do render passes work? What if you have a filter? What if you have clipping + non-screen-aligned transforms, etc.? SoftwareRenderer allocates resources in that case. Is there a design doc that goes into the details? I don't understand how this works. If you're going to skip 95% of the SoftwareRenderer, then it should probably be an explicit, different renderer, that never gets a resource provider. It can share part of the implementation with the current SoftwareRenderer (through inheritance or composition). > Therefore, any software resource that we allocate would be only a waste of > memory and a leak risk, and we would generally consider such an allocation > a > bug. We also are unable to block on the main thread to clean up resources > when > the context is lost, and would like to avoid that can of worms by not > allocating > anything in the first place. Our thinking was to catch these bugs more > easily > by crashing when they occur. > > However, it's true this has a risk of exposing real users to any remaining > crashes. One alternative I'm equally fine with is to DCHECK within the > ResourceProvider allocate-resource method. > The problem is that that DCHECK wouldn't be at the right place. It's way too late. You need, somehow, a check that you don't add a ContentLayer/TextureLayer/ScrollbarLayer/NinePatchLayer/VideoLayer etc. or any layer that allocates resources to the tree when you're in that mode. I've asked Grace to organize a meeting. A code review is not the place to have a fundamental design discussion about this. > https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >
On 2013/05/29 00:57:34, piman wrote: > On Tue, May 28, 2013 at 5:37 PM, <mailto:aelias@chromium.org> wrote: > > > Recall that we're required to support software rendering at any time, in > > particular in the case that the GL context actually exists and the > > ResourceProvider owns nothing but GL resources. > > > I don't know how the current code (with or without this CL) achieves that. > After you recreate the renderer with a context, you'll always go to the > GLRenderer See the uses of output_surface_->ForcedDrawToSoftwareDevice() in layer_tree_host_impl, particularly the scoped_ptr<SoftwareRenderer> temp_software_renderer. Note that this is already working. > > I've asked Grace to organize a meeting. A code review is not the place to > have a fundamental design discussion about this. Yes, we're also updating the design doc to go into more detail in preparation.
OK, we added several sections to the original design doc to deal with some of these issues, PTAL: http://go/sync-compositing Anyway, after looking at the resource cleanup paths more closely I changed my mind, we can still have a ResourceProvider. The main hairy part we were concerned by is the PrioritizedResourceManager cleanup, but we don't use it and it's due for deletion. Bo is looking into making that one NULL instead. I also have a plan for avoiding other kinds of fragile layer-specific hacks. Thanks for the feedback, I agree it would be nice not to have to maintain all the early outs. On 2013/05/29 01:02:14, aelias wrote: > On 2013/05/29 00:57:34, piman wrote: > > On Tue, May 28, 2013 at 5:37 PM, <mailto:aelias@chromium.org> wrote: > > > > > Recall that we're required to support software rendering at any time, in > > > particular in the case that the GL context actually exists and the > > > ResourceProvider owns nothing but GL resources. > > > > > > I don't know how the current code (with or without this CL) achieves that. > > After you recreate the renderer with a context, you'll always go to the > > GLRenderer > > See the uses of output_surface_->ForcedDrawToSoftwareDevice() in > layer_tree_host_impl, particularly the scoped_ptr<SoftwareRenderer> > temp_software_renderer. Note that this is already working. > > > > > I've asked Grace to organize a meeting. A code review is not the place to > > have a fundamental design discussion about this. > > Yes, we're also updating the design doc to go into more detail in preparation.
https://codereview.chromium.org/14772021/diff/48001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/14772021/diff/48001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1169: scheduler_on_impl_thread_->DidCreateAndInitializeOutputSurface(); This scheduler change looks to be the other loose end that hasn't been discussed above. Could you find a way to also make the scheduler behave correctly without forking the initialization path (maybe with a change to the state machine?)
https://codereview.chromium.org/14772021/diff/48001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/14772021/diff/48001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1169: scheduler_on_impl_thread_->DidCreateAndInitializeOutputSurface(); On 2013/05/30 06:42:44, aelias wrote: > This scheduler change looks to be the other loose end that hasn't been discussed > above. Could you find a way to also make the scheduler behave correctly without > forking the initialization path (maybe with a change to the state machine?) I didn't think this as a problem. As far as the scheduler is concerned, nothing has changed, so we are not changing the scheduler state from InitializeForGL. If your only concern is with forking, then it's only a matter of loosening the DCHECKs in DidCreateAndInitializeOutputSurface to allow repeated calls even if output surface is not lost.
Will separate this into 3 patchs for clarity: (1) OutputSurfaceClient::InitializeForGL. (2) Expose protected OutputSurface method to set |context3d_| after construction (3) External hook up code in aw and content Keeping this issue as (1). No new tests yet.
PTAL Renamed method to DeferredInitialize in anticipation to also call it for reverting back to software only mode when being detached from view tree. Added two smoke tests.
https://codereview.chromium.org/14772021/diff/70001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/14772021/diff/70001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:1465: resource_provider_->DidLoseOutputSurface(); You don't want to do that in the re-initialization case, it will cause the RP think the context is lost and won't properly delete resources
https://codereview.chromium.org/14772021/diff/70001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/14772021/diff/70001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:1465: resource_provider_->DidLoseOutputSurface(); On 2013/06/05 00:39:37, piman wrote: > You don't want to do that in the re-initialization case, it will cause the RP > think the context is lost and won't properly delete resources Actually this needs more work as I remembered Dana pointing out that layers are just dropping resources instead of properly releasing them. Is it correct to not change layers code, but destroy the current ResourceProvider in DeferredInitialize. I think destroying the ResourceProvider cleans up all the resources?
Destroying and recreating ResourceProvider won't be quite correct either because will also need to recreate TileManager. In this patch, decided to skip releasing resources. This is ok because we are transitioning from resourceless software mode which does not create ResourceProvider. Long term, software mode will have TileManager and ResourceProvider. Then will need a way to drop resources from TileManager and clean up all resources from ResourceProvider without recreating TileManager and ResourceProvider objects.
On Tue, Jun 4, 2013 at 10:05 PM, <boliu@chromium.org> wrote: > Destroying and recreating ResourceProvider won't be quite correct either > because > will also need to recreate TileManager. > I thought we're going down the lost context-type path when switching between modes, which would also recreate the tile manager? > In this patch, decided to skip releasing resources. This is ok because we > are > transitioning from resourceless software mode which does not create > ResourceProvider. > If you open LayoutTests/compositing/reflections/deeply-nested-reflections.html in resourceless software mode, does it not crash right now? I'm having trouble seeing how it would not. This seems like something we should get right first, creating the appropriate objects to use the compositor correctly, then deal with the switch to hardware when we have the right architecture in place? > > Long term, software mode will have TileManager and ResourceProvider. Then > will > need a way to drop resources from TileManager and clean up all resources > from > ResourceProvider without recreating TileManager and ResourceProvider > objects. > > https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >
On 2013/06/05 14:40:20, danakj wrote: > I thought we're going down the lost context-type path when switching > between modes, which would also recreate the tile manager? > I think Nat's suggestion for this software mode was to create the tile manager with zero RAM budget, then we increase the budget in when switching to hardware mode, and drop it back down to zero when switching back to software mode. Alex summarized it in crbug.com/245935. And that's just my understanding, I haven't look at into how to do this yet. > If you > open LayoutTests/compositing/reflections/deeply-nested-reflections.html in > resourceless software mode, does it not crash right now? I'm having trouble > seeing how it would not. > > This seems like something we should get right first, creating the > appropriate objects to use the compositor correctly, then deal with the > switch to hardware when we have the right architecture in place? > It does not crash, but does not display correctly either. I presume it's skipping drawing the layers from this patch: https://codereview.chromium.org/16211002/. Here's a screenshot on corp: http://www.corp.google.com/~boliu/no_crawl/reflect_software.png Incidentally in hardware, it doesn't look correct compared to on desktop chromium, it's missing the letter in the green squares: http://www.corp.google.com/~boliu/no_crawl/reflect_hardware.png
On Wed, Jun 5, 2013 at 11:39 AM, <boliu@chromium.org> wrote: > On 2013/06/05 14:40:20, danakj wrote: > >> I thought we're going down the lost context-type path when switching >> between modes, which would also recreate the tile manager? >> > > > I think Nat's suggestion for this software mode was to create the tile > manager > with zero RAM budget, then we increase the budget in when switching to > hardware > mode, and drop it back down to zero when switching back to software mode. > Alex > summarized it in crbug.com/245935. And that's just my understanding, I > haven't > look at into how to do this yet. Ok sure, then we should have a code path to do that first I think. > > > If you >> open LayoutTests/compositing/**reflections/deeply-nested-**reflections.html >> in >> resourceless software mode, does it not crash right now? I'm having >> trouble >> seeing how it would not. >> > > This seems like something we should get right first, creating the >> appropriate objects to use the compositor correctly, then deal with the >> switch to hardware when we have the right architecture in place? >> > > > It does not crash, but does not display correctly either. I presume it's > skipping drawing the layers from this patch: > https://codereview.chromium.**org/16211002/<https://codereview.chromium.org/1.... > Here's a screenshot on corp: > http://www.corp.google.com/~**boliu/no_crawl/reflect_**software.png<http://ww... Huh.. I can't see why it would be skipping layers. It's just web content (ie picture layers). Can you take a closer look? This seems fragile. Or alternatively, let's get the ResourceProvider working first. > > > Incidentally in hardware, it doesn't look correct compared to on desktop > chromium, it's missing the letter in the green squares: > http://www.corp.google.com/~**boliu/no_crawl/reflect_**hardware.png<http://ww... It's also missing all the borders.. > > > https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >
On 5 June 2013 11:03, Dana Jansens <danakj@chromium.org> wrote: > On Wed, Jun 5, 2013 at 11:39 AM, <boliu@chromium.org> wrote: > >> On 2013/06/05 14:40:20, danakj wrote: >> >>> I thought we're going down the lost context-type path when switching >>> between modes, which would also recreate the tile manager? >>> >> >> >> I think Nat's suggestion for this software mode was to create the tile >> manager >> with zero RAM budget, then we increase the budget in when switching to >> hardware >> mode, and drop it back down to zero when switching back to software mode. >> Alex >> summarized it in crbug.com/245935. And that's just my understanding, I >> haven't >> look at into how to do this yet. > > > Ok sure, then we should have a code path to do that first I think. > > (+nduca who we discussed this with originally) Just to clarify, is there a hard technical reason for fixing crbug.com/245935 first? Getting this patch (14772021<https://codereview.chromium.org/14772021/>) landed is holding up other dev & all testing. The whole point of raising that bug was to decouple the changes so we can get to the point where we can parallelize that other work. Thanks > >> >> If you >>> open LayoutTests/compositing/**reflections/deeply-nested-**reflections.html >>> in >>> resourceless software mode, does it not crash right now? I'm having >>> trouble >>> seeing how it would not. >>> >> >> This seems like something we should get right first, creating the >>> appropriate objects to use the compositor correctly, then deal with the >>> switch to hardware when we have the right architecture in place? >>> >> >> >> It does not crash, but does not display correctly either. I presume it's >> skipping drawing the layers from this patch: >> https://codereview.chromium.**org/16211002/<https://codereview.chromium.org/1.... >> Here's a screenshot on corp: >> http://www.corp.google.com/~**boliu/no_crawl/reflect_**software.png<http://ww... > > > Huh.. I can't see why it would be skipping layers. It's just web content > (ie picture layers). Can you take a closer look? This seems fragile. Or > alternatively, let's get the ResourceProvider working first. > > >> >> >> Incidentally in hardware, it doesn't look correct compared to on desktop >> chromium, it's missing the letter in the green squares: >> http://www.corp.google.com/~**boliu/no_crawl/reflect_**hardware.png<http://ww... > > > It's also missing all the borders.. > > >> >> >> https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >> > >
I'll start looking into not skip creaing resource manager/tile manager in software mode (crbug.com/245935), but really want to land this as is without before that to enable wider testing among the webview devs. On 2013/06/05 18:03:29, danakj wrote: > Huh.. I can't see why it would be skipping layers. It's just web content > (ie picture layers). Can you take a closer look? I think this is falling into the draw_direct_to_backbuffer case in PictureLayerImpl::AppendQuads: https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...
On Wed, Jun 5, 2013 at 4:29 PM, <boliu@chromium.org> wrote: > I'll start looking into not skip creaing resource manager/tile manager in > software mode (crbug.com/245935), but really want to land this as is > without > before that to enable wider testing among the webview devs. I'll take a look at the CL but this seems like we're doing the work of figuring out these code paths twice. Once with a fake path and no RP and set everything up. And then once again the real way later. > > > On 2013/06/05 18:03:29, danakj wrote: > >> Huh.. I can't see why it would be skipping layers. It's just web content >> (ie picture layers). Can you take a closer look? >> > > I think this is falling into the draw_direct_to_backbuffer case in > PictureLayerImpl::AppendQuads: > https://code.google.com/p/**chromium/codesearch#chromium/** > src/cc/layers/picture_layer_**impl.cc&q=DRAW_MODE_** > RESOURCELESS_SOFTWARE&sq=**package:chromium&type=cs&l=121<https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_layer_impl.cc&q=DRAW_MODE_RESOURCELESS_SOFTWARE&sq=package:chromium&type=cs&l=121> It's not the picture quads that are confusing to me. That page has render surfaces. I don't see anything in resourceless mode that would prevent us from making RenderSurfaceImpls or from those generating RenderPassDrawQuads. Nor anything in software renderer that would stop us from drawing them, and crashing from a lack of resource provider. > > > https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >
Thanks for keeping this pretty simple. https://codereview.chromium.org/14772021/diff/76001/cc/scheduler/scheduler_st... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/14772021/diff/76001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:436: // This can be called from LayerTreeHost::DeferredInitialize while an output Pretty sure LTH isn't calling the scheduler :) https://codereview.chromium.org/14772021/diff/76001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:437: // surface is already active. nit: double space https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:1537: return success; AIUI, the output surface will never have an offscreen provider to give here, since it can't go to the main thread. So when you switch to hardware mode, things like CSS filters are going to disappear until you do a commit. So.. you probably want to set needs commit here? https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:211: scoped_refptr<ContextProvider> offscreen_context_provider) OVERRIDE; Is the embedder just going to have to always provide this provider here? https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:2884: layer_tree_host()->SetNeedsRedraw(); normally we PostSetNeedsCommitToMainThread() here instead. https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:2890: TestWebGraphicsContext3D::Create( nit: You could just use Create() instead of making an attributes here. https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:2900: host_impl->DeferredInitialize(scoped_refptr<ContextProvider>())); Maybe it would be worth testing that we actually draw both in RESOURCELESS and in HARDWARE modes? You could do this by adding a FakePictureLayer to the tree and have it keep track of being called to WillDraw in the various modes..
Answering comments about offscreen context. No new patch set yet. On 2013/06/05 21:07:21, danakj wrote: > I'll take a look at the CL but this seems like we're doing the work of > figuring out these code paths twice. Once with a fake path and no RP and > set everything up. And then once again the real way later. Yeah, thanks for bearing with me. Hopefully this fake path one is not too hard to reason about. https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:1537: return success; On 2013/06/05 21:53:32, danakj wrote: > AIUI, the output surface will never have an offscreen provider to give here, > since it can't go to the main thread. So when you switch to hardware mode, > things like CSS filters are going to disappear until you do a commit. So.. you > probably want to set needs commit here? If the offscreen context is the only thing for this needs commit, then I would leave it out for now. I think we can should be able to get a context here "eventually". https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/14772021/diff/76001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:211: scoped_refptr<ContextProvider> offscreen_context_provider) OVERRIDE; On 2013/06/05 21:53:32, danakj wrote: > Is the embedder just going to have to always provide this provider here? Part 3 of this patch will just pass in NULL for now. The eventual plan (not reviewed by anyone yet), this takes a few steps: 1) First make sure LTHClient is creating the in process context for webview: https://codereview.chromium.org/16235005/ 2) Pre-create this context when output surface is created (cheap for in-process context) and hold a ref to be passed here. 3) Move the logic to determine if an offscreen context is needed (I think if filters are in use?) to impl side. Basically just pass the boolean over in a commit. 4) Then finally, DeferredInitialize can decide if it's actually needed and drop the ref if it's not. Dreaming big about supporting repeatedly going into and out of GL mode synchronously, dealing with lost offscreen context before the first hardware init, and possibly clean up this code up for all platforms: Have the compositor thread offscreen context be created/destroyed entirely on the compositor thread. Then can avoid the set_leak_on_destroy hack as well.
all comments done
Trying to think through the possible implications of making calls to change state on the scheduler without being in a lost context scenario. You may have pending frames in flight, and I'm not sure how this will interact with that. Have you considered this? https://codereview.chromium.org/14772021/diff/89001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/14772021/diff/89001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:2920: host_impl->SetNeedsRedrawRect(gfx::Rect(1, 1)); Why do you need this if you are causing a second commit in DidCommitAndDrawFrame(). Or, why do you cause a second commit if you are already causing a 2nd draw here? These two are going to race and I suspect may cause a 3rd draw to happen sometimes, which will make this flaky as is. https://codereview.chromium.org/14772021/diff/89001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/14772021/diff/89001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1188: OutputSurface* output_surface_ptr = layer_tree_host_impl_->output_surface(); This stuff is meant to happen when the output surface changes, but it did not change here. Should these stay in the InitializeOutputSurfaceOnImplThread() method? https://codereview.chromium.org/14772021/diff/89001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1203: scheduler_on_impl_thread_->DidCreateAndInitializeOutputSurface(); This is going to call frame_rate_controller_->DidAbortAllPendingFrames() as well. Is that okay?
On 2013/06/06 14:49:30, danakj wrote: > Trying to think through the possible implications of making calls to change > state on the scheduler without being in a lost context scenario. You may have > pending frames in flight, and I'm not sure how this will interact with that. > Have you considered this? Easy answer: DeferredInitialize should not be changing scheduler state. For now, this is only designed to promote from resourceless software mode (which cannot have lost output surface) to hardware mode. So the output surface must already be active and not lost when DeferredInitialize is called. Relaxing the DCHECK in scheduler state machine is only to avoid this path diverging from regular initialization path. In fact in PS9 and before, I did not call scheduler DidCreateAndInitializeOutputSurface. Do you think I should revert back at least for now? More winded answer for expanding this method to do other things in the future: DeferredInitialize is synchronous and all correctness depends on that. AFAIK the actual draw frame all happens in a single ScheduledActionDrawAndSwapInternal call, right? We will need to ensure that DeferredInitialize can never be called inside ScheduledActionDrawAndSwapInternal. But pending frames are ok, the frames after DeferredInitialize will be hardware frames.
On Thu, Jun 6, 2013 at 12:07 PM, <boliu@chromium.org> wrote: > On 2013/06/06 14:49:30, danakj wrote: > >> Trying to think through the possible implications of making calls to >> change >> state on the scheduler without being in a lost context scenario. You may >> have >> pending frames in flight, and I'm not sure how this will interact with >> that. >> Have you considered this? >> > > Easy answer: DeferredInitialize should not be changing scheduler state. > For now, > this is only designed to promote from resourceless software mode (which > cannot > have lost output surface) to hardware mode. So the output surface must > already > be active and not lost when DeferredInitialize is called. Relaxing the > DCHECK in > scheduler state machine is only to avoid this path diverging from regular > initialization path. In fact in PS9 and before, I did not call scheduler > DidCreateAndInitializeOutputSu**rface. Do you think I should revert back > at least > for now? > Yes, I think we should keep the scheduler setup in the "new output surface" path still. > > More winded answer for expanding this method to do other things in the > future: > DeferredInitialize is synchronous and all correctness depends on that. > AFAIK the > actual draw frame all happens in a single ScheduledActionDrawAndSwapInte** > rnal > call, right? We will need to ensure that DeferredInitialize can never be > called > Yes. > inside ScheduledActionDrawAndSwapInte**rnal. But pending frames are ok, > the frames > after DeferredInitialize will be hardware frames. > Okay, cool. Having the output surface re-enter to the compositor mid-draw would be weird, we should definitely avoid that. > > https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/14772021/diff/89001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/14772021/diff/89001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:2920: host_impl->SetNeedsRedrawRect(gfx::Rect(1, 1)); On 2013/06/06 14:49:31, danakj wrote: > Why do you need this if you are causing a second commit in > DidCommitAndDrawFrame(). Or, why do you cause a second commit if you are already > causing a 2nd draw here? > > These two are going to race and I suspect may cause a 3rd draw to happen > sometimes, which will make this flaky as is. Removed PostSetNeedsCommitToMainThread below, and make this block a PostTask since adding DCHECK(!inside_draw_) https://codereview.chromium.org/14772021/diff/89001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/14772021/diff/89001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1188: OutputSurface* output_surface_ptr = layer_tree_host_impl_->output_surface(); On 2013/06/06 14:49:31, danakj wrote: > This stuff is meant to happen when the output surface changes, but it did not > change here. Should these stay in the InitializeOutputSurfaceOnImplThread() > method? Also expanding the question to SetSwapBuffersCompleteSupported above, even though that could actually change with a new renderer. I'm on the fence for this one. On one hand, caps can change and there is no way to plumb the new values to main thread in time, so there is a weak and brittle contract with webview that these caps cannot change. (This is even happens to be true for using_swap_complete_callback since the in-process context does not support it) In this case, we should move these back into InitializeOutputSurfaceOnImplThread and actually DCHECK here that nothing has changed. On the other hand, there is this overall effort (feeling?) that for impl-side painting, nothing on main thread should need to know about caps. Nulling PrioritizedResourceManager was one step towards that but it's not complete. In this case, it's fine to change these in case something does change. Practically, it really doesn't matter for webview at this point as we just want to get hardware up without having apply a bunch of uncommitted patches. Maybe we'll tweak things like max_frames_pending later when we are ready to do perf optimizations. So...maybe just keep these here for "not diverging from init path"? Thoughts? https://codereview.chromium.org/14772021/diff/89001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1203: scheduler_on_impl_thread_->DidCreateAndInitializeOutputSurface(); On 2013/06/06 14:49:31, danakj wrote: > This is going to call frame_rate_controller_->DidAbortAllPendingFrames() as > well. Is that okay? Moved this back to InitializeOutputSurfaceOnImplThread.
https://codereview.chromium.org/14772021/diff/89001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/14772021/diff/89001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1188: OutputSurface* output_surface_ptr = layer_tree_host_impl_->output_surface(); On 2013/06/06 17:39:25, boliu wrote: > On 2013/06/06 14:49:31, danakj wrote: > > This stuff is meant to happen when the output surface changes, but it did not > > change here. Should these stay in the InitializeOutputSurfaceOnImplThread() > > method? > > Also expanding the question to SetSwapBuffersCompleteSupported above, even > though that could actually change with a new renderer. > > I'm on the fence for this one. > > On one hand, caps can change and there is no way to plumb the new values to main > thread in time, so there is a weak and brittle contract with webview that these > caps cannot change. (This is even happens to be true for > using_swap_complete_callback since the in-process context does not support it) > In this case, we should move these back into InitializeOutputSurfaceOnImplThread > and actually DCHECK here that nothing has changed. I really like the idea of DCHECK () << "You cant do this and this is why". With a TODO to re-examine this when "settings.impl_side_painting" is on everywhere. (Use the setting name exactly so we find it with grep when we go to delete this setting one day). > On the other hand, there is this overall effort (feeling?) that for impl-side > painting, nothing on main thread should need to know about caps. Nulling > PrioritizedResourceManager was one step towards that but it's not complete. In > this case, it's fine to change these in case something does change. > > Practically, it really doesn't matter for webview at this point as we just want > to get hardware up without having apply a bunch of uncommitted patches. Maybe > we'll tweak things like max_frames_pending later when we are ready to do perf > optimizations. > > So...maybe just keep these here for "not diverging from init path"? Thoughts? https://codereview.chromium.org/14772021/diff/97001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/14772021/diff/97001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:2912: // MULTI_THREAD_TEST_F(LayerTreeHostTestDeferredInitialize); Test looks good, but commented out?
Make what messes you need to make, then fix them promptly. On Wed, Jun 5, 2013 at 11:25 AM, Jonathan Dixon <joth@chromium.org> wrote: > > > > On 5 June 2013 11:03, Dana Jansens <danakj@chromium.org> wrote: > >> On Wed, Jun 5, 2013 at 11:39 AM, <boliu@chromium.org> wrote: >> >>> On 2013/06/05 14:40:20, danakj wrote: >>> >>>> I thought we're going down the lost context-type path when switching >>>> between modes, which would also recreate the tile manager? >>>> >>> >>> >>> I think Nat's suggestion for this software mode was to create the tile >>> manager >>> with zero RAM budget, then we increase the budget in when switching to >>> hardware >>> mode, and drop it back down to zero when switching back to software >>> mode. Alex >>> summarized it in crbug.com/245935. And that's just my understanding, I >>> haven't >>> look at into how to do this yet. >> >> >> Ok sure, then we should have a code path to do that first I think. >> >> > > (+nduca who we discussed this with originally) > > Just to clarify, is there a hard technical reason for fixing > crbug.com/245935 first? Getting this patch (14772021<https://codereview.chromium.org/14772021/>) > landed is holding up other dev & all testing. The whole point of raising > that bug was to decouple the changes so we can get to the point where we > can parallelize that other work. > Thanks > > > >> >>> >>> If you >>>> open LayoutTests/compositing/**reflections/deeply-nested-**reflections.html >>>> in >>>> resourceless software mode, does it not crash right now? I'm having >>>> trouble >>>> seeing how it would not. >>>> >>> >>> This seems like something we should get right first, creating the >>>> appropriate objects to use the compositor correctly, then deal with the >>>> switch to hardware when we have the right architecture in place? >>>> >>> >>> >>> It does not crash, but does not display correctly either. I presume it's >>> skipping drawing the layers from this patch: >>> https://codereview.chromium.**org/16211002/<https://codereview.chromium.org/1.... >>> Here's a screenshot on corp: >>> http://www.corp.google.com/~**boliu/no_crawl/reflect_**software.png<http://ww... >> >> >> Huh.. I can't see why it would be skipping layers. It's just web content >> (ie picture layers). Can you take a closer look? This seems fragile. Or >> alternatively, let's get the ResourceProvider working first. >> >> >>> >>> >>> Incidentally in hardware, it doesn't look correct compared to on desktop >>> chromium, it's missing the letter in the green squares: >>> http://www.corp.google.com/~**boliu/no_crawl/reflect_**hardware.png<http://ww... >> >> >> It's also missing all the borders.. >> >> >>> >>> >>> https://codereview.chromium.**org/14772021/<https://codereview.chromium.org/1... >>> >> >> >
https://codereview.chromium.org/14772021/diff/103001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/14772021/diff/103001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2894: host_impl->SetNeedsRedrawRect(gfx::Rect(1, 1)); And of course, test did not pass when enabled, so more changes. Using DeferredInitializeAndRedraw rather than PostSetNeedsCommitToMainThread because we don't really care about commit We just wnat to make sure we draw again, so need to set damage to avoid all the no damage optimizations. There is also optimization to not post DidCommitAndDrawFrame to main thread if there is a pending one, so removing DidCommitAndDrawFrame and layer_->update_count() checks. https://codereview.chromium.org/14772021/diff/103001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2895: // PostSetNeedsCommitToMainThread(); Arg...going to remove this in next PS. https://codereview.chromium.org/14772021/diff/103001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/14772021/diff/103001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:1197: if (offscreen_context_provider.get()) Moved everything back to InitializeOutputSurfaceOnImplThread. There is nothing to DCHECK for caps did not change. How strongly do you feel about this given it might be temporary? I can add an impl copy of capabilities under !defined(NDEBUG)
https://codereview.chromium.org/14772021/diff/103001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/14772021/diff/103001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2894: host_impl->SetNeedsRedrawRect(gfx::Rect(1, 1)); On 2013/06/06 19:27:14, boliu wrote: > And of course, test did not pass when enabled, so more changes. > > Using DeferredInitializeAndRedraw rather than PostSetNeedsCommitToMainThread > because we don't really care about commit We just wnat to make sure we draw > again, so need to set damage to avoid all the no damage optimizations. Note you could also SetNeedsDisplay() on the main thread layer as an alternative. > There is also optimization to not post DidCommitAndDrawFrame to main thread if > there is a pending one, so removing DidCommitAndDrawFrame and > layer_->update_count() checks. That sounds weird? Other tests rely on this method heavily. But we only call it for the first draw after a commit. Now you do 1 commit and 2 draws, so it won't fire twice. https://codereview.chromium.org/14772021/diff/103001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2898: virtual void AfterTest() OVERRIDE {} It'd be nice to test initialized_gl_ = true here still? https://codereview.chromium.org/14772021/diff/103001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/14772021/diff/103001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:1197: if (offscreen_context_provider.get()) On 2013/06/06 19:27:14, boliu wrote: > Moved everything back to InitializeOutputSurfaceOnImplThread. > > There is nothing to DCHECK for caps did not change. How strongly do you feel > about this given it might be temporary? I can add an impl copy of capabilities > under !defined(NDEBUG) You could add a getter to the scheduler to dcheck that SwapBuffersCompleteSupported is the same as the new renderer caps.
Added DCHECK to make sure swapbuffer callback is still the same. Of course the new test triggered this, since the test context does support the callback by default, so fix by disabling callback in test context. https://codereview.chromium.org/14772021/diff/103001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/14772021/diff/103001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2894: host_impl->SetNeedsRedrawRect(gfx::Rect(1, 1)); On 2013/06/06 19:36:13, danakj wrote: > > There is also optimization to not post DidCommitAndDrawFrame to main thread if > > there is a pending one, so removing DidCommitAndDrawFrame and > > layer_->update_count() checks. > > That sounds weird? Other tests rely on this method heavily. But we only call it > for the first draw after a commit. Now you do 1 commit and 2 draws, so it won't > fire twice. Yep you are right. I misunderstood next_frame_is_newly_committed_frame_on_impl_thread_. https://codereview.chromium.org/14772021/diff/103001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2898: virtual void AfterTest() OVERRIDE {} On 2013/06/06 19:36:13, danakj wrote: > It'd be nice to test initialized_gl_ = true here still? Done.
Tangent: Just talked to joth who is planning on adding OnSwapBuffersComplete callback in software mode: https://codereview.chromium.org/16430005/ I think he's hitting the DCHECK in FrameRateController because the RendererCap was not turned on. Fun times...
LGTM https://codereview.chromium.org/14772021/diff/109001/cc/scheduler/frame_rate_... File cc/scheduler/frame_rate_controller.cc (right): https://codereview.chromium.org/14772021/diff/109001/cc/scheduler/frame_rate_... cc/scheduler/frame_rate_controller.cc:91: bool FrameRateController::SwapBuffersCompleteSupported() const { nit: swap_buffers_complete_supported() and move to the header file
Last nit fixed. Going to wait for transform/clip patch (https://codereview.chromium.org/15579002/) to land first since these two patches conflict. Thanks to Dana and all the reviewers for putting up with webview requirements. Will prepare the other two parts of this patch, then work on zero-budget tile manager asap.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/boliu@chromium.org/14772021/98016
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/boliu@chromium.org/14772021/132001
Message was sent while issue was closed.
Change committed as 204771 |
