Add high DPI unit test for verifying visible and drawable content rects in compositor.

Add high DPI unit test for verifying visible and drawable content rects in compositor.

A decent array of unit tests was already added, but this one was missing; this patch finally adds that unit test.

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157863

[shawnsingh, 2012-09-20 00:38:01]

There are two comments marked "TODO" because I don't understand why those values
are considered correct.

I would have expected one of two possible behaviors:

behavior 1:
 - drawableContentRects are in target space (which would be the pixel dimensions
of the target surface), so their bounds actually double for a deviceScaleFactor
of 2

behavior 2:
 - drawableContentRects are in target space (which would be the LAYER SPACE
dimensions of the target surface), so their bounds actually stay the same as
non-deviceScale examples.

Instead, we are seeing the following behavior:
 - drawableContentRects are halved in bounds because of the
deviceScaleFactor==2.

Can someone please explain?

[danakj, 2012-09-20 15:09:41]

Surfaces have no "layer space" they are only physical pixels, and
anything in that space should be scaled appropriately. So, I'd expect
them to be doubled.

On Wed, Sep 19, 2012 at 8:38 PM,  <shawnsingh@chromium.org> wrote:
> Reviewers: enne,
>
> Message:
> There are two comments marked "TODO" because I don't understand why those
> values
> are considered correct.
>
> I would have expected one of two possible behaviors:
>
> behavior 1:
>  - drawableContentRects are in target space (which would be the pixel
> dimensions
> of the target surface), so their bounds actually double for a
> deviceScaleFactor
> of 2
>
> behavior 2:
>  - drawableContentRects are in target space (which would be the LAYER SPACE
> dimensions of the target surface), so their bounds actually stay the same as
> non-deviceScale examples.
>
> Instead, we are seeing the following behavior:
>  - drawableContentRects are halved in bounds because of the
> deviceScaleFactor==2.
>
> Can someone please explain?
>
> Description:
> Add high DPI unit test for verifying visible and drawable content rects in
> compositor.
>
> A decent array of unit tests was already added, but this one was missing;
> this
> patch finally adds that unit test.
>
> BUG=
>
>
> Please review this at https://codereview.chromium.org/10964002/
>
> SVN Base: http://git.chromium.org/chromium/src.git@master
>
> Affected files:
>   M cc/CCLayerTreeHostCommonTest.cpp
>
>
> Index: cc/CCLayerTreeHostCommonTest.cpp
> diff --git a/cc/CCLayerTreeHostCommonTest.cpp
> b/cc/CCLayerTreeHostCommonTest.cpp
> index
>
b0699d509696c9cac2177c5d8026041c9dee8ca1..58f6740ec40e0330f71667579710424f8c3dfd66
> 100644
> --- a/cc/CCLayerTreeHostCommonTest.cpp
> +++ b/cc/CCLayerTreeHostCommonTest.cpp
> @@ -123,6 +123,20 @@ public:
>      virtual bool drawsContent() const OVERRIDE { return true; }
>  };
>
> +class MockContentLayerChromiumClient : public ContentLayerChromiumClient {
> +public:
> +    MockContentLayerChromiumClient() { }
> +    virtual ~MockContentLayerChromiumClient() { }
> +    virtual void paintContents(SkCanvas*, const IntRect& clip, FloatRect&
> opaque) OVERRIDE { }
> +};
> +
> +PassRefPtr<ContentLayerChromium>
> createDrawableContentLayerChromium(ContentLayerChromiumClient* delegate)
> +{
> +    RefPtr<ContentLayerChromium> toReturn =
> ContentLayerChromium::create(delegate);
> +    toReturn->setIsDrawable(true);
> +    return toReturn.release();
> +}
> +
>  TEST(CCLayerTreeHostCommonTest, verifyTransformsForNoOpLayer)
>  {
>      // Sanity check: For layers positioned at zero, with zero size,
> @@ -2220,6 +2234,77 @@ TEST(CCLayerTreeHostCommonTest,
> verifyDrawableAndVisibleContentRectsWithTransfor
>      EXPECT_RECT_EQ(unclippedSurfaceContent, child1->drawableContentRect());
>  }
>
> +TEST(CCLayerTreeHostCommonTest,
> verifyDrawableAndVisibleContentRectsInHighDPI)
> +{
> +    MockContentLayerChromiumClient client;
> +
> +    RefPtr<LayerChromium> root = LayerChromium::create();
> +    RefPtr<ContentLayerChromium> renderSurface1 =
> createDrawableContentLayerChromium(&client);
> +    RefPtr<ContentLayerChromium> renderSurface2 =
> createDrawableContentLayerChromium(&client);
> +    RefPtr<ContentLayerChromium> child1 =
> createDrawableContentLayerChromium(&client);
> +    RefPtr<ContentLayerChromium> child2 =
> createDrawableContentLayerChromium(&client);
> +    RefPtr<ContentLayerChromium> child3 =
> createDrawableContentLayerChromium(&client);
> +    root->addChild(renderSurface1);
> +    renderSurface1->addChild(renderSurface2);
> +    renderSurface2->addChild(child1);
> +    renderSurface2->addChild(child2);
> +    renderSurface2->addChild(child3);
> +
> +    WebTransformationMatrix identityMatrix;
> +    setLayerPropertiesForTesting(root.get(), identityMatrix,
> identityMatrix, FloatPoint(0, 0), FloatPoint(0, 0), IntSize(100, 100),
> false);
> +    setLayerPropertiesForTesting(renderSurface1.get(), identityMatrix,
> identityMatrix, FloatPoint(0, 0), FloatPoint(5, 5), IntSize(3, 4), false);
> +    setLayerPropertiesForTesting(renderSurface2.get(), identityMatrix,
> identityMatrix, FloatPoint(0, 0), FloatPoint(5, 5), IntSize(7, 13), false);
> +    setLayerPropertiesForTesting(child1.get(), identityMatrix,
> identityMatrix, FloatPoint(0, 0), FloatPoint(5, 5), IntSize(50, 50), false);
> +    setLayerPropertiesForTesting(child2.get(), identityMatrix,
> identityMatrix, FloatPoint(0, 0), FloatPoint(75, 75), IntSize(50, 50),
> false);
> +    setLayerPropertiesForTesting(child3.get(), identityMatrix,
> identityMatrix, FloatPoint(0, 0), FloatPoint(125, 125), IntSize(50, 50),
> false);
> +
> +    const double deviceScaleFactor = 2;
> +    root->setContentsScale(deviceScaleFactor);
> +    renderSurface1->setContentsScale(deviceScaleFactor);
> +    renderSurface2->setContentsScale(deviceScaleFactor);
> +    child1->setContentsScale(deviceScaleFactor);
> +    child2->setContentsScale(deviceScaleFactor);
> +    child3->setContentsScale(deviceScaleFactor);
> +
> +    root->setMasksToBounds(true);
> +    renderSurface1->setForceRenderSurface(true);
> +    renderSurface2->setForceRenderSurface(true);
> +    executeCalculateDrawTransformsAndVisibility(root.get());
> +
> +    ASSERT_TRUE(renderSurface1->renderSurface());
> +    ASSERT_TRUE(renderSurface2->renderSurface());
> +
> +    EXPECT_RECT_EQ(IntRect(0, 0, 100, 100),
> root->renderSurface()->drawableContentRect());
> +    EXPECT_RECT_EQ(IntRect(0, 0, 100, 100), root->drawableContentRect());
> +
> +    // Layers that do not draw content should have empty
> visibleContentRects.
> +    EXPECT_RECT_EQ(IntRect(0, 0, 0, 0), root->visibleContentRect());
> +
> +    // A clipped surface grows its drawableContentRect to include all
> drawable regions of the subtree,
> +    // but also gets clamped by the ancestor's clip.
> +    // TODO(shawnsingh) why is this correct?  or is it actually incorrect?
> I thought it should be 95, 95 instead of 47, 47.
> +    EXPECT_RECT_EQ(IntRect(5, 5, 47, 47),
> renderSurface1->renderSurface()->drawableContentRect());
> +
> +    // renderSurface2 lives in the "unclipped universe" of renderSurface1,
> and is only
> +    // implicitly clipped by renderSurface1's contentRect. So,
> renderSurface2 grows to
> +    // enclose all drawable content of its subtree.
> +    // TODO(shawnsingh) why is this correct?  or is it actually incorrect?
> I thought it should be 170, 170 instead of 88, 88.
> +    EXPECT_RECT_EQ(IntRect(5, 5, 88, 88),
> renderSurface2->renderSurface()->drawableContentRect());
> +
> +    // All layer visibleContentRects are expressed in content space of each
> layer, so they
> +    // are scaled by the deviceScaleFactor.
> +    EXPECT_RECT_EQ(IntRect(0, 0, 6, 8),
> renderSurface1->visibleContentRect());
> +    EXPECT_RECT_EQ(IntRect(0, 0, 14, 26),
> renderSurface2->visibleContentRect());
> +    EXPECT_RECT_EQ(IntRect(0, 0, 100, 100), child1->visibleContentRect());
> +    EXPECT_RECT_EQ(IntRect(0, 0, 100, 100), child2->visibleContentRect());
> +    EXPECT_RECT_EQ(IntRect(0, 0, 100, 100), child3->visibleContentRect());
> +
> +    // drawableContentRects are expressed in target surface space, which is
> not scaled by deviceScaleFactor.
> +    EXPECT_RECT_EQ(IntRect(5, 5, 50, 50), child1->drawableContentRect());
> +    EXPECT_RECT_EQ(IntRect(75, 75, 50, 50), child2->drawableContentRect());
> +    EXPECT_RECT_EQ(IntRect(125, 125, 50, 50),
> child3->drawableContentRect());
> +}
> +
>  TEST(CCLayerTreeHostCommonTest, verifyBackFaceCullingWithoutPreserves3d)
>  {
>      // Verify the behavior of back-face culling when there are no
> preserve-3d layers. Note
> @@ -3340,20 +3425,6 @@ TEST(CCLayerTreeHostCommonTest,
> verifyHitTestingForMultipleLayerLists)
>      EXPECT_EQ(4, resultLayer->id());
>  }
>
> -class MockContentLayerChromiumClient : public ContentLayerChromiumClient {
> -public:
> -    MockContentLayerChromiumClient() { }
> -    virtual ~MockContentLayerChromiumClient() { }
> -    virtual void paintContents(SkCanvas*, const IntRect& clip, FloatRect&
> opaque) OVERRIDE { }
> -};
> -
> -PassRefPtr<ContentLayerChromium>
> createDrawableContentLayerChromium(ContentLayerChromiumClient* delegate)
> -{
> -    RefPtr<ContentLayerChromium> toReturn =
> ContentLayerChromium::create(delegate);
> -    toReturn->setIsDrawable(true);
> -    return toReturn.release();
> -}
> -
>  TEST(CCLayerTreeHostCommonTest, verifyLayerTransformsInHighDPI)
>  {
>      // Verify draw and screen space transforms of layers not in a surface.
>
>

[enne (OOO), 2012-09-20 16:31:10]

Dana is totally right.  Surfaces only have "content" space aka device pixels.

In this test, you are setting contents scale to be 2 for each layer, but you
haven't changed the device scale that you're passing to calcDraw, so surfaces
get created with device scale = 1.

[danakj, 2012-09-20 16:36:02]

Oh good catch. To be thorough and deeply understanding these spaces..

Layer Content Space <-> Layer Space
- The layer's content space is determined by its contentsScale.
- Moving between these spaces is done by using the ratio of bounds to
contentBounds. You *must* do the width and height independently.

Layer Space <-> Target space
- Moving from the layer's space to its target space applies the parentTransform
(which can include deviceScaleFactor), and the layer's transform. This is
completely independent from the layer's contentsScale or contentBounds.

[shawnsingh, 2012-09-20 17:27:53]

Thanks for catching that mistake, Enne.  Latest patch should be good to go,
then.

[enne (OOO), 2012-09-20 19:48:30]

LGTM

[commit-bot: I haz the power, 2012-09-20 20:18:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/10964002/7002

[commit-bot: I haz the power, 2012-09-20 22:59:56]

Change committed as 157863