Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11189037:
toggle FPS counter in compositor (Closed)
|
Created:
8 years, 2 months ago by egraether Modified:
8 years, 1 month ago CC:
chromium-reviews, cc-bugs_chromium.org, darin-cc_chromium.org, brianderson, slavi Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionThe FPS counter displays the framerate in the compositor's HUD layer. This change allows for toggling the FPS counter without using the '--show-fps-counter' command-line flag.
WebKit side: https://bugs.webkit.org/show_bug.cgi?id=99660
BUG=154754
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166233
Patch Set 1 #
Total comments: 9
Patch Set 2 : added check for accelerated compositing, removed 'showFPSCounter' from settings, command-line flag … #
Total comments: 5
Patch Set 3 : plumbing 'showFPSCounter' through HUD, added 'createHUDLayerIfNeeded' #
Total comments: 5
Patch Set 4 : added DCHECKs and moved hasFontAtlas into HUDLayer #
Total comments: 6
Patch Set 5 : removed hasFontAtlas methods, put DCHECKs in HUDLayerImpl #
Total comments: 8
Patch Set 6 : restored showFPSCounter in WeblayerTreeView::Settings and removed fontAtlas DCHECKS for flags set i… #
Total comments: 2
Patch Set 7 : Rebase to 165943 #Patch Set 8 : Rebase to 166046 #
Messages
Total messages: 31 (0 generated)
Looking good. Let me know when you fix the crash when enabling the fps counter on the welcome page. If you run into any issues with the gdb instructions I sent you, ping me. https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_imp... File cc/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_imp... cc/heads_up_display_layer_impl.cc:149: if (settings.showFPSCounter || layerTreeHostImpl()->showFPSCounter()) { How hard would it be to have the command-line flag affect layerTreeHostImpl()->showFPSCounter()? Although not a requirement, it would be nice if the fps counter could be disabled even if it were enabled by the command line. https://codereview.chromium.org/11189037/diff/1/cc/single_thread_proxy.cc File cc/single_thread_proxy.cc (right): https://codereview.chromium.org/11189037/diff/1/cc/single_thread_proxy.cc#new... cc/single_thread_proxy.cc:311: { I don't think this anonymous scope is necessary. https://codereview.chromium.org/11189037/diff/1/cc/single_thread_proxy.cc#new... cc/single_thread_proxy.cc:313: ASSERT(CCProxy::isImplThread()); You should move this ASSERT into CCLayerTreeHostImpl::setShowFPSCounter. https://codereview.chromium.org/11189037/diff/1/webkit/compositor_bindings/We... File webkit/compositor_bindings/WebLayerTreeViewImpl.h (right): https://codereview.chromium.org/11189037/diff/1/webkit/compositor_bindings/We... webkit/compositor_bindings/WebLayerTreeViewImpl.h:52: virtual void setShowFPSCounter(bool show) OVERRIDE; Is this part of the WebLayerTreeView class?
replies to Brian Anderson https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_imp... File cc/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_imp... cc/heads_up_display_layer_impl.cc:149: if (settings.showFPSCounter || layerTreeHostImpl()->showFPSCounter()) { It would be nice, but I think it's not a real problem that the command-line flag overrides the option in the inspector On 2012/10/18 00:13:21, Brian Anderson wrote: > How hard would it be to have the command-line flag affect > layerTreeHostImpl()->showFPSCounter()? > > Although not a requirement, it would be nice if the fps counter could be > disabled even if it were enabled by the command line. https://codereview.chromium.org/11189037/diff/1/cc/single_thread_proxy.cc File cc/single_thread_proxy.cc (right): https://codereview.chromium.org/11189037/diff/1/cc/single_thread_proxy.cc#new... cc/single_thread_proxy.cc:311: { I saw that other similar code parts also don't use it. I will remove it. On 2012/10/18 00:13:21, Brian Anderson wrote: > I don't think this anonymous scope is necessary. https://codereview.chromium.org/11189037/diff/1/cc/single_thread_proxy.cc#new... cc/single_thread_proxy.cc:313: ASSERT(CCProxy::isImplThread()); You are right. On 2012/10/18 00:13:21, Brian Anderson wrote: > You should move this ASSERT into CCLayerTreeHostImpl::setShowFPSCounter. https://codereview.chromium.org/11189037/diff/1/webkit/compositor_bindings/We... File webkit/compositor_bindings/WebLayerTreeViewImpl.h (right): https://codereview.chromium.org/11189037/diff/1/webkit/compositor_bindings/We... webkit/compositor_bindings/WebLayerTreeViewImpl.h:52: virtual void setShowFPSCounter(bool show) OVERRIDE; Yes, it's in the WebKit patch: https://bugs.webkit.org/show_bug.cgi?id=99660 On 2012/10/18 00:13:21, Brian Anderson wrote: > Is this part of the WebLayerTreeView class?
https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_imp... File cc/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_imp... cc/heads_up_display_layer_impl.cc:149: if (settings.showFPSCounter || layerTreeHostImpl()->showFPSCounter()) { On 2012/10/18 15:56:30, egraether wrote: > It would be nice, but I think it's not a real problem that the command-line flag > overrides the option in the inspector > > On 2012/10/18 00:13:21, Brian Anderson wrote: > > How hard would it be to have the command-line flag affect > > layerTreeHostImpl()->showFPSCounter()? > > > > Although not a requirement, it would be nice if the fps counter could be > > disabled even if it were enabled by the command line. > I agree with Brian here, having a single point decide if the feature is on would be preferrable.
On 2012/10/18 16:45:07, danakj wrote: > https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_imp... > File cc/heads_up_display_layer_impl.cc (right): > > https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_imp... > cc/heads_up_display_layer_impl.cc:149: if (settings.showFPSCounter || > layerTreeHostImpl()->showFPSCounter()) { > On 2012/10/18 15:56:30, egraether wrote: > > It would be nice, but I think it's not a real problem that the command-line > flag > > overrides the option in the inspector > > > > On 2012/10/18 00:13:21, Brian Anderson wrote: > > > How hard would it be to have the command-line flag affect > > > layerTreeHostImpl()->showFPSCounter()? > > > > > > Although not a requirement, it would be nice if the fps counter could be > > > disabled even if it were enabled by the command line. > > > > I agree with Brian here, having a single point decide if the feature is on would > be preferrable. I created a new Patch Set which takes care of the command-line flag for the initial state. The FPS counter can then be toggled from the inspector. The checkbox in the inspector is not yet able to display 'checked' when the command-line flag was set. https://bugs.webkit.org/show_bug.cgi?id=99660
On 2012/10/18 00:13:21, Brian Anderson wrote: > Looking good. Let me know when you fix the crash when enabling the fps counter > on the welcome page. > > If you run into any issues with the gdb instructions I sent you, ping me. > > https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_imp... > File cc/heads_up_display_layer_impl.cc (right): > > https://codereview.chromium.org/11189037/diff/1/cc/heads_up_display_layer_imp... > cc/heads_up_display_layer_impl.cc:149: if (settings.showFPSCounter || > layerTreeHostImpl()->showFPSCounter()) { > How hard would it be to have the command-line flag affect > layerTreeHostImpl()->showFPSCounter()? > > Although not a requirement, it would be nice if the fps counter could be > disabled even if it were enabled by the command line. > > https://codereview.chromium.org/11189037/diff/1/cc/single_thread_proxy.cc > File cc/single_thread_proxy.cc (right): > > https://codereview.chromium.org/11189037/diff/1/cc/single_thread_proxy.cc#new... > cc/single_thread_proxy.cc:311: { > I don't think this anonymous scope is necessary. > > https://codereview.chromium.org/11189037/diff/1/cc/single_thread_proxy.cc#new... > cc/single_thread_proxy.cc:313: ASSERT(CCProxy::isImplThread()); > You should move this ASSERT into CCLayerTreeHostImpl::setShowFPSCounter. > > https://codereview.chromium.org/11189037/diff/1/webkit/compositor_bindings/We... > File webkit/compositor_bindings/WebLayerTreeViewImpl.h (right): > > https://codereview.chromium.org/11189037/diff/1/webkit/compositor_bindings/We... > webkit/compositor_bindings/WebLayerTreeViewImpl.h:52: virtual void > setShowFPSCounter(bool show) OVERRIDE; > Is this part of the WebLayerTreeView class? The crash on the welcome page was fixed by adding a check for accelerated compositing. I still need to find a way to only show the checkbox in the inspector if this is enabled.
> > The crash on the welcome page was fixed by adding a check for accelerated > compositing. I still need to find a way to only show the checkbox in the > inspector if this is enabled. You dont have to make the UI conditional accelerated compositing being present. Instead, make the FPS checkbox only show up when --force-compositing-mode is on. I think or'ing the command line state in with the runtime state is okay. But, the CCSettings object should go away and we shoudl check the command line directly, now that we can. That may involve moving the command line state to cc_flags.cc.
Some more comments http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host.cc File cc/layer_tree_host.cc (right): http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host.cc#newc... cc/layer_tree_host.cc:283: m_proxy->setShowFPSCounter(show); so what happens if you set the atlas, then call setShowFPSCounter? How does the m_hudLayer get created? This seems very fragile. Please refactor setShowFPS and setFontAtlas so that they call createHUDLayerIfNeeded(). http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host.h File cc/layer_tree_host.h (right): http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host.h#newco... cc/layer_tree_host.h:260: bool m_showFPSCounter; why are you storing this? http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host_impl.cc File cc/layer_tree_host_impl.cc (right): http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host_impl.cc... cc/layer_tree_host_impl.cc:1463: void CCLayerTreeHostImpl::setShowFPSCounter(bool show) Can this be state on the hud layer? e.g this function just says hudLayer()->setShowFPSCounter(show) http://codereview.chromium.org/11189037/diff/12001/webkit/compositor_bindings... File webkit/compositor_bindings/WebLayerTreeViewImpl.h (right): http://codereview.chromium.org/11189037/diff/12001/webkit/compositor_bindings... webkit/compositor_bindings/WebLayerTreeViewImpl.h:53: virtual bool hasFontAtlas() OVERRIDE; You're going to need to set up your patches to land one at a time. So, for exmple, since you've written OVERRIDE here, you can't actually land this until the code on the webkit side lands. And, your webkit-side code probably has this set as pure virtual. Which means that if you land the webkit side without your chrome changes, the chrome side will break because this file wont have these changes. I suggest you land the webkit side change first, ultimately, and not make these methods pure virtual. Then you can land these after just as they are. http://codereview.chromium.org/11189037/diff/12001/webkit/compositor_bindings... webkit/compositor_bindings/WebLayerTreeViewImpl.h:53: virtual bool hasFontAtlas() OVERRIDE; Also, put hasFontAtlas next to the setFontAtlas function. They are related.
On Sat, Oct 20, 2012 at 3:39 PM, <nduca@chromium.org> wrote: > >> The crash on the welcome page was fixed by adding a check for accelerated >> compositing. I still need to find a way to only show the checkbox in the >> inspector if this is enabled. > > > You dont have to make the UI conditional accelerated compositing being > present. > Instead, make the FPS checkbox only show up when --force-compositing-mode is > on. > > I think or'ing the command line state in with the runtime state is okay. > But, > the CCSettings object should go away and we shoudl check the command line > directly, now that we can. That may involve moving the command line state to > cc_flags.cc. The difficulty here is knowing if you are UI/renderer and if the command line flag should affect you. Do we have a way to do that? I know piman was concerned about using command-line directly for stuff.
On 2012/10/20 22:08:30, danakj wrote: > On Sat, Oct 20, 2012 at 3:39 PM, <mailto:nduca@chromium.org> wrote: > > > >> The crash on the welcome page was fixed by adding a check for accelerated > >> compositing. I still need to find a way to only show the checkbox in the > >> inspector if this is enabled. > > > > > > You dont have to make the UI conditional accelerated compositing being > > present. > > Instead, make the FPS checkbox only show up when --force-compositing-mode is > > on. > > > > I think or'ing the command line state in with the runtime state is okay. > > But, > > the CCSettings object should go away and we shoudl check the command line > > directly, now that we can. That may involve moving the command line state to > > cc_flags.cc. > > The difficulty here is knowing if you are UI/renderer and if the > command line flag should affect you. Do we have a way to do that? I > know piman was concerned about using command-line directly for stuff. I managed to find a way to hide the checkbox if accelerated compositing is not active. The problem is that this can change, which a reviewer of the WebKit patch pointed out. I agree that only showing the checkbox when --force-compositing-mode is set might be the best solution for now.
On 2012/10/20 19:49:41, nduca wrote: > Some more comments > > http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host.cc > File cc/layer_tree_host.cc (right): > > http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host.cc#newc... > cc/layer_tree_host.cc:283: m_proxy->setShowFPSCounter(show); > so what happens if you set the atlas, then call setShowFPSCounter? How does the > m_hudLayer get created? This seems very fragile. Please refactor setShowFPS and > setFontAtlas so that they call createHUDLayerIfNeeded(). done, please have a look. > http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host.h > File cc/layer_tree_host.h (right): > > http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host.h#newco... > cc/layer_tree_host.h:260: bool m_showFPSCounter; > why are you storing this? it was stored, because CCLayerTreeHost::willCommit created a HUDLayer only if according flags in the settings were set. Now I forward showFPSCounter directly to the HUDLayer. > http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host_impl.cc > File cc/layer_tree_host_impl.cc (right): > > http://codereview.chromium.org/11189037/diff/12001/cc/layer_tree_host_impl.cc... > cc/layer_tree_host_impl.cc:1463: void > CCLayerTreeHostImpl::setShowFPSCounter(bool show) > Can this be state on the hud layer? e.g this function just says > hudLayer()->setShowFPSCounter(show) This seemed not to work using the plumbing of showFPSCounter through CCProxy, because the HUDLayer is on the MainThread. I changed the implementation to a similar one of setFontAtlas which is passed to CCHeadsUpDisplayLayerImpl via HeadsUpDisplayLayerChromium. I'm not sure if this is better than the Proxy method, but it equals setFontAtlas. > http://codereview.chromium.org/11189037/diff/12001/webkit/compositor_bindings... > File webkit/compositor_bindings/WebLayerTreeViewImpl.h (right): > > http://codereview.chromium.org/11189037/diff/12001/webkit/compositor_bindings... > webkit/compositor_bindings/WebLayerTreeViewImpl.h:53: virtual bool > hasFontAtlas() OVERRIDE; > You're going to need to set up your patches to land one at a time. So, for > exmple, since you've written OVERRIDE here, you can't actually land this until > the code on the webkit side lands. And, your webkit-side code probably has this > set as pure virtual. Which means that if you land the webkit side without your > chrome changes, the chrome side will break because this file wont have these > changes. > I suggest you land the webkit side change first, ultimately, and not make these > methods pure virtual. Then you can land these after just as they are. I tested both patches, how they work without the other. It seems to me that this one is easier to get in first. I just had to remove OVERRIDE from methods in WebLayerTreeViewImpl. > http://codereview.chromium.org/11189037/diff/12001/webkit/compositor_bindings... > webkit/compositor_bindings/WebLayerTreeViewImpl.h:53: virtual bool > hasFontAtlas() OVERRIDE; > Also, put hasFontAtlas next to the setFontAtlas function. They are related. done
Some comments for you. This looks cool, thanks for taking this on. > I changed the implementation to a > similar one of setFontAtlas which is passed to > CCHeadsUpDisplayLayerImpl via > HeadsUpDisplayLayerChromium. I'm not sure if this is better > than the Proxy method, but it equals setFontAtlas. This sounds much better, prefer to not add new paths through CCProxy. https://codereview.chromium.org/11189037/diff/12003/cc/layer_tree_host.cc File cc/layer_tree_host.cc (right): https://codereview.chromium.org/11189037/diff/12003/cc/layer_tree_host.cc#new... cc/layer_tree_host.cc:281: createHUDLayerIfNeeded(); Can we DCHECK some stuff here? DCHECK(m_hudLayer); DCHECK(m_hudLayer->hasFontAtlas()); This means that we have to setFontAtlast *before* setShowFPSCounter on the webkit side, but I'd like some DCHECKs here for future code-movings. https://codereview.chromium.org/11189037/diff/12003/cc/layer_tree_host.cc#new... cc/layer_tree_host.cc:303: if (m_settings.showDebugRects()) This is missing m_settings.showPlatformLayerTree. Use showDebugInfo() instead? https://codereview.chromium.org/11189037/diff/12003/cc/layer_tree_host.h File cc/layer_tree_host.h (left): https://codereview.chromium.org/11189037/diff/12003/cc/layer_tree_host.h#oldc... cc/layer_tree_host.h:70: bool showDebugInfo() const { return showPlatformLayerTree || showFPSCounter || showDebugRects(); } Can we keep this and just remove showFPSCounter from it? https://codereview.chromium.org/11189037/diff/12003/webkit/compositor_binding... File webkit/compositor_bindings/web_layer_tree_view_impl.h (right): https://codereview.chromium.org/11189037/diff/12003/webkit/compositor_binding... webkit/compositor_bindings/web_layer_tree_view_impl.h:51: // NOTE: not marked OVERRIDE to avoid dependency till associated WebKit patch is landed: https://bugs.webkit.org/show_bug.cgi?id=99660 Don't worry about adding OVERRIDE to new methods here. The current OVERRIDEs are just there because they were already there when we moved stuff here. https://codereview.chromium.org/11189037/diff/12003/webkit/compositor_binding... webkit/compositor_bindings/web_layer_tree_view_impl.h:76: bool m_hasFontAtlas; How about moving this piece of state into CCLayerTreeHost so we can keep this impl class as just a wrapper? It can get/set this value from the CCLayerTreeHost. Or, even better, into the hud layer? Then CCLayerTreeHost::hasFontAtlas() { return m_hudLayer && m_hudLayer->hasFontAtlast(); }
On 2012/10/23 15:19:53, danakj wrote: > https://codereview.chromium.org/11189037/diff/12003/cc/layer_tree_host.cc#new... > cc/layer_tree_host.cc:281: createHUDLayerIfNeeded(); > Can we DCHECK some stuff here? > DCHECK(m_hudLayer); > DCHECK(m_hudLayer->hasFontAtlas()); > > This means that we have to setFontAtlast *before* setShowFPSCounter on the > webkit side, but I'd like some DCHECKs here for future code-movings. I added DCHECKS to assure that fontAtlas and HUDLayer have been loaded. > https://codereview.chromium.org/11189037/diff/12003/cc/layer_tree_host.cc#new... > cc/layer_tree_host.cc:303: if (m_settings.showDebugRects()) > This is missing m_settings.showPlatformLayerTree. Use showDebugInfo() instead? Thr 'showPlatformLayerTree' flag gets checked in 'WebViewImpl::setIsAcceleratedCompositingActive()', because it also needs the fontAtlas. So the HUDLayer is already created at this point. I added a DCHECK to assure that the fontAtlas and HUDLayer were loaded. I use 'showDebugRects()' here, because 'showDebugInfo()' is no longer necessary and may be misleading. > https://codereview.chromium.org/11189037/diff/12003/cc/layer_tree_host.h#oldc... > cc/layer_tree_host.h:70: bool showDebugInfo() const { return > showPlatformLayerTree || showFPSCounter || showDebugRects(); } > Can we keep this and just remove showFPSCounter from it? I removed this, because it was only used in 'CCLayerTreeHost::willCommit()' where it is no longer needed. I can put it back without the showFPSCounter though. > https://codereview.chromium.org/11189037/diff/12003/webkit/compositor_binding... > webkit/compositor_bindings/web_layer_tree_view_impl.h:51: // NOTE: not marked > OVERRIDE to avoid dependency till associated WebKit patch is landed: > https://bugs.webkit.org/show_bug.cgi?id=99660 > Don't worry about adding OVERRIDE to new methods here. The current OVERRIDEs are > just there because they were already there when we moved stuff here. I removed the Note. > https://codereview.chromium.org/11189037/diff/12003/webkit/compositor_binding... > webkit/compositor_bindings/web_layer_tree_view_impl.h:76: bool m_hasFontAtlas; > How about moving this piece of state into CCLayerTreeHost so we can keep this > impl class as just a wrapper? It can get/set this value from the > CCLayerTreeHost. > > Or, even better, into the hud layer? Then > CCLayerTreeHost::hasFontAtlas() { return m_hudLayer && > m_hudLayer->hasFontAtlast(); } done Thank you.
https://codereview.chromium.org/11189037/diff/23004/cc/heads_up_display_layer.cc File cc/heads_up_display_layer.cc (right): https://codereview.chromium.org/11189037/diff/23004/cc/heads_up_display_layer... cc/heads_up_display_layer.cc:51: return true; Maybe this should be hasFontAtlas()? https://codereview.chromium.org/11189037/diff/23004/cc/heads_up_display_layer... cc/heads_up_display_layer.cc:68: m_showFPSCounter = show; Only setNeedsCommit if m_showFPSCounter changes value. https://codereview.chromium.org/11189037/diff/23004/cc/layer_tree_host.cc File cc/layer_tree_host.cc (right): https://codereview.chromium.org/11189037/diff/23004/cc/layer_tree_host.cc#new... cc/layer_tree_host.cc:284: setNeedsCommit(); You have multiple layers of setNeedsCommit here. You should do this in one place. Probably on the layer itself. https://codereview.chromium.org/11189037/diff/23004/cc/layer_tree_host.cc#new... cc/layer_tree_host.cc:294: createHUDLayerIfNeeded(); Why does this create the hud layer? What if none of the debug things are turned on? https://codereview.chromium.org/11189037/diff/23004/cc/layer_tree_host.cc#new... cc/layer_tree_host.cc:313: DCHECK(hasFontAtlas()); Maybe just move all the checks for the font atlas into createHUDLayerIfNeeded? https://codereview.chromium.org/11189037/diff/23004/webkit/compositor_binding... File webkit/compositor_bindings/web_layer_tree_view_impl.h (right): https://codereview.chromium.org/11189037/diff/23004/webkit/compositor_binding... webkit/compositor_bindings/web_layer_tree_view_impl.h:51: virtual bool hasFontAtlas() const; I don't like exposing hasFontAtlas here. The caller should know itself whether or not setFontAtlas has been called previously. I'd prefer if instead this was just documented that setFontAtlas must be called before setShowFPSCounter.
On 2012/10/24 18:35:27, enne wrote: > https://codereview.chromium.org/11189037/diff/23004/cc/heads_up_display_layer... > cc/heads_up_display_layer.cc:51: return true; > Maybe this should be hasFontAtlas()? If debug rectangles are displayed on the HUDLayer then this should be true and there is no fontAtlas set. Also the HUDLayer is only created when needed, so this should be alright. > https://codereview.chromium.org/11189037/diff/23004/cc/heads_up_display_layer... > cc/heads_up_display_layer.cc:68: m_showFPSCounter = show; > Only setNeedsCommit if m_showFPSCounter changes value. The setNeedsCommit call in setFontAtlas is necessary in case the showPlatformLayerTree setting is active. > https://codereview.chromium.org/11189037/diff/23004/cc/layer_tree_host.cc#new... > cc/layer_tree_host.cc:284: setNeedsCommit(); > You have multiple layers of setNeedsCommit here. You should do this in one > place. Probably on the layer itself. I removed the setNeedsCommit calls from LayerTreeHost and left the ones in the HUDLayer. The changes will be in the next upload. > https://codereview.chromium.org/11189037/diff/23004/cc/layer_tree_host.cc#new... > cc/layer_tree_host.cc:294: createHUDLayerIfNeeded(); > Why does this create the hud layer? What if none of the debug things are turned > on? Again, this is in case the showPlatformLayerTree setting is active. This setting is passed to the HUDLayer via the CCSettings object. > https://codereview.chromium.org/11189037/diff/23004/cc/layer_tree_host.cc#new... > cc/layer_tree_host.cc:313: DCHECK(hasFontAtlas()); > Maybe just move all the checks for the font atlas into createHUDLayerIfNeeded? The loading of the fontAtlas is partly done in WebViewImpl. These checks assure that all flags in the WebView have been read correctly, the fontAtlas was set and the HUDLayer created. > https://codereview.chromium.org/11189037/diff/23004/webkit/compositor_binding... > webkit/compositor_bindings/web_layer_tree_view_impl.h:51: virtual bool > hasFontAtlas() const; > I don't like exposing hasFontAtlas here. The caller should know itself whether > or not setFontAtlas has been called previously. I'm not sure here, there are different opinions about this. > I'd prefer if instead this was just documented that setFontAtlas must be called > before setShowFPSCounter. Right now the DCHECK in LayerTreeHost::setShowFPSCounter fails if the fontAtlas was not already set. Is there something else I should do? Thank you. NOTE: It appears to me that many remarks of the reviewers are made about the way the showPlatformLayerTree and showDebugRects flags are set. Because the showFPSCounter setting changed it's path, the code looks messy now. Maybe the solution is a createHUDLayer call to LayerTreeHost so that the LayerTreeHost no longer decides itself when a HUDLayer is necessary.
(In the future, would you mind responding to code comments with more comments in code, rather than replying inline in the text box? It's awfully hard to follow the discussion without the context of the code when there's a lot of back and forth. Maybe that's just my preference.) On 2012/10/24 23:41:13, egraether wrote: > On 2012/10/24 18:35:27, enne wrote: > > > https://codereview.chromium.org/11189037/diff/23004/cc/heads_up_display_layer... > > cc/heads_up_display_layer.cc:51: return true; > > Maybe this should be hasFontAtlas()? > > If debug rectangles are displayed on the HUDLayer then this should be true and > there is no fontAtlas set. Also the HUDLayer is only created when needed, so > this should be alright. Ah, right. https://codereview.chromium.org/11189037/diff/23004/cc/heads_up_display_layer... > > cc/heads_up_display_layer.cc:68: m_showFPSCounter = show; > > Only setNeedsCommit if m_showFPSCounter changes value. > > The setNeedsCommit call in setFontAtlas is necessary in case the > showPlatformLayerTree setting is active. That's true. I was saying that you shouldn't call setNeedsCommit in showFPSCounter if that value doesn't change. I don't follow what setFontAtlas has to do with that? https://codereview.chromium.org/11189037/diff/23004/cc/layer_tree_host.cc#new... > > cc/layer_tree_host.cc:294: createHUDLayerIfNeeded(); > > Why does this create the hud layer? What if none of the debug things are > turned > > on? > > Again, this is in case the showPlatformLayerTree setting is active. This setting > is passed to the HUDLayer via the CCSettings object. Ok, sure. I forgot that you also changed from showDebugInfo() to just showDebugRects in willCommit. I think the previous code was a little bit clearer in that all the cases that created the HUD layer were consolidated in willCommit, rather than being spread across willCommit, setFontAtlas, and setShowFPSCounter, but that's a tiny quibble. https://codereview.chromium.org/11189037/diff/23004/cc/layer_tree_host.cc#new... > > cc/layer_tree_host.cc:313: DCHECK(hasFontAtlas()); > > Maybe just move all the checks for the font atlas into createHUDLayerIfNeeded? > > The loading of the fontAtlas is partly done in WebViewImpl. These checks assure > that all flags in the WebView have been read correctly, the fontAtlas was set > and the HUDLayer created. Ah, right. I forgot that there's the one case of debug rects where you might not need the font atlas. Too many different debug flags. https://codereview.chromium.org/11189037/diff/23004/webkit/compositor_binding... > > webkit/compositor_bindings/web_layer_tree_view_impl.h:51: virtual bool > > hasFontAtlas() const; > > I don't like exposing hasFontAtlas here. The caller should know itself > whether > > or not setFontAtlas has been called previously. > > I'm not sure here, there are different opinions about this. I would rather have a bool in WebViewImpl than a set of functions piped through the LayerTreeHost/View interface. > > I'd prefer if instead this was just documented that setFontAtlas must be > called > > before setShowFPSCounter. > > Right now the DCHECK in LayerTreeHost::setShowFPSCounter fails if the fontAtlas > was not already set. Is there something else I should do? Yeah. I'm trying to say that WebLayerTreeView(Impl) should not have a hasFontAtlas function. > NOTE: > It appears to me that many remarks of the reviewers are made about the way the > showPlatformLayerTree and showDebugRects flags are set. Because the > showFPSCounter setting changed it's path, the code looks messy now. Maybe the > solution is a createHUDLayer call to LayerTreeHost so that the LayerTreeHost no > longer decides itself when a HUDLayer is necessary. I don't know that we should go this route. I think previously showDebugInfo() encapsulated the fact that we wanted to show *some* debug HUD layer and now it's broken up into separate conditions that are hard to keep track of. If you wanted to put it back together, rather than adding a separate m_showFPSCounter, one option would be to mutate m_settings. Then showDebugInfo() still would behave as it did previously.
I got convinced that removing hasFontAtlas again makes the code a lot cleaner. The WebViewImpl has now a member that saves whether the fontAtlas was loaded (WebKit patch not updated yet). LayerTreeHost assures that a HUDLayer is created in every case by calling createHUDLayerIfNeeded and does no other checks. I added DCHECKs into the HUDLayer instead, which assure that a fontAtlas is loaded as soon as a draw call happens. It can be criticized that the DCHECKS fail pretty late in case a setting was missed, but with all the different kinds of flags it's though to both check if everything is correct and the code is still understandable. https://codereview.chromium.org/11189037/diff/33001/cc/heads_up_display_layer... File cc/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/11189037/diff/33001/cc/heads_up_display_layer... cc/heads_up_display_layer_impl.cc:157: DCHECK(m_fontAtlas.get()); I put DCHECKs in here, so that the fontAtlas still gets checked at one point. https://codereview.chromium.org/11189037/diff/33001/cc/layer_tree_host.cc File cc/layer_tree_host.cc (right): https://codereview.chromium.org/11189037/diff/33001/cc/layer_tree_host.cc#new... cc/layer_tree_host.cc:279: void LayerTreeHost::createHUDLayerIfNeeded() Called whenever there is need to create a HUDLayer or set a member of it. https://codereview.chromium.org/11189037/diff/33001/cc/layer_tree_host.cc#new... cc/layer_tree_host.cc:301: if (m_settings.showDebugRects()) Checking for the rects is enough here. In case showPlatformLayerTree is set, the HUDLayer is created via setFontAtlas triggered in WebViewImpl.
The code to enable/disable the fps counter at runtime looks good, but I have some concerns about the removal of the command line / about:flags setting. I know a number of folks who run with the FPS counter on all the time via about:flags (maybe as an indication that compositing is happening as much as an FPS estimate?). They'd probably be irritated to only be able to turn it on via inspector. Can we leave the command-line flag on to always show the FPS counter? Alternatively, can you sync up with folks who I know use this flag (maybe vangelis/wiltzius) and see how they feel about this?
On 2012/10/25 17:01:23, enne wrote: > The code to enable/disable the fps counter at runtime looks good, but I have > some concerns about the removal of the command line / about:flags setting. > > I know a number of folks who run with the FPS counter on all the time via > about:flags (maybe as an indication that compositing is happening as much as an > FPS estimate?). They'd probably be irritated to only be able to turn it on via > inspector. > > Can we leave the command-line flag on to always show the FPS counter? > Alternatively, can you sync up with folks who I know use this flag (maybe > vangelis/wiltzius) and see how they feel about this? The command-line flag is still working. It's checked in WebViewImpl::setIsAcceleratedCompositingActive, the inspector setting can then be used to turn it off and on. I just figured out a way to assure that the command-line flag is also working when this patch gets landed without the WebKit one being approved yet. It will be in the next upload.
Ah, I see. I misread what was going in in ui/compositor/compositor.cc. The --show-fps-counter flag will still show the fps counter, because showFPSCounter is called explicitly, rather than through settings on the WebKit side. http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.cc File ui/compositor/compositor.cc (left): http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.c... ui/compositor/compositor.cc:150: settings.showFPSCounter = Won't this break the --ui-show-fps-counter flag, since there's no corresponding calls to showFPSCounter? I think you might be missing a host_->setShowFPSCounter(...) call here to counteract that.
http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.cc File ui/compositor/compositor.cc (left): http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.c... ui/compositor/compositor.cc:150: settings.showFPSCounter = On 2012/10/29 17:58:57, enne wrote: > Won't this break the --ui-show-fps-counter flag, since there's no corresponding > calls to showFPSCounter? I think you might be missing a > host_->setShowFPSCounter(...) call here to counteract that. I did not realize that this was a different flag when I removed that. Anyhow, I started testing what it did and if your suggestion works, but I couldn't figure out what this does so far. When is a Compositor instantiated? What is the --ui-show-fps-counter flag for?
http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.cc File ui/compositor/compositor.cc (left): http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.c... ui/compositor/compositor.cc:150: settings.showFPSCounter = On 2012/10/30 00:03:57, egraether wrote: > On 2012/10/29 17:58:57, enne wrote: > > Won't this break the --ui-show-fps-counter flag, since there's no > corresponding > > calls to showFPSCounter? I think you might be missing a > > host_->setShowFPSCounter(...) call here to counteract that. > > I did not realize that this was a different flag when I removed that. Anyhow, I > started testing what it did and if your suggestion works, but I couldn't figure > out what this does so far. > When is a Compositor instantiated? What is the --ui-show-fps-counter flag for? It's for Aura builds, which has a separate compositor in the browser process. This allows a top-level FPS counter for the desktop rather than on each tab.
http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.cc File ui/compositor/compositor.cc (left): http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.c... ui/compositor/compositor.cc:150: settings.showFPSCounter = On 2012/10/30 16:09:45, enne wrote: > On 2012/10/30 00:03:57, egraether wrote: > > On 2012/10/29 17:58:57, enne wrote: > > > Won't this break the --ui-show-fps-counter flag, since there's no > > corresponding > > > calls to showFPSCounter? I think you might be missing a > > > host_->setShowFPSCounter(...) call here to counteract that. > > > > I did not realize that this was a different flag when I removed that. Anyhow, > I > > started testing what it did and if your suggestion works, but I couldn't > figure > > out what this does so far. > > When is a Compositor instantiated? What is the --ui-show-fps-counter flag for? > > It's for Aura builds, which has a separate compositor in the browser process. > This allows a top-level FPS counter for the desktop rather than on each tab. And how is the fontAtlas loaded in that case?
On 2012/10/30 16:09:45, enne wrote: > It's for Aura builds, which has a separate compositor in the browser process. > This allows a top-level FPS counter for the desktop rather than on each tab. If you stick "chromeos=1" in your GYP_DEFINES when you gclient runhooks, you'll get an aura build. Then you can run with --ui-show-fps-counter and see what's up. :)
The setting of the '--ui-' flags in compositor.cc forced me to remove the DCHECKs for the fontAtlas, as they are not loaded in that case. I re-added the showFPSCounter flag to the WebLayerTreeView::Settings and re-added the showDebugInfo method to the LayerTreeSettings to account for these flags. I think that removing showFPSCounter from WebLayerTreeView::Settings would be better, but it is a bit tricky right now. It is used in a few places and I think it can be removed more easily after this patch was landed. Right now both patches, this one and the WebKit part, can be landed independent without breaking builds. Related: Why is there a --ui-show-layer-tree flag if no fontAtlas is loaded when it is active? http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.cc File ui/compositor/compositor.cc (left): http://codereview.chromium.org/11189037/diff/33001/ui/compositor/compositor.c... ui/compositor/compositor.cc:150: settings.showFPSCounter = On 2012/10/30 16:41:34, egraether wrote: > On 2012/10/30 16:09:45, enne wrote: > > On 2012/10/30 00:03:57, egraether wrote: > > > On 2012/10/29 17:58:57, enne wrote: > > > > Won't this break the --ui-show-fps-counter flag, since there's no > > > corresponding > > > > calls to showFPSCounter? I think you might be missing a > > > > host_->setShowFPSCounter(...) call here to counteract that. > > > > > > I did not realize that this was a different flag when I removed that. > Anyhow, > > I > > > started testing what it did and if your suggestion works, but I couldn't > > figure > > > out what this does so far. > > > When is a Compositor instantiated? What is the --ui-show-fps-counter flag > for? > > > > It's for Aura builds, which has a separate compositor in the browser process. > > This allows a top-level FPS counter for the desktop rather than on each tab. > > And how is the fontAtlas loaded in that case? It isn't and that caused some problems. I removed the change here and re-added showFPSCounter to the WebLayerTreeView::Settings, so --ui-show-fps-counter works again. I did not use host_->setShowFPSCounter() instead, because that causes a dependency on the associated WebKit patch which defines the method setShowFPSCounter. http://codereview.chromium.org/11189037/diff/46001/cc/layer_tree_host.h File cc/layer_tree_host.h (right): http://codereview.chromium.org/11189037/diff/46001/cc/layer_tree_host.h#newco... cc/layer_tree_host.h:71: bool showDebugInfo() const { return showPlatformLayerTree || showDebugRects(); } I reintroduced showDebugInfo as the --ui-show-layer-tree flag would be ignored, because in that case no fontAtlas is set, so that a HUDLayer would be created. http://codereview.chromium.org/11189037/diff/46001/webkit/compositor_bindings... File webkit/compositor_bindings/web_layer_tree_view_impl.cc (right): http://codereview.chromium.org/11189037/diff/46001/webkit/compositor_bindings... webkit/compositor_bindings/web_layer_tree_view_impl.cc:59: if (webSettings.showFPSCounter) This condition detects the commandline-flag set in the WebLayerTreeView::Settings and calls setShowFPSCounter on the LayerTreeHost. This is necessary for the --ui-show-fps-counter flag to work.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/egraether@chromium.org/11189037/55011
Retried try job too often for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/egraether@chromium.org/11189037/55011
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/egraether@chromium.org/11189037/55011
Change committed as 166233 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
