Bug 91417 – [chromium] Unify compositor quadTransform/drawTransform/originTransform

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

[Adrienne Walker]

[chromium] Unify compositor quadTransform/drawTransform/originTransform

[Adrienne Walker]

Created attachment 152595 [details]
FYI

[Adrienne Walker]

Two failing layout tests (occlusion-related) and a boatload of failing unit tests (oops), but this is where I'm headed.

[Dana Jansens]

Comment on attachment 152595 [details]
FYI

Aside while we're here.. renderMatrix, renderTransform, drawTransform.. could we improve the consistency?

[Adrienne Walker]

(In reply to comment #3)
> (From update of attachment 152595 [details])
> Aside while we're here.. renderMatrix, renderTransform, drawTransform.. could we improve the consistency?

Will do.  LRC is pretty inconsistent in general.  The next step is to make the shared geometry quad 0, 0, 1, 1 which will remove all the half-width code from textured quad drawing and maybe to make some better lower-level drawTexturedQuad functions that don't take a million parameters.

[Adrienne Walker]

Created attachment 153387 [details]
Patch

[Adrienne Walker]

Passes all tests, but on the other hand I also had to modify a lot of tests.  My brain has been melted by transforms, so please call me on anything that looks the least bit dodgy.  :)

[Dana Jansens]

Comment on attachment 153387 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:292
> +        FloatRect updateContentRect = layer->updateRect();
> +        float widthScale = layer->contentBounds().width() / static_cast<float>(layer->bounds().width());
> +        float heightScale = layer->contentBounds().height() / static_cast<float>(layer->bounds().height());
> +        updateContentRect.scale(widthScale, heightScale);

We should really stop scaling updateRect down to layer space in TiledLayerChromium.cpp, and just make it a content-space rect (separately from this).

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:150
> +    return CCSharedQuadState::create(id, drawTransform(), m_visibleContentRect, m_scissorRect, m_drawOpacity, m_opaque);

nit: m_drawTransform

> Source/WebCore/platform/graphics/chromium/cc/CCLayerSorter.cpp:157
> +    FloatQuad layerQuad(FloatRect(FloatPoint(), FloatSize(width, height)));

nit: FloatRect(0, 0, width, height) is less ()s

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:446
> -    //        Interpreting the math left-to-right, this transforms from the root layer space to the local layer's origin.
> +    //        Interpreting the math left-to-right, this transforms from the root layer space to the local layer's origin in layer space.

While we're being precise, this transforms "from the root render surface's content space to..."

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:562
> +        layerDrawTransform.scale(contentsScale);

If this is going from content space to target content space now, this shouldn't be scaled at all?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:565
> +        rectInTargetSpace = IntRect(0, 0, contentsScale * bounds.width(), contentsScale * bounds.height());

hm.. why doesn't this use contentBounds()?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:605
> -        layer->setDrawTransform(combinedTransform);
> +        layer->setDrawTransform(drawTransform);

:D

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:642
> +    layerScreenSpaceTransform.scale(1 / contentsScale);

I just don't get this line.. The drawTransform puts us in the space of the target contents. nextHierarchyMatrix expects an input in the logical space of the owning layer of the target surface?

Should we be scaling by bounds/contentBounds here?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-762
> -        // Adjust the origin of the transform to be the center of the render surface.
> -        WebTransformationMatrix drawTransform = renderSurface->originTransform();
> -        drawTransform.translate3d(surfaceCenter.x() + centerOffsetDueToClipping.width(), surfaceCenter.y() + centerOffsetDueToClipping.height(), 0);
> -        renderSurface->setDrawTransform(drawTransform);
> -

<33!

[Dana Jansens]

Comment on attachment 153387 [details]
Patch

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

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:3710
> -    expectedReplicaDrawTransform.setM41(13.5);
> -    expectedReplicaDrawTransform.setM42(-1.5);
> +    expectedReplicaDrawTransform.setM41(6);
> +    expectedReplicaDrawTransform.setM42(6);

this confuses me, can you justify this? i was expecting deviceScaleFactor * (2,-2) since there is a y-flip and it's at position 2,2. or maybe deviceScaleFactor * (0,0) since its transform is the identity.

[Adrienne Walker]

Comment on attachment 153387 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:292
>> +        updateContentRect.scale(widthScale, heightScale);
> 
> We should really stop scaling updateRect down to layer space in TiledLayerChromium.cpp, and just make it a content-space rect (separately from this).

Definitely.  The changes in bug 91807 make this even more apparent that update rect should probably be a content-space rect, but I wanted to get both of these changes done first (and separately), before considering that.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:150
>> +    return CCSharedQuadState::create(id, drawTransform(), m_visibleContentRect, m_scissorRect, m_drawOpacity, m_opaque);
> 
> nit: m_drawTransform

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerSorter.cpp:157
>> +    FloatQuad layerQuad(FloatRect(FloatPoint(), FloatSize(width, height)));
> 
> nit: FloatRect(0, 0, width, height) is less ()s

