cc: Unify the occlusion access by adding an ability to scale transform.

cc: Unify the occlusion access by adding an ability to scale transform.

This change adds GetOcclusionWithGivenDrawTransform to Occlusion. This
allows us to get occlusion tracker out of the layer by always passing
an occlusion with a draw transform. Then, each layer can still get
a more convenient version of the occlusion by providing a different
draw transform.

R=danakj

Committed: https://crrev.com/61c35e8db648a01dc75b7ab04af454e0fec9dfba
Cr-Commit-Position: refs/heads/master@{#298403}

[vmpstr, 2014-10-03 18:34:02]

PTAL

[danakj, 2014-10-03 18:49:42]

Can you add a unit test for Occlusion?
On Oct 3, 2014 2:34 PM, <vmpstr@chromium.org> wrote:

> Reviewers: danakj,
>
> Message:
> PTAL
>
> Description:
> cc: Unify the occlusion access by adding an ability to scale transform.
>
> This change adds GetOcclusionWithScaledDrawTransform to Occlusion. This
> allows us to get occlusion tracker out of the layer by always passing
> an occlusion with a draw transform. Then, each layer can still get
> a more convenient version of the occlusion by scaling the internal
> transform.
>
> R=danakj
>
> Please review this at https://codereview.chromium.org/607023003/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+24, -14 lines):
>   M cc/layers/picture_layer_impl.cc
>   M cc/trees/occlusion.h
>   M cc/trees/occlusion.cc
>
>
> Index: cc/layers/picture_layer_impl.cc
> diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_
> impl.cc
> index 8a78bb1a8e2c9cce50a8837696e64e43a9f748aa..
> fc4a41517dcb496a7bd37834def05548b4dcd19d 100644
> --- a/cc/layers/picture_layer_impl.cc
> +++ b/cc/layers/picture_layer_impl.cc
> @@ -165,30 +165,30 @@ void PictureLayerImpl::AppendQuads(
>      AppendDebugBorderQuad(
>          render_pass, content_bounds(), shared_quad_state,
> append_quads_data);
>
> -    SolidColorLayerImpl::AppendSolidQuads(
> -        render_pass,
> -        occlusion_tracker,
> -        shared_quad_state,
> -        content_bounds(),
> -        draw_properties().target_space_transform,
> -        pile_->solid_color(),
> -        append_quads_data);
> +    SolidColorLayerImpl::AppendSolidQuads(render_pass,
> +                                          occlusion_tracker,
> +                                          shared_quad_state,
> +                                          content_bounds(),
> +                                          draw_transform(),
> +                                          pile_->solid_color(),
> +                                          append_quads_data);
>      return;
>    }
>
> +  Occlusion occlusion =
> +      occlusion_tracker.GetCurrentOcclusionForLayer(draw_transform());
> +
>    float max_contents_scale = MaximumTilingContentsScale();
>    gfx::Transform scaled_draw_transform = draw_transform();
>    scaled_draw_transform.Scale(SK_MScalar1 / max_contents_scale,
>                                SK_MScalar1 / max_contents_scale);
>    gfx::Size scaled_content_bounds =
>        gfx::ToCeiledSize(gfx::ScaleSize(content_bounds(),
> max_contents_scale));
> -
>    gfx::Rect scaled_visible_content_rect =
>        gfx::ScaleToEnclosingRect(visible_content_rect(),
> max_contents_scale);
>    scaled_visible_content_rect.Intersect(gfx::Rect(scaled_
> content_bounds));
> -
> -  Occlusion occlusion =
> -      occlusion_tracker.GetCurrentOcclusionForLayer(
> scaled_draw_transform);
> +  Occlusion scaled_occlusion = occlusion.GetOcclusionWithScaledDrawTran
> sform(
> +      SK_MScalar1 / max_contents_scale, SK_MScalar1 / max_contents_scale);
>
>    shared_quad_state->SetAll(scaled_draw_transform,
>                              scaled_content_bounds,
> @@ -211,7 +211,7 @@ void PictureLayerImpl::AppendQuads(
>      gfx::Rect geometry_rect = scaled_visible_content_rect;
>      gfx::Rect opaque_rect = contents_opaque() ? geometry_rect :
> gfx::Rect();
>      gfx::Rect visible_geometry_rect =
> -        occlusion.GetUnoccludedContentRect(geometry_rect);
> +        scaled_occlusion.GetUnoccludedContentRect(geometry_rect);
>      if (visible_geometry_rect.IsEmpty())
>        return;
>
> @@ -306,7 +306,7 @@ void PictureLayerImpl::AppendQuads(
>      gfx::Rect geometry_rect = iter.geometry_rect();
>      gfx::Rect opaque_rect = contents_opaque() ? geometry_rect :
> gfx::Rect();
>      gfx::Rect visible_geometry_rect =
> -        occlusion.GetUnoccludedContentRect(geometry_rect);
> +        scaled_occlusion.GetUnoccludedContentRect(geometry_rect);
>      if (visible_geometry_rect.IsEmpty())
>        continue;
>
> Index: cc/trees/occlusion.cc
> diff --git a/cc/trees/occlusion.cc b/cc/trees/occlusion.cc
> index d9da9382bcafc6788128ba25488412599dc11356..
> 2daae7a24d814be66957379ffa9630c1f3da9b93 100644
> --- a/cc/trees/occlusion.cc
> +++ b/cc/trees/occlusion.cc
> @@ -20,6 +20,15 @@ Occlusion::Occlusion(const gfx::Transform&
> draw_transform,
>        occlusion_from_inside_target_(occlusion_from_inside_target) {
>  }
>
> +Occlusion Occlusion::GetOcclusionWithScaledDrawTransform(SkMScalar x,
> +                                                         SkMScalar y) {
> +  gfx::Transform scaled_transform = draw_transform_;
> +  scaled_transform.Scale(x, y);
> +  return Occlusion(scaled_transform,
> +                   occlusion_from_outside_target_,
> +                   occlusion_from_inside_target_);
> +}
> +
>  bool Occlusion::IsOccluded(const gfx::Rect& content_rect) const {
>    if (content_rect.IsEmpty())
>      return true;
> Index: cc/trees/occlusion.h
> diff --git a/cc/trees/occlusion.h b/cc/trees/occlusion.h
> index 60493cc2f7d5ff737469cd84adfa8a8daf5a3a4f..
> 7802d81ae5117bb518b9c59e9daa62c680ef3794 100644
> --- a/cc/trees/occlusion.h
> +++ b/cc/trees/occlusion.h
> @@ -19,6 +19,7 @@ class CC_EXPORT Occlusion {
>    Occlusion(const gfx::Transform& draw_transform,
>              const SimpleEnclosedRegion& occlusion_from_outside_target,
>              const SimpleEnclosedRegion& occlusion_from_inside_target);
> +  Occlusion GetOcclusionWithScaledDrawTransform(SkMScalar x, SkMScalar
> y);
>
>    bool IsOccluded(const gfx::Rect& content_rect) const;
>    gfx::Rect GetUnoccludedContentRect(const gfx::Rect& content_rect) const;
>
>
>

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

[danakj, 2014-10-03 18:52:16]

And how about an override with only 1 scale that calls the other so less
repeating in PLI?
On Oct 3, 2014 2:34 PM, <vmpstr@chromium.org> wrote:

> Reviewers: danakj,
>
> Message:
> PTAL
>
> Description:
> cc: Unify the occlusion access by adding an ability to scale transform.
>
> This change adds GetOcclusionWithScaledDrawTransform to Occlusion. This
> allows us to get occlusion tracker out of the layer by always passing
> an occlusion with a draw transform. Then, each layer can still get
> a more convenient version of the occlusion by scaling the internal
> transform.
>
> R=danakj
>
> Please review this at https://codereview.chromium.org/607023003/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+24, -14 lines):
>   M cc/layers/picture_layer_impl.cc
>   M cc/trees/occlusion.h
>   M cc/trees/occlusion.cc
>
>
> Index: cc/layers/picture_layer_impl.cc
> diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_
> impl.cc
> index 8a78bb1a8e2c9cce50a8837696e64e43a9f748aa..
> fc4a41517dcb496a7bd37834def05548b4dcd19d 100644
> --- a/cc/layers/picture_layer_impl.cc
> +++ b/cc/layers/picture_layer_impl.cc
> @@ -165,30 +165,30 @@ void PictureLayerImpl::AppendQuads(
>      AppendDebugBorderQuad(
>          render_pass, content_bounds(), shared_quad_state,
> append_quads_data);
>
> -    SolidColorLayerImpl::AppendSolidQuads(
> -        render_pass,
> -        occlusion_tracker,
> -        shared_quad_state,
> -        content_bounds(),
> -        draw_properties().target_space_transform,
> -        pile_->solid_color(),
> -        append_quads_data);
> +    SolidColorLayerImpl::AppendSolidQuads(render_pass,
> +                                          occlusion_tracker,
> +                                          shared_quad_state,
> +                                          content_bounds(),
> +                                          draw_transform(),
> +                                          pile_->solid_color(),
> +                                          append_quads_data);
>      return;
>    }
>
> +  Occlusion occlusion =
> +      occlusion_tracker.GetCurrentOcclusionForLayer(draw_transform());
> +
>    float max_contents_scale = MaximumTilingContentsScale();
>    gfx::Transform scaled_draw_transform = draw_transform();
>    scaled_draw_transform.Scale(SK_MScalar1 / max_contents_scale,
>                                SK_MScalar1 / max_contents_scale);
>    gfx::Size scaled_content_bounds =
>        gfx::ToCeiledSize(gfx::ScaleSize(content_bounds(),
> max_contents_scale));
> -
>    gfx::Rect scaled_visible_content_rect =
>        gfx::ScaleToEnclosingRect(visible_content_rect(),
> max_contents_scale);
>    scaled_visible_content_rect.Intersect(gfx::Rect(scaled_
> content_bounds));
> -
> -  Occlusion occlusion =
> -      occlusion_tracker.GetCurrentOcclusionForLayer(
> scaled_draw_transform);
> +  Occlusion scaled_occlusion = occlusion.GetOcclusionWithScaledDrawTran
> sform(
> +      SK_MScalar1 / max_contents_scale, SK_MScalar1 / max_contents_scale);
>
>    shared_quad_state->SetAll(scaled_draw_transform,
>                              scaled_content_bounds,
> @@ -211,7 +211,7 @@ void PictureLayerImpl::AppendQuads(
>      gfx::Rect geometry_rect = scaled_visible_content_rect;
>      gfx::Rect opaque_rect = contents_opaque() ? geometry_rect :
> gfx::Rect();
>      gfx::Rect visible_geometry_rect =
> -        occlusion.GetUnoccludedContentRect(geometry_rect);
> +        scaled_occlusion.GetUnoccludedContentRect(geometry_rect);
>      if (visible_geometry_rect.IsEmpty())
>        return;
>
> @@ -306,7 +306,7 @@ void PictureLayerImpl::AppendQuads(
>      gfx::Rect geometry_rect = iter.geometry_rect();
>      gfx::Rect opaque_rect = contents_opaque() ? geometry_rect :
> gfx::Rect();
>      gfx::Rect visible_geometry_rect =
> -        occlusion.GetUnoccludedContentRect(geometry_rect);
> +        scaled_occlusion.GetUnoccludedContentRect(geometry_rect);
>      if (visible_geometry_rect.IsEmpty())
>        continue;
>
> Index: cc/trees/occlusion.cc
> diff --git a/cc/trees/occlusion.cc b/cc/trees/occlusion.cc
> index d9da9382bcafc6788128ba25488412599dc11356..
> 2daae7a24d814be66957379ffa9630c1f3da9b93 100644
> --- a/cc/trees/occlusion.cc
> +++ b/cc/trees/occlusion.cc
> @@ -20,6 +20,15 @@ Occlusion::Occlusion(const gfx::Transform&
> draw_transform,
>        occlusion_from_inside_target_(occlusion_from_inside_target) {
>  }
>
> +Occlusion Occlusion::GetOcclusionWithScaledDrawTransform(SkMScalar x,
> +                                                         SkMScalar y) {
> +  gfx::Transform scaled_transform = draw_transform_;
> +  scaled_transform.Scale(x, y);
> +  return Occlusion(scaled_transform,
> +                   occlusion_from_outside_target_,
> +                   occlusion_from_inside_target_);
> +}
> +
>  bool Occlusion::IsOccluded(const gfx::Rect& content_rect) const {
>    if (content_rect.IsEmpty())
>      return true;
> Index: cc/trees/occlusion.h
> diff --git a/cc/trees/occlusion.h b/cc/trees/occlusion.h
> index 60493cc2f7d5ff737469cd84adfa8a8daf5a3a4f..
> 7802d81ae5117bb518b9c59e9daa62c680ef3794 100644
> --- a/cc/trees/occlusion.h
> +++ b/cc/trees/occlusion.h
> @@ -19,6 +19,7 @@ class CC_EXPORT Occlusion {
>    Occlusion(const gfx::Transform& draw_transform,
>              const SimpleEnclosedRegion& occlusion_from_outside_target,
>              const SimpleEnclosedRegion& occlusion_from_inside_target);
> +  Occlusion GetOcclusionWithScaledDrawTransform(SkMScalar x, SkMScalar
> y);
>
>    bool IsOccluded(const gfx::Rect& content_rect) const;
>    gfx::Rect GetUnoccludedContentRect(const gfx::Rect& content_rect) const;
>
>
>

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

[vmpstr, 2014-10-03 18:53:17]

On 2014/10/03 18:52:16, danakj wrote:
> And how about an override with only 1 scale that calls the other so less
> repeating in PLI?

Are we cool with overrides? I was under the impression that mumble mumble style
guide. Or are you suggesting a different name?

[vmpstr, 2014-10-03 21:28:25]

Occlusion was missing some tests, so I've added them. Let me know if you really
do want an overload of a function with one parameter (I think it's fine, but I'm
not sure the style guide agrees?)

[danakj, 2014-10-05 03:06:58]

They're allowed but consider another name is what it says. You could adjust
the name if you want. Or just leave it as is. I just thot of our like
Vextor2d scale methods.
On Oct 3, 2014 5:28 PM, <vmpstr@chromium.org> wrote:

> Occlusion was missing some tests, so I've added them. Let me know if you
> really
> do want an overload of a function with one parameter (I think it's fine,
> but I'm
> not sure the style guide agrees?)
>
> https://codereview.chromium.org/607023003/
>

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

[danakj, 2014-10-05 17:27:39]

Tests look cool. FWIW the two query funcs on the Tracker have a test each at the
bottom of the tracker unit tests. Those can just move to be tests on Occlusion
in the near future.

https://codereview.chromium.org/607023003/diff/20001/cc/cc_tests.gyp
File cc/cc_tests.gyp (right):

https://codereview.chromium.org/607023003/diff/20001/cc/cc_tests.gyp#newcode125
cc/cc_tests.gyp:125: 'trees/occlusion_unittest.cc',
There should be a GN change too

https://codereview.chromium.org/607023003/diff/20001/cc/trees/occlusion.cc
File cc/trees/occlusion.cc (right):

https://codereview.chromium.org/607023003/diff/20001/cc/trees/occlusion.cc#ne...
cc/trees/occlusion.cc:26: scaled_transform.Scale(x, y);
Rather than doing these matrix multiplies twice you could just pass an alternate
transform in that has already been constructed. This is essentially a scoped use
of the original occlusion with a different transform.

[vmpstr, 2014-10-06 15:44:52]

https://codereview.chromium.org/607023003/diff/20001/cc/trees/occlusion.cc
File cc/trees/occlusion.cc (right):

https://codereview.chromium.org/607023003/diff/20001/cc/trees/occlusion.cc#ne...
cc/trees/occlusion.cc:26: scaled_transform.Scale(x, y);
On 2014/10/05 17:27:38, danakj wrote:
> Rather than doing these matrix multiplies twice you could just pass an
alternate
> transform in that has already been constructed. This is essentially a scoped
use
> of the original occlusion with a different transform. 

I think this is a great idea. I think I'll add something along the lines of
GetOcclusionWith{,Custom,Alternative,Different}DrawTransform. 

I started replacing the occlusion tracker with occlusion passed in layers, and I
found that delegated layer impl basically uses a quad transform (you probably
know this...), and so I do need this functionality anyway.

[vmpstr, 2014-10-06 18:39:10]

PTAL

[danakj, 2014-10-06 19:09:22]

Update the patch description please. LGTM

[vmpstr, 2014-10-06 23:19:07]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-10-06 23:19:59]

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

[commit-bot: I haz the power, 2014-10-07 01:54:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-07 01:54:28]

Exceeded time limit waiting for builds to trigger.

[vmpstr, 2014-10-07 06:33:07]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-10-07 06:33:50]

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

[commit-bot: I haz the power, 2014-10-07 06:34:40]

Committed patchset #4 (id:60001) as ffac766a6fd32bbbc37c1eb8cd3d7fb29a2a74f0

[commit-bot: I haz the power, 2014-10-07 06:35:19]

Patchset 4 (id:??) landed as
https://crrev.com/61c35e8db648a01dc75b7ab04af454e0fec9dfba
Cr-Commit-Position: refs/heads/master@{#298403}