Calculate damage rects before aggregating frame data.

Calculate damage rects before aggregating frame data.

Walk the entire Surface tree ahead of normal aggregation to determine Surface validity and calculate damage rect. This will later allow us to avoid aggregating quads that are outside the damage rect.

BUG=490447
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/3bd7e0f50dce1e4d66027de1e7175d4f75762f8f
Cr-Commit-Position: refs/heads/master@{#337538}

[jbauman, 2015-06-09 03:03:35]

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

[danakj, 2015-06-09 16:16:49]

what's this do for thread_times?
On Jun 8, 2015 8:03 PM, <jbauman@chromium.org> wrote:

> Reviewers: danakj,
>
> Description:
> Calculate damage rects before aggregating frame data.
>
> Walk the entire Surface tree ahead of normal aggregation to determine
> Surface
> validity and calculate damage rect. This will later allow us to avoid
> aggregating quads that are outside the damage rect.
>
> BUG=490447
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
>
> Please review this at https://codereview.chromium.org/1143403003/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@fixremap
>
> Affected files (+114, -83 lines):
>   M cc/surfaces/surface_aggregator.h
>   M cc/surfaces/surface_aggregator.cc
>
>
>

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

[jbauman, 2015-06-09 23:23:59]

Currently working on getting that number. For SurfaceAggregator cc_perftests the
difference seems to be within the noise (maybe 3% worse), as expected.

On 2015/06/09 16:16:49, danakj wrote:
> what's this do for thread_times?
> On Jun 8, 2015 8:03 PM, <mailto:jbauman@chromium.org> wrote:
> 
> > Reviewers: danakj,
> >
> > Description:
> > Calculate damage rects before aggregating frame data.
> >
> > Walk the entire Surface tree ahead of normal aggregation to determine
> > Surface
> > validity and calculate damage rect. This will later allow us to avoid
> > aggregating quads that are outside the damage rect.
> >
> > BUG=490447
> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> >
> > Please review this at https://codereview.chromium.org/1143403003/
> >
> > Base URL: https://chromium.googlesource.com/chromium/src.git@fixremap
> >
> > Affected files (+114, -83 lines):
> >   M cc/surfaces/surface_aggregator.h
> >   M cc/surfaces/surface_aggregator.cc
> >
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-06-10 17:56:46]

On Tue, Jun 9, 2015 at 4:23 PM, <jbauman@chromium.org> wrote:

> Currently working on getting that number. For SurfaceAggregator
> cc_perftests the
> difference seems to be within the noise (maybe 3% worse), as expected.


Cool, were you still working on numbers here?


>
>
> On 2015/06/09 16:16:49, danakj wrote:
>
>> what's this do for thread_times?
>> On Jun 8, 2015 8:03 PM, <mailto:jbauman@chromium.org> wrote:
>>
>
>  > Reviewers: danakj,
>> >
>> > Description:
>> > Calculate damage rects before aggregating frame data.
>> >
>> > Walk the entire Surface tree ahead of normal aggregation to determine
>> > Surface
>> > validity and calculate damage rect. This will later allow us to avoid
>> > aggregating quads that are outside the damage rect.
>> >
>> > BUG=490447
>> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
>> >
>> > Please review this at https://codereview.chromium.org/1143403003/
>> >
>> > Base URL: https://chromium.googlesource.com/chromium/src.git@fixremap
>> >
>> > Affected files (+114, -83 lines):
>> >   M cc/surfaces/surface_aggregator.h
>> >   M cc/surfaces/surface_aggregator.cc
>> >
>> >
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/1143403003/
>

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

[jbauman, 2015-06-10 23:17:05]

On 2015/06/10 17:56:46, danakj wrote:
> On Tue, Jun 9, 2015 at 4:23 PM, <mailto:jbauman@chromium.org> wrote:
> 
> > Currently working on getting that number. For SurfaceAggregator
> > cc_perftests the
> > difference seems to be within the noise (maybe 3% worse), as expected.
> 
> 
> Cool, were you still working on numbers here?

