Only Record Ugliness Histogram for Compositor Side Changes

Only Record Ugliness Histogram for Compositor Side Changes

Set up a counter in LayerTreeImpl to indicate whether a recent frame has
been pushed from main thread. Since missing and incomplete tiles are
compositor side concept, only track those numbers on compositor side.

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

[weiliangc, 2014-07-22 22:01:13]

Attempt for only recording number for histogram on compositor thread.

https://codereview.chromium.org/411693004/diff/20001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/411693004/diff/20001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.h:192: if (frames_have_been_drawn_)
I use this check to make sure if TreeLayerImpl if has never been updated from
main thread, we are not going to count frames, and consequently not tracking
histogram info.

Is there a better way to achieve this?

https://codereview.chromium.org/411693004/diff/20001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.h:339: int frames_have_been_drawn_;
I'm not very happy about this variable name but not sure what else to call it?

[enne (OOO), 2014-07-22 22:35:38]

https://codereview.chromium.org/411693004/diff/20001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/411693004/diff/20001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.h:192: if (frames_have_been_drawn_)
On 2014/07/22 22:01:13, weiliangc wrote:
> I use this check to make sure if TreeLayerImpl if has never been updated from
> main thread, we are not going to count frames, and consequently not tracking
> histogram info.
> 
> Is there a better way to achieve this?

I'm not sure what you're worried about here.  If a LayerTreeImpl has never been
updated, then it won't have a root layer and can't draw.  (Is this a concrete
issue that you're running into or a more theoretical worry?)

I think you could make your variable a boolean "has_ever_been_drawn" and it'd be
more clear.

[weiliangc, 2014-07-22 23:12:42]

On 2014/07/22 22:35:38, enne wrote:
>
https://codereview.chromium.org/411693004/diff/20001/cc/trees/layer_tree_impl.h
> File cc/trees/layer_tree_impl.h (right):
> 
>
https://codereview.chromium.org/411693004/diff/20001/cc/trees/layer_tree_impl...
> cc/trees/layer_tree_impl.h:192: if (frames_have_been_drawn_)
> On 2014/07/22 22:01:13, weiliangc wrote:
> > I use this check to make sure if TreeLayerImpl if has never been updated
from
> > main thread, we are not going to count frames, and consequently not tracking
> > histogram info.
> > 
> > Is there a better way to achieve this?
> 
> I'm not sure what you're worried about here.  If a LayerTreeImpl has never
been
> updated, then it won't have a root layer and can't draw.  (Is this a concrete
> issue that you're running into or a more theoretical worry?)
> 
> I think you could make your variable a boolean "has_ever_been_drawn" and it'd
be
> more clear.

When I used a bool, default to false, set to true in DrawLayer, and resets to
false at PushPropertiesTo, I see a LayerImpl that never get PushPropertiesTo
called upon, thus always true and always recording number, which I assume is the
single thread browser compositor? I was trying to avoid this situation.

[enne (OOO), 2014-07-22 23:17:27]

Ah, probably because LayerTreeImpl::PushPropertiesTo never got called because
there's no impl-side painting? Maybe you should reset the sync tree during
commit as well in case the sync tree is the active tree?

[weiliangc, 2014-07-22 23:27:06]

On 2014/07/22 23:17:27, enne wrote:
> Ah, probably because LayerTreeImpl::PushPropertiesTo never got called because
> there's no impl-side painting? Maybe you should reset the sync tree during
> commit as well in case the sync tree is the active tree?

It's a mixed case when I run cros build on linux. Some LTI goes through cycle of
PushPropertiesTo->DrawLayer, one LTI only get called to DrawLayer all the time.
I thought the LTI without be PushPropertiesTo must be browser compositor and
shouldn't be taken into account for histogram?

[enne (OOO), 2014-07-22 23:31:27]

On 2014/07/22 23:27:06, weiliangc wrote:
> On 2014/07/22 23:17:27, enne wrote:
> > Ah, probably because LayerTreeImpl::PushPropertiesTo never got called
because
> > there's no impl-side painting? Maybe you should reset the sync tree during
> > commit as well in case the sync tree is the active tree?
> 
> It's a mixed case when I run cros build on linux. Some LTI goes through cycle
of
> PushPropertiesTo->DrawLayer, one LTI only get called to DrawLayer all the
time.
> I thought the LTI without be PushPropertiesTo must be browser compositor and
> shouldn't be taken into account for histogram?

Yeah, that would be exactly what I would expect.  However, I don't think "is
using impl-side painting" is a correct way to differentiate the browser
compositor.

[weiliangc, 2014-07-22 23:44:09]

On 2014/07/22 23:31:27, enne wrote:
> On 2014/07/22 23:27:06, weiliangc wrote:
> > On 2014/07/22 23:17:27, enne wrote:
> > > Ah, probably because LayerTreeImpl::PushPropertiesTo never got called
> because
> > > there's no impl-side painting? Maybe you should reset the sync tree during
> > > commit as well in case the sync tree is the active tree?
> > 
> > It's a mixed case when I run cros build on linux. Some LTI goes through
cycle
> of
> > PushPropertiesTo->DrawLayer, one LTI only get called to DrawLayer all the
> time.
> > I thought the LTI without be PushPropertiesTo must be browser compositor and
> > shouldn't be taken into account for histogram?
> 
> Yeah, that would be exactly what I would expect.  However, I don't think "is
> using impl-side painting" is a correct way to differentiate the browser
> compositor.

Ah yes, I was trying to ask for a way to better differentiate browser compositor
*handwaving* but failed to articulate it. (also not 100% sure I was right in
believing it is browser compositor)

Do you mean reset bool on sync_tree in LTH::FinishCommitOnImplThread?

[enne (OOO), 2014-07-22 23:46:05]

On 2014/07/22 23:44:09, weiliangc wrote:

> Do you mean reset bool on sync_tree in LTH::FinishCommitOnImplThread?

Yeah, exactly.

[weiliangc, 2014-07-23 00:16:08]

On 2014/07/22 23:46:05, enne wrote:
> On 2014/07/22 23:44:09, weiliangc wrote:
> 
> > Do you mean reset bool on sync_tree in LTH::FinishCommitOnImplThread?
> 
> Yeah, exactly.

Done. :)

[enne (OOO), 2014-07-23 00:28:26]

lgtm

[weiliangc, 2014-07-23 14:41:11]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-07-23 14:42:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/weiliangc@chromium.org/411693004/40001

[commit-bot: I haz the power, 2014-07-23 19:18:57]

Change committed as 285010