[cc] Trace detailed tile info when --trace-all-rendered-frames

[cc] Trace detailed tile info when --trace-all-rendered-frames

With this command line flag, this store a detailed dump of all tile data into the trace buffer, including tile quads in screenspace. This provides enough foundation for cc-frame-viewer to visualize what impl-side painting is doing.

NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181956

[nduca, 2013-02-01 12:18:31]

ptal, especially at tile.h's new set_screenspace_quad --- how do i make this
cheap but runtime?

[shawnsingh, 2013-02-01 19:39:21]

I'll take a look over the weekend...  in the meantime, here's my first reaction
skimming through it.

https://codereview.chromium.org/12096112/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12096112/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:440: MathUtil::projectQuad(current_screen_transform,
First, are you sure we want project here?  This is mapping "forward" from local
space to screen, so map should be good enough.  (If mapQuad doesn't exist, we
can consider adding it back, I may have left it out for now to make sure we dont
use it for non-debugging code and shoot ourselves in the foot with more clipping
issues)

Second, yeah this will definitely hurt performance.  Is it possible to pull all
this out into a completely separate walk through tiles that does not get called
unless we want to serialize?

Otherwise, maybe we need to for now just #define it.  I'm still unfamiliar with
the tile priority code, but I suspect this is pretty hot code path and even an
if (serializeStuff) condition might be worth avoiding, right?

[whunt, 2013-02-01 21:09:26]

On 2013/02/01 19:39:21, shawnsingh wrote:
> I'll take a look over the weekend...  in the meantime, here's my first
reaction
> skimming through it.
> 
> https://codereview.chromium.org/12096112/diff/1/cc/picture_layer_tiling.cc
> File cc/picture_layer_tiling.cc (right):
> 
>
https://codereview.chromium.org/12096112/diff/1/cc/picture_layer_tiling.cc#ne...
> cc/picture_layer_tiling.cc:440:
MathUtil::projectQuad(current_screen_transform,
> First, are you sure we want project here?  This is mapping "forward" from
local
> space to screen, so map should be good enough.  (If mapQuad doesn't exist, we
> can consider adding it back, I may have left it out for now to make sure we
dont
> use it for non-debugging code and shoot ourselves in the foot with more
clipping
> issues)
> 
> Second, yeah this will definitely hurt performance.  Is it possible to pull
all
> this out into a completely separate walk through tiles that does not get
called
> unless we want to serialize?
> 
> Otherwise, maybe we need to for now just #define it.  I'm still unfamiliar
with
> the tile priority code, but I suspect this is pretty hot code path and even an
> if (serializeStuff) condition might be worth avoiding, right?

To note:  It path looks correct but it's impossible to verify w/o getting used. 
Given that it's not used it's not going to break anything even if it's buggy so
I don't think it's dangerous in any way.  We'll need to verify it once we start
using it.

[whunt, 2013-02-01 21:09:33]

https://codereview.chromium.org/12096112/diff/1/cc/tile.h
File cc/tile.h (right):

https://codereview.chromium.org/12096112/diff/1/cc/tile.h#newcode85
cc/tile.h:85: #ifndef NDEBUG
question: does the style guide say anything about using #ifndef vs. #if
!defined()

https://codereview.chromium.org/12096112/diff/1/cc/tile.h#newcode86
cc/tile.h:86: gfx::RectF current_screen_rect_[2];
I don't see this value used anywhere in your tile.cc patch and it's private.

https://codereview.chromium.org/12096112/diff/1/cc/tile_priority.cc
File cc/tile_priority.cc (right):

https://codereview.chromium.org/12096112/diff/1/cc/tile_priority.cc#newcode94
cc/tile_priority.cc:94: state->Set("resolution",
TileResolutionAsValue(resolution).release());
Is this going to cause a double-release as the values goes out of scope when
state eventually dies?

https://codereview.chromium.org/12096112/diff/1/cc/tile_priority.h
File cc/tile_priority.h (right):

https://codereview.chromium.org/12096112/diff/1/cc/tile_priority.h#newcode97
cc/tile_priority.h:97: #ifdef TRACK_TILE_SCREEN_SPACE_QUADS
same question on #if defined

[shawnsingh, 2013-02-07 21:11:33]

LGTM on my behalf;  all my comments are just nits.

In general I wonder if this AsValue() pattern may start becoming a maintenance
burden.  It would probably be worth discussing down the road how we might want
to automate or friendlify it for when we want to change these data structures...
it's always ideal for us to say "we'll remember to change it, it's not that much
effort", but realistically it probably won't happen that way.

https://codereview.chromium.org/12096112/diff/17019/cc/layer_tree_debug_state.cc
File cc/layer_tree_debug_state.cc (right):

https://codereview.chromium.org/12096112/diff/17019/cc/layer_tree_debug_state...
cc/layer_tree_debug_state.cc:76: 
Is there some reason we're not adding this new variable to
LayerTreeDebugState::equal() as well?

https://codereview.chromium.org/12096112/diff/17019/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12096112/diff/17019/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:996: 
I think this line should have indentation of only 2 spaces, right?

https://codereview.chromium.org/12096112/diff/17019/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1737: 
typo - Scren --> Screen

https://codereview.chromium.org/12096112/diff/17019/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12096112/diff/17019/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:15: 
Is it kosher to do this?   Another option is to make it a protected static
member variable?  Also, what happens with ubercomp here, will we have two
contenders for one static?  But I do agree with keeping unnecessary debug junk
out of a hot class...

https://codereview.chromium.org/12096112/diff/17019/cc/picture_layer_tiling.h
File cc/picture_layer_tiling.h (right):

https://codereview.chromium.org/12096112/diff/17019/cc/picture_layer_tiling.h...
cc/picture_layer_tiling.h:41: 
Sren --> Screen again

https://codereview.chromium.org/12096112/diff/17019/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12096112/diff/17019/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:428: 
Do you think it's helpful to dump the BasicStateAsValue() as part of this?  (and
maybe a slight rename like DetailedStateAsValue()? )

