Bug 81296 – [chromium] Store origin/screen space transforms for surface and replica in the surface

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Store origin/screen space transforms for surface and replica in the surface

[Dana Jansens]

Created attachment 132173 [details]
Patch

[Dana Jansens]

Two changes worth noting to the damage tracker code that I moved into CCLTHCommon:

1. The origin of the layer owning a surface is the same as the origin of the surface. So the screenSpaceTransforms are the same.

The old code was:

    // The layer's screen space transform can be written as:
    //   layerScreenSpaceTransform = surfaceScreenSpaceTransform * layerOriginTransform
    // So, to compute the surface screen space, we can do:
    //   surfaceScreenSpaceTransform = layerScreenSpaceTransform * inverse(layerOriginTransform)

    TransformationMatrix layerOriginTransform = renderSurfaceLayer->drawTransform();
    layerOriginTransform.translate(-0.5 * renderSurfaceLayer->bounds().width(), -0.5 * renderSurfaceLayer->bounds().height());
    TransformationMatrix surfaceScreenSpaceTransform = renderSurfaceLayer->screenSpaceTransform();
    surfaceScreenSpaceTransform.multiply(layerOriginTransform.inverse());

However, the draw transform for the owning layer of a surface is always just a transform from the center of the layer (set in CCLTHCommon.cpp:283). So the layerOriginTransform in the above is the identity matrix.

2. The replicaOriginTransform was wrong when the surface had a non-zero position.

The old code was:

    TransformationMatrix replicaOriginTransform = layer->renderSurface()->originTransform();
    replicaOriginTransform.translate(layer->replicaLayer()->position().x(), layer->replicaLayer()->position().y());
    replicaOriginTransform.multiply(layer->replicaLayer()->transform());
    replicaOriginTransform.translate(-layer->replicaLayer()->position().x(), -layer->replicaLayer()->position().y());

The transform starts with the surface origin at The Origin. It used to subtract the surface's position, which is the surfaceOriginPosition + anchor. However, what we want is to put the anchor at The Origin, so we should just subtract the anchor.


The replica screen space transform works similarly to the replica origin transform (but finishes with the surface's screen space transform instead of the surface's origin transform).

I tested this replica screen space transform out using Shawn's visualization stuff, and the complicated replica layout tests (using rotation etc) and verified the transforms that way as well.

[Dana Jansens]

Note: i want these transforms so i can check occlusion of replicas for culling surfaces.

[Shawn Singh]

Comment on attachment 132173 [details]
Patch


Dana - this is awesome!  I had a few comments we should address before final patch.

Everyone else - its probably worth pointing out that this sudden inflation of transforms being stored on surfaces has been long-since coming and I think its appropriate. Transform cleanups will be coming from us over the next several months, and the end result will be much tighter.  I still think dana's patch here is the right thing to do for now, because it removes a lot of uncomfortable precarious-ness at the expense of storing more per renderSurface.


View in context: https://bugs.webkit.org/attachment.cgi?id=132173&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:453
> +        renderSurface->setScreenSpaceTransform(layer->screenSpaceTransform());

Can you please add a unit test to verify if this is still true if there is a child somewhere in the subtree that ends up positioned in negative position w.r.t. to the renderSurface?  i.e. in surfaceSpace a child's visible rect in surface space positioned at (-10, -20) might change the transform of the surface??  I think it will work, but if it doesn't then we can't compute screen space transform directly from its owning layer.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:456
> +            // Compute the transformation matrix used to draw the replica of the render surface.

Could we please re-word this - "used to draw the surface's replica to the target surface"

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:464
> +            TransformationMatrix replicaOriginTransform = layer->renderSurface()->originTransform();

can you please add an entry in the comments above  (near the bottom, where all the other transforms are spelled out), to spell out the actual sequence of transforms of the replica?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:467
> +            replicaOriginTransform.translate(layer->replicaLayer()->position().x(), layer->replicaLayer()->position().y());
> +            replicaOriginTransform.multiply(layer->replicaLayer()->transform());
> +            replicaOriginTransform.translate(-anchorPoint.x() * bounds.width(), -anchorPoint.y() * bounds.height());

We should combine these in to a temporary named "surfaceOriginToReplicaOriginTransform"  and re-use it in the code below, to avoid redundancy.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:474
> +            replicaScreenSpaceTransform.translate(layer->replicaLayer()->position().x(), layer->replicaLayer()->position().y());
> +            replicaScreenSpaceTransform.multiply(layer->replicaLayer()->transform());
> +            replicaScreenSpaceTransform.translate(-anchorPoint.x() * bounds.width(), -anchorPoint.y() * bounds.height());

redundancy here to be removed based on comment above =)

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:291
> +    TransformationMatrix replicaLayerTransform;

