cc: Ensure that large damage doesn't register as "frame has no damage"

cc: Ensure that large damage doesn't register as "frame has no damage"

When we have accumulating damage from really large or really far rects
we can't union them correctly. So instead, introduce a DamageAccumulator
that detects this and an additional function ShouldDamageEverything
that ensures that if we run into this situation, we just damage
everything we can.

R=danakj@chromium.org, enne@chromium.org
BUG=678432
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2632463005
Cr-Commit-Position: refs/heads/master@{#445208}
Committed: https://chromium.googlesource.com/chromium/src/+/efb8e7ef70d18b7380210e7244113653ab41cf42

[vmpstr, 2017-01-13 21:55:08]

Description was changed from

==========
cc: Ensure that large damage doesn't register as "frame has no damage"

When we have accumulating damage from really large or really far rects
we can't union them correctly. So instead, introduce a DamageAccumulator
that detects this and an additional function ShouldDamageEverything
that ensures that if we run into this situation, we just damage
everything we can.

R=danakj@chromium.org, enne@chromium.org
BUG=678432
==========

to

==========
cc: Ensure that large damage doesn't register as "frame has no damage"

When we have accumulating damage from really large or really far rects
we can't union them correctly. So instead, introduce a DamageAccumulator
that detects this and an additional function ShouldDamageEverything
that ensures that if we run into this situation, we just damage
everything we can.

R=danakj@chromium.org, enne@chromium.org
BUG=678432
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2017-01-13 22:01:50]

Please take a look.