https://codereview.chromium.org/12096112/diff/17019/cc/tile_priority.h
File cc/tile_priority.h (right):

https://codereview.chromium.org/12096112/diff/17019/cc/tile_priority.h#newcod...
cc/tile_priority.h:130: 
I can't remember what we discussed - do we need to worry about this inflation
hurting performance?  Or is that a necessary compromise we're making for now, to
get better introspection for now?

[enne (OOO), 2013-02-12 00:21:13]

https://codereview.chromium.org/12096112/diff/12007/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12096112/diff/12007/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:15: static bool store_screen_space_quads_on_tiles =
false;
I agree with Shawn that this isn't great as a static.  Make this a
LayerTreeSetting and just set it as a local member on the tiling itself when you
create it from PictureLayerImpl.

https://codereview.chromium.org/12096112/diff/12007/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:20: void
PictureLayerTiling::SetStoreScrenSpaceQuadsOnTiles(bool b) {
Can you use a better variable name than b?

https://codereview.chromium.org/12096112/diff/12007/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:457: MathUtil::projectQuad(current_screen_transform,
In this case, you can just set_current_screen_quad(current_screen_rect) rather
than doing a projection.

https://codereview.chromium.org/12096112/diff/12007/cc/picture_layer_tiling.c...
cc/picture_layer_tiling.cc:503: gfx::QuadF(tile_bounds),
Here's your low res issue: s/tile_bounds/current_layer_content_rect/.  The same
issue applies above, although you don't have to do a projection there.

[nduca, 2013-02-12 00:54:44]

Re the static, you can't do it as a setting, its a runtime togglable thing. We
need it that way. Any other options that you can think of that are mutable?

[enne (OOO), 2013-02-12 01:00:04]

Is the debug state considered mutable?

[danakj, 2013-02-12 01:00:32]

On Mon, Feb 11, 2013 at 4:54 PM, <nduca@chromium.org> wrote:

> Re the static, you can't do it as a setting, its a runtime togglable
> thing. We
> need it that way. Any other options that you can think of that are mutable?
>

random thought: LayerTreeSettings::DebugState?

[nduca, 2013-02-12 01:54:52]

yeah, debug state is changable at runtime. Its my hope to not have this behidn a
commandline flag for long but rather be a checkbox in tracing or something like
that.

[nduca, 2013-02-12 03:15:26]

ptal, this new patch routes the debug state in.

[enne (OOO), 2013-02-12 03:35:51]

lgtm

[commit-bot: I haz the power, 2013-02-12 03:46:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/12096112/14005

[commit-bot: I haz the power, 2013-02-12 03:47:06]

Presubmit check for 12096112-14005 failed and returned exit status 1.

INFO:root:Found 31 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/cc/PRESUBMIT.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/renderer/gpu
    content/browser/renderer_host

Presubmit checks took 2.4s to calculate.

[nduca, 2013-02-12 04:03:07]

+piman for owners

[piman, 2013-02-12 04:55:07]

https://chromiumcodereview.appspot.com/12096112/diff/14005/content/renderer/g...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://chromiumcodereview.appspot.com/12096112/diff/14005/content/renderer/g...
content/renderer/gpu/gpu_benchmarking_extension.cc:472:
static_cast<WebLayerTreeViewImpl*>(render_view_impl->layerTreeView());
This is a content::RenderWidgetCompositor now... Might be worth adding an
accessor on RenderWidget so that we can use it without casting (protecting
ourselves against possible breakage - if this had landed before James's CL, we'd
have had an ugly boom.)

[jamesr, 2013-02-12 05:25:41]

On 2013/02/12 04:55:07, piman wrote:
>
https://chromiumcodereview.appspot.com/12096112/diff/14005/content/renderer/g...
> File content/renderer/gpu/gpu_benchmarking_extension.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12096112/diff/14005/content/renderer/g...
> content/renderer/gpu/gpu_benchmarking_extension.cc:472:
> static_cast<WebLayerTreeViewImpl*>(render_view_impl->layerTreeView());
> This is a content::RenderWidgetCompositor now... Might be worth adding an
> accessor on RenderWidget so that we can use it without casting (protecting
> ourselves against possible breakage - if this had landed before James's CL,
we'd
> have had an ugly boom.)

This will be a very ugly boom very soon now as well.  Add a getter to
content::RenderWidget

[nduca, 2013-02-12 05:29:57]

I'm actually going to remove the extension part from this patch. I can add in as
followup. PTAL

[jamesr, 2013-02-12 05:43:18]

content/renderer lgtm

(I don't have OWNERS for content/browser but that looks harmless as well)

[piman, 2013-02-12 06:30:49]

lgtm

[commit-bot: I haz the power, 2013-02-12 06:51:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/12096112/16019

[commit-bot: I haz the power, 2013-02-12 07:42:20]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 18:28:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/12096112/16019

[commit-bot: I haz the power, 2013-02-12 18:31:10]

Change committed as 181956