I would prefer to have a separate test  "verifyTransformsForReplica", even though its a bit inflated.  I feel like the concepts here are complicated enough to warrant that inflation.

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:337
> +    RefPtr<LayerChromium> replicaOfRS1 = LayerChromium::create();

But for this test case, I think its OK to keep the complexity here.

We should update the comments for this test to indicate that its also testing replicas.

[Dana Jansens]

Comment on attachment 132173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132173&action=review

Thanks Shawn :)

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:453
>> +        renderSurface->setScreenSpaceTransform(layer->screenSpaceTransform());
> 
> Can you please add a unit test to verify if this is still true if there is a child somewhere in the subtree that ends up positioned in negative position w.r.t. to the renderSurface?  i.e. in surfaceSpace a child's visible rect in surface space positioned at (-10, -20) might change the transform of the surface??  I think it will work, but if it doesn't then we can't compute screen space transform directly from its owning layer.

The verifyTransformsForSingleRenderSurface test actually does have a child of a surface with position -0.5, -0.5, and the surface transforms are unaffected.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:456
>> +            // Compute the transformation matrix used to draw the replica of the render surface.
> 
> Could we please re-word this - "used to draw the surface's replica to the target surface"

done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:464
>> +            TransformationMatrix replicaOriginTransform = layer->renderSurface()->originTransform();
> 
> can you please add an entry in the comments above  (near the bottom, where all the other transforms are spelled out), to spell out the actual sequence of transforms of the replica?

done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:467
>> +            replicaOriginTransform.translate(-anchorPoint.x() * bounds.width(), -anchorPoint.y() * bounds.height());
> 
> We should combine these in to a temporary named "surfaceOriginToReplicaOriginTransform"  and re-use it in the code below, to avoid redundancy.

done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:474
>> +            replicaScreenSpaceTransform.translate(-anchorPoint.x() * bounds.width(), -anchorPoint.y() * bounds.height());
> 
> redundancy here to be removed based on comment above =)

done.

>> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:291
>> +    TransformationMatrix replicaLayerTransform;
> 
> I would prefer to have a separate test  "verifyTransformsForReplica", even though its a bit inflated.  I feel like the concepts here are complicated enough to warrant that inflation.

done.

>> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:337
>> +    RefPtr<LayerChromium> replicaOfRS1 = LayerChromium::create();
> 
> But for this test case, I think its OK to keep the complexity here.
> 
> We should update the comments for this test to indicate that its also testing replicas.

done.

[Dana Jansens]

Created attachment 132361 [details]
Patch

[Shawn Singh]

Comment on attachment 132361 [details]
Patch


LGTM except for the following =)


View in context: https://bugs.webkit.org/attachment.cgi?id=132361&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:-81
> -    void setFilters(const FilterOperations& filters) { m_filters = filters; }
> -    const FilterOperations& filters() const { return m_filters; }
> -    SkBitmap applyFilters(LayerRendererChromium*);

This seems irrelevant - did you have a merge conflict when updating your codebase?  We probably don't mean to remove this, right?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:-84
> -    void setNearestAncestorThatMovesPixels(CCRenderSurface* surface) { m_nearestAncestorThatMovesPixels = surface; }
> -    const CCRenderSurface* nearestAncestorThatMovesPixels() const { return m_nearestAncestorThatMovesPixels; }

And this too

[Dana Jansens]

Comment on attachment 132361 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132361&action=review

Thanks :)

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:-81
>> -    SkBitmap applyFilters(LayerRendererChromium*);
> 
> This seems irrelevant - did you have a merge conflict when updating your codebase?  We probably don't mean to remove this, right?

Yes, it just moved up above and the diff is messy.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:-84
>> -    const CCRenderSurface* nearestAncestorThatMovesPixels() const { return m_nearestAncestorThatMovesPixels; }
> 
> And this too

Ditto.

[Shawn Singh]

ah, I see it now.  no problem then =)

[Adrienne Walker]

Comment on attachment 132361 [details]
Patch

Nice cleanup.  Thanks for all the comments and tests.

[WebKit Review Bot]

Comment on attachment 132361 [details]
Patch

Rejecting attachment 132361 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
cceeded at 310 (offset 2 lines).
Hunk #2 succeeded at 367 (offset 2 lines).
Hunk #3 succeeded at 377 (offset 2 lines).
Hunk #4 succeeded at 391 (offset 2 lines).
Hunk #5 succeeded at 401 (offset 2 lines).
Hunk #6 succeeded at 413 (offset 2 lines).
Hunk #7 succeeded at 429 (offset 2 lines).
Hunk #8 succeeded at 496 (offset 2 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adrienne W..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12035078

[Dana Jansens]

Created attachment 132957 [details]
Patch

[WebKit Review Bot]

Comment on attachment 132957 [details]
Patch

Clearing flags on attachment: 132957

Committed r111499: <http://trac.webkit.org/changeset/111499>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.