Ok, sure.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:446
>> +    //        Interpreting the math left-to-right, this transforms from the root layer space to the local layer's origin in layer space.
> 
> While we're being precise, this transforms "from the root render surface's content space to..."

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:562
>> +        layerDrawTransform.scale(contentsScale);
> 
> If this is going from content space to target content space now, this shouldn't be scaled at all?

This is the same as the previous code without the half-offset.  Draw transforms go from content space of a layer to content space of its target surface.  This scale here implies that the surface itself has a scaled content space.  These content spaces are not the same.  If it helps, this scale here is equivalent to the scale(m_deviceScale) in CCLayerTreeHost::updateLayers when passing in the parent draw transform for things going into the root render surface.

As I think about it, I think this exposes a problem with contentsScale. If only TiledLayerChromium gets contentsScale set on it, then a render surface created by some other layer [e.g. ImageLayerChromium] will not be scaled in the same way that a TiledLayerChromium-owned surface was.  Really, layer->contentsScale() is just here as a hint about what device scale is, so maybe that should just be stored more centrally.  Even better than that, we should look at what the final draw transform of the layer will be in device pixels and estimate the best scale, in case the owning layer has some scale applied to it and we're unnecessarily drawing the surface at a larger scale than it needs to be.  I would like to punt on all of that in this patch.

vollick, do you have any thoughts about any of this?

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:565
>> +        rectInTargetSpace = IntRect(0, 0, contentsScale * bounds.width(), contentsScale * bounds.height());
> 
> hm.. why doesn't this use contentBounds()?

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:642
>> +    layerScreenSpaceTransform.scale(1 / contentsScale);
> 
> I just don't get this line.. The drawTransform puts us in the space of the target contents. nextHierarchyMatrix expects an input in the logical space of the owning layer of the target surface?
> 
> Should we be scaling by bounds/contentBounds here?

Thanks for calling me out on this.  I think it's necessary (given CCLayerTreeHostCommonTest.verifyRenderSurfaceTransformsInHighDPI), but maybe only in some cases.  I'll add more tests and investigate why.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-762
>> -
> 
> <33!

Yeah, centerOffsetDueToClipping has always been a giant wart.

>> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:3710
>> +    expectedReplicaDrawTransform.setM42(6);
> 
> this confuses me, can you justify this? i was expecting deviceScaleFactor * (2,-2) since there is a y-flip and it's at position 2,2. or maybe deviceScaleFactor * (0,0) since its transform is the identity.

replicaDrawTransform = surface->drawTransform() * surfaceOriginToReplicaOriginTransform = (child->position() + replica->position()) * deviceScaleFactor = ( (2, 2) + (2, 2) ) * 1.5 = (6, 6)

I took the values from the expectedReplicaOriginTransform transform below, since the draw transform is now equivalent to it.

[Shawn Singh]

Comment on attachment 153387 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:419
> +    //        S[content2layer] is the ratio of a layer's contentBounds() to its bounds().

Shouldn't this comment be located 4 lines above instead?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:644
> +    // The draw transform is in content space. This needs to be converted back to layer space.

Transforms convert between spaces.  I feel its too ambiguous to say that a transform is "in" a space.   How about we say something like "The draw transform receives objects in content space. For now we want the screen space transform to receive objects in layer space"

[Shawn Singh]

unofficially LGTM =)   This is indeed work done by Efficient Nifty Ninja Enne (ENNE).   As far as I can see the math looks great.

I'm wondering, though.  Is there some push for us to eventually eliminate any usage of "layer space" or "abstract layout space"  (same thing) ?    I think it could eventually work; any contentsScale, deviceScale, pageScale, or other scaling could be done on GraphicsLayerChromium as a translation step to our compositor types.  What do you guys think?  That way we can benefit from (1) one less space to deal with, and (2) the nice refreshing feeling of units being meaningfully pixels again.

Is there any downside to going in that direction?

[Adrienne Walker]

Comment on attachment 153387 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:419
>> +    //        S[content2layer] is the ratio of a layer's contentBounds() to its bounds().
> 
> Shouldn't this comment be located 4 lines above instead?

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:644
>> +    // The draw transform is in content space. This needs to be converted back to layer space.
> 
> Transforms convert between spaces.  I feel its too ambiguous to say that a transform is "in" a space.   How about we say something like "The draw transform receives objects in content space. For now we want the screen space transform to receive objects in layer space"

This is going away in another patch.  I'll change it here, though.

[Adrienne Walker]

Created attachment 153648 [details]
Patch

[Adrienne Walker]

