Don't aggregate quads outside of damage rect when using partial swap.

Don't aggregate quads outside of damage rect when using partial swap.

The quads will be scissored out, so aggregating them is a waste.
For me on a webpage with a tiny spinner this reduces DisplayScheduler::OnBeginFrameDeadline time by around 30%, and reduces the GPU process CPU time by around 50%.

BUG=490447
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/826aae36bc7da8a39f9b5eebefe9e51c59a63cfd
Cr-Commit-Position: refs/heads/master@{#338619}

[jbauman, 2015-06-10 01:39:52]

jbauman@chromium.org changed reviewers:
+ danakj@chromium.org

[jbauman, 2015-06-10 01:39:52]

Depends on https://codereview.chromium.org/1143403003/

[danakj, 2015-07-07 20:53:53]

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:309: if
(!quad_to_root_transform.GetInverse(&inverse_transform))
i don't think compositor is supposed to produce non-invertible transforms in the
frame. so you could just DCHECK the result was true here

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:346: if (aggregate_only_damaged_ &&
!has_copy_requests_) {
can you comment why you're checking has copy requests?

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:352: root_damage_rect_,
&damage_rect_in_quad_space);
you're overwriting the damage_rect_in_quad_space, but the
last_copied_source_shared_quad_state isn't changed, this looks like it would
cause us to use the wrong damage_rect_in_quad_space after a surface quad?

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:467: // calculate their combined damage rect.
and determine if they have copy requests?

at some point this function needs a more general name :)

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:588: has_copy_requests_ = false;
Do these need to be member variables? Can they be kept on the stack instead?
Maybe a struct passed through the recursion?

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.h (right):

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.h:97: bool aggregate_only_damaged_;
const?

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator_unittest.cc (right):

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_unittest.cc:1375:
aggregated_pass_list[0]->damage_rect.Contains(gfx::Rect(SurfaceSize())));
would EQ work here? then the error output is a bit nicer

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_unittest.cc:1411: EXPECT_EQ(gfx::Rect(10, 10, 2,
2).ToString(),
you don't need ToString for EXPECT_EQ with gfx::Rects

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surfaces_pi...
File cc/surfaces/surfaces_pixeltest.cc (right):

https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surfaces_pi...
cc/surfaces/surfaces_pixeltest.cc:86: SurfaceAggregator aggregator(&manager_,
resource_provider_.get(), false);
you could pass true for these?

[jbauman, 2015-07-08 23:29:15]

PTAL.

On 2015/07/07 20:53:53, danakj wrote:
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
> File cc/surfaces/surface_aggregator.cc (right):
> 
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
> cc/surfaces/surface_aggregator.cc:309: if
> (!quad_to_root_transform.GetInverse(&inverse_transform))
> i don't think compositor is supposed to produce non-invertible transforms in
the
> frame. so you could just DCHECK the result was true here

Ok, this seems to work.
> 
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
> cc/surfaces/surface_aggregator.cc:346: if (aggregate_only_damaged_ &&
> !has_copy_requests_) {
> can you comment why you're checking has copy requests?
> 
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
> cc/surfaces/surface_aggregator.cc:352: root_damage_rect_,
> &damage_rect_in_quad_space);
> you're overwriting the damage_rect_in_quad_space, but the
> last_copied_source_shared_quad_state isn't changed, this looks like it would
> cause us to use the wrong damage_rect_in_quad_space after a surface quad?

Good point. We should actually completely reset
last_copied_source_shared_quad_state.
> 
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
> cc/surfaces/surface_aggregator.cc:467: // calculate their combined damage
rect.
> and determine if they have copy requests?
> 
> at some point this function needs a more general name :)

Ok, added a somewhat more generic name.
> 
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
> cc/surfaces/surface_aggregator.cc:588: has_copy_requests_ = false;
> Do these need to be member variables? Can they be kept on the stack instead?
> Maybe a struct passed through the recursion?

I'd prefer to match the other member variables like dest_pass_list_ and so on.
There are a lot of functions they'd need to be threaded through otherwise, and
they're effectively global per aggregation.
> 
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
> File cc/surfaces/surface_aggregator.h (right):
> 
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
> cc/surfaces/surface_aggregator.h:97: bool aggregate_only_damaged_;
> const?
> 
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
> File cc/surfaces/surface_aggregator_unittest.cc (right):
> 
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
> cc/surfaces/surface_aggregator_unittest.cc:1375:
> aggregated_pass_list[0]->damage_rect.Contains(gfx::Rect(SurfaceSize())));
> would EQ work here? then the error output is a bit nicer