[enne (OOO), 2017-01-13 22:21:38]

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc
File cc/trees/damage_tracker.cc (right):

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc#...
cc/trees/damage_tracker.cc:406: target_damage->SetInvalid();
Should you use the render surface content rect as the damage rect in this case,
instead of propagating the invalid state?

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker_uni...
File cc/trees/damage_tracker_unittest.cc (right):

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker_uni...
cc/trees/damage_tracker_unittest.cc:1588: TEST_F(DamageTrackerTest,
DamageRectTooBig) {
wtb DamageRectTooBigInRenderSurface

[danakj, 2017-01-13 23:17:59]

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc
File cc/trees/damage_tracker.cc (right):

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc#...
cc/trees/damage_tracker.cc:145: return !current_damage_.GetAsRect(&rect);
I thot it a bit odd this computes a rect to throw it away, and compute it again
below. I also noticed all callers of ShouldDamageEverything then call
CurrentDamageRect after and vice versa.. So maybe they should be one function
that returns bool and rect also?

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc#...
cc/trees/damage_tracker.cc:439: *rect = gfx::Rect();
think not needed? dont use the rect if return is false

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc#...
cc/trees/damage_tracker.cc:443: double width = right_ - static_cast<double>(x_);
does not base::CheckedNumeric do this stuff and you should use that?

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc#...
cc/trees/damage_tracker.cc:444: if (width >
static_cast<double>(std::numeric_limits<int>::max())) {
if not, can you store the intmax as a double in a constexr variable?

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.h
File cc/trees/damage_tracker.h (right):

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.h#n...
cc/trees/damage_tracker.h:64: if (rect.IsEmpty())
why is this below setting everything if we are already empty?

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.h#n...
cc/trees/damage_tracker.h:103: // These helper functions are used only in
TrackDamageFromActiveLayers().
ExpandDamageInsideRectWithFilters can be grouped with these? And maybe take
|damage| as last parameter, out params should be last.

[vmpstr, 2017-01-19 23:08:27]

Please take another look.

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc
File cc/trees/damage_tracker.cc (right):

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc#...
cc/trees/damage_tracker.cc:145: return !current_damage_.GetAsRect(&rect);
On 2017/01/13 23:17:58, danakj (slow) wrote:
> I thot it a bit odd this computes a rect to throw it away, and compute it
again
> below. I also noticed all callers of ShouldDamageEverything then call
> CurrentDamageRect after and vice versa.. So maybe they should be one function
> that returns bool and rect also?

Done. I with with bool GetRect(Rect*) type of function, but let me know if you
prefer something different like returning a pair or maybe returning a rect and
taking an optional bool*

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc#...
cc/trees/damage_tracker.cc:406: target_damage->SetInvalid();
On 2017/01/13 22:21:38, enne wrote:
> Should you use the render surface content rect as the damage rect in this
case,
> instead of propagating the invalid state?

Done.

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc#...
cc/trees/damage_tracker.cc:439: *rect = gfx::Rect();
On 2017/01/13 23:17:58, danakj (slow) wrote:
> think not needed? dont use the rect if return is false

Done.

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc#...
cc/trees/damage_tracker.cc:443: double width = right_ - static_cast<double>(x_);
On 2017/01/13 23:17:58, danakj (slow) wrote:
> does not base::CheckedNumeric do this stuff and you should use that?

Done.

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.cc#...
cc/trees/damage_tracker.cc:444: if (width >
static_cast<double>(std::numeric_limits<int>::max())) {
On 2017/01/13 23:17:58, danakj (slow) wrote:
> if not, can you store the intmax as a double in a constexr variable?

Removed this.

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.h
File cc/trees/damage_tracker.h (right):

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.h#n...
cc/trees/damage_tracker.h:64: if (rect.IsEmpty())
On 2017/01/13 23:17:58, danakj (slow) wrote:
> why is this below setting everything if we are already empty?

Moved it up.

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker.h#n...
cc/trees/damage_tracker.h:103: // These helper functions are used only in
TrackDamageFromActiveLayers().
On 2017/01/13 23:17:59, danakj (slow) wrote:
> ExpandDamageInsideRectWithFilters can be grouped with these? And maybe take
> |damage| as last parameter, out params should be last.

Done.

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker_uni...
File cc/trees/damage_tracker_unittest.cc (right):

https://codereview.chromium.org/2632463005/diff/1/cc/trees/damage_tracker_uni...
cc/trees/damage_tracker_unittest.cc:1588: TEST_F(DamageTrackerTest,
DamageRectTooBig) {
On 2017/01/13 22:21:38, enne wrote:
> wtb DamageRectTooBigInRenderSurface

Done.

[enne (OOO), 2017-01-19 23:23:39]

Two more small comments:

https://codereview.chromium.org/2632463005/diff/20001/cc/debug/debug_rect_his...
File cc/debug/debug_rect_history.cc (right):

https://codereview.chromium.org/2632463005/diff/20001/cc/debug/debug_rect_his...
cc/debug/debug_rect_history.cc:130:
render_surface->damage_tracker()->GetDamageRectIfValid(&damage_rect);
Maybe encapsulate this conditional block in a RenderSurfaceImpl::GetDamageRect()
function since you do it three times?

https://codereview.chromium.org/2632463005/diff/20001/cc/trees/damage_tracker.cc
File cc/trees/damage_tracker.cc (right):

https://codereview.chromium.org/2632463005/diff/20001/cc/trees/damage_tracker...
cc/trees/damage_tracker.cc:292: void
DamageTracker::ExpandDamageInsideRectWithFilters(
Could you add a test with an invalid rect and a moving pixel filter?

[danakj, 2017-01-20 17:03:04]

LGTM % enne

https://codereview.chromium.org/2632463005/diff/20001/cc/debug/debug_rect_his...
File cc/debug/debug_rect_history.cc (right):

https://codereview.chromium.org/2632463005/diff/20001/cc/debug/debug_rect_his...
cc/debug/debug_rect_history.cc:130:
render_surface->damage_tracker()->GetDamageRectIfValid(&damage_rect);
On 2017/01/19 23:23:39, enne wrote:
> Maybe encapsulate this conditional block in a
RenderSurfaceImpl::GetDamageRect()
> function since you do it three times?

My one thot on this is that then you have 2 ways to get a damage rect, one on
RSI and one on it's DamageTracker. It might be nice to not allow access to the
DamageTracker directly, but there's like 3 or 4 other methods we use from
LayerTreeHostImpl so idk..

[vmpstr, 2017-01-20 21:38:18]

Please take another look.

https://codereview.chromium.org/2632463005/diff/20001/cc/debug/debug_rect_his...
File cc/debug/debug_rect_history.cc (right):

https://codereview.chromium.org/2632463005/diff/20001/cc/debug/debug_rect_his...
cc/debug/debug_rect_history.cc:130:
render_surface->damage_tracker()->GetDamageRectIfValid(&damage_rect);
On 2017/01/19 23:23:39, enne wrote:
> Maybe encapsulate this conditional block in a
RenderSurfaceImpl::GetDamageRect()
> function since you do it three times?

Done.

https://codereview.chromium.org/2632463005/diff/20001/cc/debug/debug_rect_his...
cc/debug/debug_rect_history.cc:130:
render_surface->damage_tracker()->GetDamageRectIfValid(&damage_rect);
On 2017/01/20 17:03:04, danakj (slow) wrote:
> On 2017/01/19 23:23:39, enne wrote:
> > Maybe encapsulate this conditional block in a
> RenderSurfaceImpl::GetDamageRect()
> > function since you do it three times?
> 
> My one thot on this is that then you have 2 ways to get a damage rect, one on
> RSI and one on it's DamageTracker. It might be nice to not allow access to the
> DamageTracker directly, but there's like 3 or 4 other methods we use from
> LayerTreeHostImpl so idk..

I think it still makes sense, since the two accessors have slightly different
results: The RS one always returns a rect (if the damage was invalid, it just
returns the content rect). However, the damage tracker one provides more fine
grained information: either a rect, or information that the rect wasn't valid.

I agree that hiding damage tracker might be nice, but then that means adding
some forwarding functions to the render surface, which I'd like to avoid in this
patch.

https://codereview.chromium.org/2632463005/diff/20001/cc/trees/damage_tracker.cc
File cc/trees/damage_tracker.cc (right):

https://codereview.chromium.org/2632463005/diff/20001/cc/trees/damage_tracker...
cc/trees/damage_tracker.cc:292: void
DamageTracker::ExpandDamageInsideRectWithFilters(
On 2017/01/19 23:23:39, enne wrote:
> Could you add a test with an invalid rect and a moving pixel filter?

Done.

[enne (OOO), 2017-01-20 22:15:13]

lgtm

https://codereview.chromium.org/2632463005/diff/40001/cc/debug/debug_rect_his...
File cc/debug/debug_rect_history.cc (right):

https://codereview.chromium.org/2632463005/diff/40001/cc/debug/debug_rect_his...
cc/debug/debug_rect_history.cc:130:
render_surface->damage_tracker()->GetDamageRectIfValid(&damage_rect);
Maybe use RenderSurfaceImpl::GetDamageRect here?

[vmpstr, 2017-01-20 22:41:51]

https://codereview.chromium.org/2632463005/diff/40001/cc/debug/debug_rect_his...
File cc/debug/debug_rect_history.cc (right):

https://codereview.chromium.org/2632463005/diff/40001/cc/debug/debug_rect_his...
cc/debug/debug_rect_history.cc:130:
render_surface->damage_tracker()->GetDamageRectIfValid(&damage_rect);
On 2017/01/20 22:15:13, enne wrote:
> Maybe use RenderSurfaceImpl::GetDamageRect here?

Err missed this one, thanks

[vmpstr, 2017-01-20 22:42:04]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2017-01-20 22:42:04]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2632463005/#ps60001
(title: "update")

[commit-bot: I haz the power, 2017-01-20 22:42:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 23:36:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484952124511980,
"parent_rev": "e9d917487af2ce004e9b15acde07b25290356cef", "commit_rev":
"efb8e7ef70d18b7380210e7244113653ab41cf42"}

[commit-bot: I haz the power, 2017-01-20 23:36:53]

Description was changed from

==========
cc: Ensure that large damage doesn't register as "frame has no damage"

When we have accumulating damage from really large or really far rects
we can't union them correctly. So instead, introduce a DamageAccumulator
that detects this and an additional function ShouldDamageEverything
that ensures that if we run into this situation, we just damage
everything we can.

R=danakj@chromium.org, enne@chromium.org
BUG=678432
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Ensure that large damage doesn't register as "frame has no damage"

When we have accumulating damage from really large or really far rects
we can't union them correctly. So instead, introduce a DamageAccumulator
that detects this and an additional function ShouldDamageEverything
that ensures that if we run into this situation, we just damage
everything we can.

R=danakj@chromium.org, enne@chromium.org
BUG=678432
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2632463005
Cr-Commit-Position: refs/heads/master@{#445208}
Committed:
https://chromium.googlesource.com/chromium/src/+/efb8e7ef70d18b7380210e724411...
==========

[commit-bot: I haz the power, 2017-01-20 23:36:54]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/efb8e7ef70d18b7380210e724411...