(In reply to comment #9)
> (From update of attachment 153387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153387&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:642
> >> +    layerScreenSpaceTransform.scale(1 / contentsScale);
> > 
> > I just don't get this line.. The drawTransform puts us in the space of the target contents. nextHierarchyMatrix expects an input in the logical space of the owning layer of the target surface?
> > 
> > Should we be scaling by bounds/contentBounds here?
> 
> Thanks for calling me out on this.  I think it's necessary (given CCLayerTreeHostCommonTest.verifyRenderSurfaceTransformsInHighDPI), but maybe only in some cases.  I'll add more tests and investigate why.

Good eye; this was totally bogus.  This extra scale was correct, but only in the case that the layer created a render surface.  (This is because of an extra scale in nextHierarchyMatrix when creating a surface.)  I moved the screen space calculations earlier in the file before the surface vs. non-surface case occurs, and it cleaned things up immensely.

[Adrienne Walker]

Created attachment 153682 [details]
Fix sublayerMatrix/nextHierarchyMatrix errors

[Adrienne Walker]

Comment on attachment 153682 [details]
Fix sublayerMatrix/nextHierarchyMatrix errors

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

Sorry for all the noise, I think this is ready for a final look.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:582
> +        // The sublayer matrix operates on centered layer rects, but the draw
> +        // transform operates on origin space content rects.
> +        sublayerMatrix = layer->drawTransform();
> +        sublayerMatrix.scale(contentsScale);
> +        sublayerMatrix.translate(0.5 * bounds.width(), 0.5 * bounds.height());

The contentsScale line is a diff from the previous patch.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:602
> -        // Update the aggregate hierarchy matrix to include the transform of the newly created RenderSurface.
> -        nextHierarchyMatrix.multiply(surfaceOriginTransform);
> +        // Update the aggregate hierarchy matrix to include the transform of the
> +        // newly created RenderSurface. Both the origin transform and the
> +        // nextHierarchyMatrix include the device scale, so remove this double
> +        // scale so that nextHierarchyMatrix * drawTransform() is a transform to
> +        // root content space.
> +        nextHierarchyMatrix.multiply(renderSurface->originTransform());
> +        nextHierarchyMatrix.scale(1 / contentsScale);

The contentsScale line is a diff from the previous patch.

[Dana Jansens]

Comment on attachment 153682 [details]
Fix sublayerMatrix/nextHierarchyMatrix errors

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

Last points below, looks good elsewise.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:532
> +    if (!layer->contentBounds().isEmpty() && !layer->bounds().isEmpty()) {

I feel like this if is actually wrong, but it probably won't matter. If something is 0-width bounds, it should still have its height scaled. I know we had some regression (on acko.net) from a change like (using isEmpty) this but it was with positions not sizes, since children are relative to positions. Net/net you can probably ignore this comment.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:576
> +        rectInTargetSpace = IntRect(IntPoint(), layer->contentBounds());

This is wrong, my bad on the last comment. :( This should take bounds and transform it with the drawTransform to the target.

[Adrienne Walker]

Created attachment 154163 [details]
Rebased, fixed owner drawableContentRect

[WebKit Review Bot]

Comment on attachment 154163 [details]
Rebased, fixed owner drawableContentRect

Attachment 154163 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13341022

New failing tests:
fast/loader/unload-form-post-about-blank.html

[WebKit Review Bot]

Created attachment 154168 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Adrienne Walker]

(In reply to comment #17)
> (From update of attachment 153682 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153682&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:576
> > +        rectInTargetSpace = IntRect(IntPoint(), layer->contentBounds());
> 
> This is wrong, my bad on the last comment. :( This should take bounds and transform it with the drawTransform to the target.

Thanks for pointing that out.  It's now fixed.  rectInTargetSpace is now calculated via drawTransform and contentBounds in the same place, regardless of whether a layer creates a surface.

[Dana Jansens]

Comment on attachment 154163 [details]
Rebased, fixed owner drawableContentRect

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

Wow is this so simple and making-sense compared to the old code. Great job. LGTM with nit.

Is that bot failure from this CL? Seems unlikely.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:665
> +    rectInTargetSpace = enclosingIntRect(CCMathUtil::mapClippedRect(layer->drawTransform(), contentRect));

nit: might as well declare this variable here instead of way up above, since this is the only place it is set now

[Dana Jansens]

@kbr or @senorblanco for review?

[Kenneth Russell]

Comment on attachment 154163 [details]
Rebased, fixed owner drawableContentRect

rs=me based on danakj's review and assuming the layout test failures aren't caused by this patch. Please double-check that before committing.

[Adrienne Walker]

Created attachment 154211 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 154211 [details]
Patch for landing

Clearing flags on attachment: 154211

Committed r123628: <http://trac.webkit.org/changeset/123628>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 92270