Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2625203004:
Add debug method to print surface references. (Closed)
DescriptionAdd debug method to print surface references.
SurfaceManager::SurfaceReferencesToString() prints current surface
references starting at the top level root to a string. Only compiled for
debug/DCHECK builds. Helpful to visualize current references.
BUG=659227
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2625203004
Cr-Commit-Position: refs/heads/master@{#444432}
Committed: https://chromium.googlesource.com/chromium/src/+/2226adf3f8999d114147ffcfc79089cc2e136076
Patch Set 1 #Patch Set 2 : Cleanup. #
Total comments: 20
Patch Set 3 : Address comments. #
Total comments: 4
Patch Set 4 : Little fixes. #
Messages
Total messages: 23 (13 generated)
Description was changed from ========== Add debug print for surface references. WIP. BUG= ========== to ========== Add debug print for surface references. WIP. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Add debug print for surface references. WIP. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add debug method to print for surface references. SurfaceManager::PrintSurfaceReferences() prints current surface references starting at the top level root. Only compiled for debug/DCHECK builds. Helpful to visualize current references. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Add debug method to print for surface references. SurfaceManager::PrintSurfaceReferences() prints current surface references starting at the top level root. Only compiled for debug/DCHECK builds. Helpful to visualize current references. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add debug method to print for surface references. SurfaceManager::PrintSurfaceReferences() prints current surface references starting at the top level root. Only compiled for debug/DCHECK builds. Helpful to visualize current references. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
kylechar@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:23: std::string SurfaceToString(const Surface& surface) { seems like this should use the stringstream? https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:29: .render_pass_list[0] dont u want the back? why the topmost one? https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:79: // Starting a |root_surface_id_| will print all reachable surfaces. starting at? seems mostly duplicating the header tho https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:84: #endif // !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) i dont think u need the // here when its only like 6 lines https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:542: void SurfaceManager::PrintSurfaceReferencesImpl(const SurfaceId& surface_id, You could do this with a lambda and then the stringstream stuff would be only in the .cc file.. https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:555: const SurfaceIdSet& children = iter->second; Set is a misleading name, it sounds ordered already.. :/ https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:12: #include <sstream> do u need more than iosfwd? and should these be inside DCHECK_IS_ON()? https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:48: #if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) #if DCHECK_IS_ON() is the way to spell this https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:50: std::string PrintSurfaceReferences(); This doesnt print, SurfaceReferencesToString ?
https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:23: std::string SurfaceToString(const Surface& surface) { On 2017/01/17 23:21:57, danakj (slow) wrote: > seems like this should use the stringstream? Sure. It's not that useful as a function on it's own if it doesn't return a string though, so I moved it back into the SurfaceReferencesToStringImpl method. https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:29: .render_pass_list[0] On 2017/01/17 23:21:57, danakj (slow) wrote: > dont u want the back? why the topmost one? I probably do want the back one if you're asking :) I had assumed the render_pass_list[0] got drawn first, so it would be the biggest usually, but that must not be right. What exactly is the point of RenderPass anyways? Like, what does have multiple RenderPass objects made up a depth ordered quads let you do rather than just having more quads in a single RenderPass? https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:79: // Starting a |root_surface_id_| will print all reachable surfaces. On 2017/01/17 23:21:57, danakj (slow) wrote: > starting at? seems mostly duplicating the header tho Removed. https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:84: #endif // !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) On 2017/01/17 23:21:57, danakj (slow) wrote: > i dont think u need the // here when its only like 6 lines Done. https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:542: void SurfaceManager::PrintSurfaceReferencesImpl(const SurfaceId& surface_id, On 2017/01/17 23:21:57, danakj (slow) wrote: > You could do this with a lambda and then the stringstream stuff would be only in > the .cc file.. It's possible but the lambda definition isn't the most readable, since you can't use auto with a recursive lambda. What do you think? std::function<void(const SurfaceId&, std::string, std::stringstream*)> SurfaceReferencesToStringImpl = [this, &SurfaceReferencesToStringImpl]( const SurfaceId& surface_id, std::string indent, std::stringstream* str) { https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:555: const SurfaceIdSet& children = iter->second; On 2017/01/17 23:21:57, danakj (slow) wrote: > Set is a misleading name, it sounds ordered already.. :/ It's not really necessary, so removed the variable entirely. Although I can change the typedef name if you want? https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:12: #include <sstream> On 2017/01/17 23:21:57, danakj (slow) wrote: > do u need more than iosfwd? and should these be inside DCHECK_IS_ON()? iosfwd has a forward def for stringstream, so it should be fine. https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:48: #if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) On 2017/01/17 23:21:57, danakj (slow) wrote: > #if DCHECK_IS_ON() is the way to spell this Huh, that's handy. https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:50: std::string PrintSurfaceReferences(); On 2017/01/17 23:21:57, danakj (slow) wrote: > This doesnt print, SurfaceReferencesToString ? Done.
https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:29: .render_pass_list[0] On 2017/01/18 15:06:18, kylechar wrote: > On 2017/01/17 23:21:57, danakj (slow) wrote: > > dont u want the back? why the topmost one? > > I probably do want the back one if you're asking :) I had assumed the > render_pass_list[0] got drawn first, so it would be the biggest usually, but > that must not be right. > > What exactly is the point of RenderPass anyways? Like, what does have multiple > RenderPass objects made up a depth ordered quads let you do rather than just > having more quads in a single RenderPass? A renderpass is an indirect surface that we draw into then use as a source into another renderpass. It lets us apply effects to all the things drawn into it as a single unit that we could not to each of the elements individually. Some examples are blurs and transparency. We also use them for masks, and also for clipping when the clip isn't a right-angle rectangle in its target, since we can draw at some other angle into a texture and pixels that fall of the edge are implicitly clipped. Think if you have two rects that partially overlap and are each 50% opacity. Then you can see teh bottom rect thru the top rect. Now consider if you want them to have 50% opacity TOGETHER. You have to draw them both then apply 50% to that, now you can see them them but you don't see teh bottom rect thru the top one. They are in the order we draw them, you're right. This isn't documented on CompositorFrame, it probably should be. This can be found here https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=148... where we "AppendRenderPasses" which ends up doing push_back (https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=148...) in "dependency order" meaning A depends on B if B draws into A, so B comes first ie exactly first one is drawn into first. The order is the same as found in the RenderSurfaceLayerList, which is the output of LayerTreeHostCommon::CalculateDrawProperties. This is all more subtle and not described in comments than I thought it was going to be... Anyhow, this makes the back one the root, or the one all the others draw into. So its most representative of the size as it clips the rest, also usually the largest modulo said clipping.
https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:542: void SurfaceManager::PrintSurfaceReferencesImpl(const SurfaceId& surface_id, On 2017/01/18 15:06:18, kylechar wrote: > On 2017/01/17 23:21:57, danakj (slow) wrote: > > You could do this with a lambda and then the stringstream stuff would be only > in > > the .cc file.. > > It's possible but the lambda definition isn't the most readable, since you can't > use auto with a recursive lambda. What do you think? > > std::function<void(const SurfaceId&, std::string, std::stringstream*)> > SurfaceReferencesToStringImpl = [this, &SurfaceReferencesToStringImpl]( > const SurfaceId& surface_id, std::string indent, > std::stringstream* str) { Ah, std::function is banned. http://chromium-cpp.appspot.com/ You'd use a Callback to do that, which you could. But then the lambda needs to not capture, and take those as arguments instead.
LGTM https://codereview.chromium.org/2625203004/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2625203004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:536: *str << surface->GetEligibleFrame() can you try using a temp var to split this into two statements and maybe get some better reading line splits (or not splits) https://codereview.chromium.org/2625203004/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2625203004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:29: #if DCHECK_IS_ON() should include base/logging.h for this macro
Description was changed from ========== Add debug method to print for surface references. SurfaceManager::PrintSurfaceReferences() prints current surface references starting at the top level root. Only compiled for debug/DCHECK builds. Helpful to visualize current references. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add debug method to print for surface references. SurfaceManager::SurfaceReferencesToString() prints current surface references starting at the top level root to a string. Only compiled for debug/DCHECK builds. Helpful to visualize current references. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by kylechar@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2625203004/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2625203004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:536: *str << surface->GetEligibleFrame() On 2017/01/18 16:57:11, danakj (slow) wrote: > can you try using a temp var to split this into two statements and maybe get > some better reading line splits (or not splits) Done. https://codereview.chromium.org/2625203004/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2625203004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:29: #if DCHECK_IS_ON() On 2017/01/18 16:57:11, danakj (slow) wrote: > should include base/logging.h for this macro Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add debug method to print for surface references. SurfaceManager::SurfaceReferencesToString() prints current surface references starting at the top level root to a string. Only compiled for debug/DCHECK builds. Helpful to visualize current references. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add debug method to print surface references. SurfaceManager::SurfaceReferencesToString() prints current surface references starting at the top level root to a string. Only compiled for debug/DCHECK builds. Helpful to visualize current references. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/01/18 16:50:40, danakj (slow) wrote: > https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... > File cc/surfaces/surface_manager.cc (right): > > https://codereview.chromium.org/2625203004/diff/20001/cc/surfaces/surface_man... > cc/surfaces/surface_manager.cc:29: .render_pass_list[0] > On 2017/01/18 15:06:18, kylechar wrote: > > On 2017/01/17 23:21:57, danakj (slow) wrote: > > > dont u want the back? why the topmost one? > > > > I probably do want the back one if you're asking :) I had assumed the > > render_pass_list[0] got drawn first, so it would be the biggest usually, but > > that must not be right. > > > > What exactly is the point of RenderPass anyways? Like, what does have multiple > > RenderPass objects made up a depth ordered quads let you do rather than just > > having more quads in a single RenderPass? > > A renderpass is an indirect surface that we draw into then use as a source into > another renderpass. It lets us apply effects to all the things drawn into it as > a single unit that we could not to each of the elements individually. Some > examples are blurs and transparency. We also use them for masks, and also for > clipping when the clip isn't a right-angle rectangle in its target, since we can > draw at some other angle into a texture and pixels that fall of the edge are > implicitly clipped. > > Think if you have two rects that partially overlap and are each 50% opacity. > Then you can see teh bottom rect thru the top rect. Now consider if you want > them to have 50% opacity TOGETHER. You have to draw them both then apply 50% to > that, now you can see them them but you don't see teh bottom rect thru the top > one. > > They are in the order we draw them, you're right. This isn't documented on > CompositorFrame, it probably should be. This can be found here > https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=148... > where we "AppendRenderPasses" which ends up doing push_back > (https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=148...) > in "dependency order" meaning A depends on B if B draws into A, so B comes first > ie exactly first one is drawn into first. The order is the same as found in the > RenderSurfaceLayerList, which is the output of > LayerTreeHostCommon::CalculateDrawProperties. This is all more subtle and not > described in comments than I thought it was going to be... > > Anyhow, this makes the back one the root, or the one all the others draw into. > So its most representative of the size as it clips the rest, also usually the > largest modulo said clipping. That makes a lot more sense!
The CQ bit was unchecked by kylechar@chromium.org
The CQ bit was checked by kylechar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2625203004/#ps60001 (title: "Little fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484764312999760,
"parent_rev": "7031a828c5dcedc937bbf375c42daab08ca6162f", "commit_rev":
"2226adf3f8999d114147ffcfc79089cc2e136076"}
Message was sent while issue was closed.
Description was changed from ========== Add debug method to print surface references. SurfaceManager::SurfaceReferencesToString() prints current surface references starting at the top level root to a string. Only compiled for debug/DCHECK builds. Helpful to visualize current references. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add debug method to print surface references. SurfaceManager::SurfaceReferencesToString() prints current surface references starting at the top level root to a string. Only compiled for debug/DCHECK builds. Helpful to visualize current references. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2625203004 Cr-Commit-Position: refs/heads/master@{#444432} Committed: https://chromium.googlesource.com/chromium/src/+/2226adf3f8999d114147ffcfc790... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/2226adf3f8999d114147ffcfc790... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