No change in thread_times on my system, and I've had some issues with the perf
trybots so I'm giving up on them.
> 
> 
> >
> >
> > On 2015/06/09 16:16:49, danakj wrote:
> >
> >> what's this do for thread_times?
> >> On Jun 8, 2015 8:03 PM, <mailto:jbauman@chromium.org> wrote:
> >>
> >
> >  > Reviewers: danakj,
> >> >
> >> > Description:
> >> > Calculate damage rects before aggregating frame data.
> >> >
> >> > Walk the entire Surface tree ahead of normal aggregation to determine
> >> > Surface
> >> > validity and calculate damage rect. This will later allow us to avoid
> >> > aggregating quads that are outside the damage rect.
> >> >
> >> > BUG=490447
> >> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> >> >
> >> > Please review this at https://codereview.chromium.org/1143403003/
> >> >
> >> > Base URL: https://chromium.googlesource.com/chromium/src.git@fixremap
> >> >
> >> > Affected files (+114, -83 lines):
> >> >   M cc/surfaces/surface_aggregator.h
> >> >   M cc/surfaces/surface_aggregator.cc
> >> >
> >> >
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> >
> > https://codereview.chromium.org/1143403003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[jbauman, 2015-06-29 22:27:47]

danakj, ping

[danakj, 2015-07-06 22:08:43]

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

https://codereview.chromium.org/1143403003/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:503: if
(!frame_data->render_pass_list.empty())
{}

https://codereview.chromium.org/1143403003/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:512:
MathUtil::MapEnclosingClippedRect(surface_info.second, surface_damage));
Does this do the right thing with the transform?

The transform in second is to the root. So if you had a child surface, that
function would return a damage rect in the root. Then the parent would use that
and map it from its own space to the root (again), putting it in the wrong
place?

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

https://codereview.chromium.org/1143403003/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.h:115: base::hash_map<SurfaceId, bool>
surface_validity_;
how about a hash_set and you're valid if you're in it?

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

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

https://codereview.chromium.org/1143403003/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:503: if
(!frame_data->render_pass_list.empty())
On 2015/07/06 22:08:43, danakj wrote:
> {}

Done.

https://codereview.chromium.org/1143403003/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:512:
MathUtil::MapEnclosingClippedRect(surface_info.second, surface_damage));
On 2015/07/06 22:08:43, danakj wrote:
> Does this do the right thing with the transform?
> 
> The transform in second is to the root. So if you had a child surface, that
> function would return a damage rect in the root. Then the parent would use
that
> and map it from its own space to the root (again), putting it in the wrong
> place?

The transform in the second is from the quad space to the root of this surface
(not the overall global root) - in particular transform_to_root_target is always
the identity for the last renderpass in a Surface. The root space of the child
surface is the quad space of its parent surface, so this chain of
transformations should work.

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

https://codereview.chromium.org/1143403003/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.h:115: base::hash_map<SurfaceId, bool>
surface_validity_;
On 2015/07/06 22:08:43, danakj wrote:
> how about a hash_set and you're valid if you're in it?

Done.

[danakj, 2015-07-06 23:23:26]

LGTM w/comments

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

https://codereview.chromium.org/1143403003/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:472: gfx::Transform target_to_root_transform(
maybe name this target_to_surface_transform then?

https://codereview.chromium.org/1143403003/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:482:
ResourceProvider::ResourceIdMap::const_iterator it =
this variable name shadows the one defined at L434

https://codereview.chromium.org/1143403003/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:515: referenced_surfaces_.erase(it);
It's very unclear to me why the insert/erase is happening. Can you do them
around just the pieces of code that need it? And if that's not something very
small, can you comment on the insert explaining what will use it?

https://codereview.chromium.org/1143403003/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:538: SurfaceSet::iterator it =
referenced_surfaces_.insert(surface_id).first;
Can you group the insert/erase around the CopyPasses call without whitespace in
between so it's obvious it's being done just for it?

[jbauman, 2015-07-06 23:42:56]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2015-07-06 23:42:56]

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

[commit-bot: I haz the power, 2015-07-06 23:44:30]

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

[commit-bot: I haz the power, 2015-07-07 00:53:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-07 00:54:11]

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