Done.
> 
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
> cc/surfaces/surface_aggregator_unittest.cc:1411: EXPECT_EQ(gfx::Rect(10, 10,
2,
> 2).ToString(),
> you don't need ToString for EXPECT_EQ with gfx::Rects
> 

Done.

>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surfaces_pi...
> File cc/surfaces/surfaces_pixeltest.cc (right):
> 
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surfaces_pi...
> cc/surfaces/surfaces_pixeltest.cc:86: SurfaceAggregator aggregator(&manager_,
> resource_provider_.get(), false);
> you could pass true for these?

Sure.

[danakj, 2015-07-13 18:48:55]

On Wed, Jul 8, 2015 at 4:29 PM, <jbauman@chromium.org> wrote:

> PTAL.
>
> On 2015/07/07 20:53:53, danakj wrote:
>
>
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
>
>> File cc/surfaces/surface_aggregator.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
>
>> cc/surfaces/surface_aggregator.cc:309: if
>> (!quad_to_root_transform.GetInverse(&inverse_transform))
>> i don't think compositor is supposed to produce non-invertible transforms
>> in
>>
> the
>
>> frame. so you could just DCHECK the result was true here
>>
>
> Ok, this seems to work.
>
>
>
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
>
>> cc/surfaces/surface_aggregator.cc:346: if (aggregate_only_damaged_ &&
>> !has_copy_requests_) {
>> can you comment why you're checking has copy requests?
>>
>
>
>
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
>
>> cc/surfaces/surface_aggregator.cc:352: root_damage_rect_,
>> &damage_rect_in_quad_space);
>> you're overwriting the damage_rect_in_quad_space, but the
>> last_copied_source_shared_quad_state isn't changed, this looks like it
>> would
>> cause us to use the wrong damage_rect_in_quad_space after a surface quad?
>>
>
> Good point. We should actually completely reset
> last_copied_source_shared_quad_state.
>
>
>
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
>
>> cc/surfaces/surface_aggregator.cc:467: // calculate their combined damage
>>
> rect.
>
>> and determine if they have copy requests?
>>
>
>  at some point this function needs a more general name :)
>>
>
> Ok, added a somewhat more generic name.
>
>
>
>
https://codereview.chromium.org/1172883004/diff/20001/cc/surfaces/surface_agg...
>
>> cc/surfaces/surface_aggregator.cc:588: has_copy_requests_ = false;
>> Do these need to be member variables? Can they be kept on the stack
>> instead?
>> Maybe a struct passed through the recursion?
>>
>
> I'd prefer to match the other member variables like dest_pass_list_ and so
> on.
> There are a lot of functions they'd need to be threaded through otherwise,
> and
> they're effectively global per aggregation.


Using a struct means you wouldn't need to have huge numbers of arguments
and update every site when you add/remove one at least.

This reminds me of the GLRenderer class before we had the DrawingFrame
structure. The class had a lot of state, and it was not clear what state
was part of the current frame, and what was long lived. It is a lot easier
to read and understand now IMO with the per-frame state in a temporary
struct that is passed around as it leaves no room for guessing.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-07-13 20:15:15]

LGTM but I'd really like to see the per-frame data in a on-the-stack struct in
the future.

https://codereview.chromium.org/1172883004/diff/60001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1172883004/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:305: gfx::Rect* quad_damage_rect) {
just return this instead of Rect*?

https://codereview.chromium.org/1172883004/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:324: const SharedQuadState*
last_copied_source_shared_quad_state = NULL;
nullptr while youre here?

https://codereview.chromium.org/1172883004/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:327: // the transform from that is invertible.
not relevant anymore?

https://codereview.chromium.org/1172883004/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:349: if (aggregate_only_damaged_ &&
!has_copy_requests_) {
can you put this in a temp bool at the top of the function and move the comment
there, then use that var here and below?

https://codereview.chromium.org/1172883004/diff/60001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.h (right):

https://codereview.chromium.org/1172883004/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.h:125: bool has_copy_requests_;
Can you comment about the lifetime/use of this variable

[jbauman, 2015-07-14 01:16:11]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2015-07-14 01:16:11]

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/1172883004/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-07-14 01:17:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1172883004/80001

[commit-bot: I haz the power, 2015-07-14 01:55:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-14 01:56:28]

Patchset 5 (id:??) landed as
https://crrev.com/826aae36bc7da8a39f9b5eebefe9e51c59a63cfd
Cr-Commit-Position: refs/heads/master@{#338619}