Add missing transform into DelegatedRendererLayerImpl::AppendRenderPassQuads.

Add missing transform into DelegatedRendererLayerImpl::AppendRenderPassQuads.

We can miss applying a transform if we have a transformed, partially
occluded DelegatedRendererLayerImpl with a contributing RenderPass. This
causes the wrong transform to be sent into the occlusion tracker, so
occlusion gets applied incorrectly, which hides desired content.

BUG=379072, 390873
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288403

[hartmanng, 2014-08-07 20:35:35]

Please take a look

[danakj, 2014-08-07 20:38:41]

On Aug 7, 2014 4:35 PM, <hartmanng@chromium.org> wrote:
>
> Reviewers: enne, danakj,
>
> Message:
> Please take a look
>
> Description:
> Add missing transform into
DelegatedRendererLayerImpl::AppendRenderPassQuads.
>
> We can miss applying a transform if we have a transformed, partially
> occluded DelegatedRendererLayerImpl with a contributing RenderPass. This
> causes one of the clip rects to be in the incorrect space while performing
> an intersection, which clips off desired content.

It causes the transform passed to the occlusion tracker to not correctly
map the query rect into the space of the DRL's render target, which causes
occlusion to be applied wrong.

(The clips turned out to be fine)

>
> BUG=379072,390873
>
> Please review this at https://codereview.chromium.org/447193005/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+35, -2 lines):
>   M cc/layers/delegated_renderer_layer_impl.cc
>   M cc/layers/delegated_renderer_layer_impl_unittest.cc
>
>
> Index: cc/layers/delegated_renderer_layer_impl.cc
> diff --git a/cc/layers/delegated_renderer_layer_impl.cc
b/cc/layers/delegated_renderer_layer_impl.cc
> index
0fea04e852c895454ce109530640b01069295254..596284f291cc9135094272290be71bfb2e1dda9b
100644
> --- a/cc/layers/delegated_renderer_layer_impl.cc
> +++ b/cc/layers/delegated_renderer_layer_impl.cc
> @@ -436,9 +436,15 @@ void
DelegatedRendererLayerImpl::AppendRenderPassQuads(
>      }
>      DCHECK(output_shared_quad_state);
>
> +    gfx::Transform quad_content_to_delegated_target_space =
> +        output_shared_quad_state->content_to_target_transform;
> +    quad_content_to_delegated_target_space.ConcatTransform(
> +        render_pass->transform_to_root_target);
> +
 quad_content_to_delegated_target_space.ConcatTransform(draw_transform());
> +
>      gfx::Rect quad_visible_rect =
occlusion_tracker.UnoccludedContentRect(
> -        delegated_quad->visible_rect,
> -        output_shared_quad_state->content_to_target_transform);
> +        delegated_quad->visible_rect,
quad_content_to_delegated_target_space);
> +
>      if (quad_visible_rect.IsEmpty())
>        continue;
>
> Index: cc/layers/delegated_renderer_layer_impl_unittest.cc
> diff --git a/cc/layers/delegated_renderer_layer_impl_unittest.cc
b/cc/layers/delegated_renderer_layer_impl_unittest.cc
> index
33082cc6d2b70345e96c1f70922ea01caa6cd28a..193b7e607bfe933f696b9ac4932c1c0aa10433d3
100644
> --- a/cc/layers/delegated_renderer_layer_impl_unittest.cc
> +++ b/cc/layers/delegated_renderer_layer_impl_unittest.cc
> @@ -1507,6 +1507,33 @@ TEST_F(DelegatedRendererLayerImplTest, Occlusion) {
>                  impl.quad_list()[0]->visible_rect.ToString());
>      }
>    }
> +  {
> +    gfx::Rect occluded(0, 0, 500, 1000);
> +    // Move the occlusion to where it is in the contributing surface.
> +    occluded -= quad_rect.OffsetFromOrigin() + gfx::Vector2d(11, 0);
> +
> +    SCOPED_TRACE("Contributing render pass with transformed root");
> +
> +    delegated_renderer_layer_impl->SetTransform(transform);
> +    impl.CalcDrawProps(viewport_size);
> +
> +    impl.AppendQuadsForPassWithOcclusion(
> +        delegated_renderer_layer_impl, pass2_id, occluded);
> +    size_t partially_occluded_count = 0;
> +    LayerTestCommon::VerifyQuadsCoverRectWithOcclusion(
> +        impl.quad_list(),
> +        gfx::Rect(quad_rect.size()),
> +        occluded,
> +        &partially_occluded_count);
> +    // The layer outputs one quad, which is partially occluded.
> +    EXPECT_EQ(1u, impl.quad_list().size());
> +    EXPECT_EQ(1u, partially_occluded_count);
> +    // The quad in the contributing surface is at (222,300) in the
transformed
> +    // root. The occlusion extends to 500 in the x-axis, pushing the
left of the
> +    // visible part of the quad to 500 - 222 = 300 - 22 inside the quad.
> +    EXPECT_EQ(gfx::Rect(300 - 22, 0, 100 + 22, 500).ToString(),
> +              impl.quad_list()[0]->visible_rect.ToString());
> +  }
>  }
>
>  TEST_F(DelegatedRendererLayerImplTest, PushPropertiesTo) {
>
>

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

[hartmanng, 2014-08-07 20:46:51]

On 2014/08/07 20:38:41, danakj wrote:
> On Aug 7, 2014 4:35 PM, <mailto:hartmanng@chromium.org> wrote:
> >
> > Reviewers: enne, danakj,
> >
> > Message:
> > Please take a look
> >
> > Description:
> > Add missing transform into
> DelegatedRendererLayerImpl::AppendRenderPassQuads.
> >
> > We can miss applying a transform if we have a transformed, partially
> > occluded DelegatedRendererLayerImpl with a contributing RenderPass. This
> > causes one of the clip rects to be in the incorrect space while performing
> > an intersection, which clips off desired content.
> 
> It causes the transform passed to the occlusion tracker to not correctly
> map the query rect into the space of the DRL's render target, which causes
> occlusion to be applied wrong.
> 
> (The clips turned out to be fine)


Erm, yeah, that. Thanks :)

[danakj, 2014-08-08 13:45:05]

LGTM thanks for the test

https://codereview.chromium.org/447193005/diff/20001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/447193005/diff/20001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer_impl.cc:441:
quad_content_to_delegated_target_space.ConcatTransform(
to avoid these matrix mults in most cases, can you only do this when
|is_root_delegated_render_pass| is false? (you'll have to move that var out one
nested level to re-use it.)

[hartmanng, 2014-08-08 14:00:02]

Thanks!

https://codereview.chromium.org/447193005/diff/20001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/447193005/diff/20001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer_impl.cc:441:
quad_content_to_delegated_target_space.ConcatTransform(
On 2014/08/08 13:45:05, danakj wrote:
> to avoid these matrix mults in most cases, can you only do this when
> |is_root_delegated_render_pass| is false? (you'll have to move that var out
one
> nested level to re-use it.)

Done.

[hartmanng, 2014-08-08 14:00:06]

The CQ bit was checked by hartmanng@chromium.org

[commit-bot: I haz the power, 2014-08-08 14:04:39]

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

[commit-bot: I haz the power, 2014-08-08 15:49:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/44372)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-08 18:52:35]

Change committed as 288403