Bug 91350 – [chromium] Unify compositor quad transforms into content space

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

[Adrienne Walker]

[chromium] Unify compositor quad transforms into content space

[Adrienne Walker]

Created attachment 152466 [details]
Patch

[Adrienne Walker]

shawnsingh, danakj: Could one or both of y'all unofficially review this patch?

[Adrienne Walker]

Comment on attachment 152466 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:160
> +    if (contentBounds().isEmpty())
> +        quadTransformation.scale(1 / contentsScale());
> +    else {
> +        quadTransformation.scaleNonUniform(bounds().width() / static_cast<double>(contentBounds().width()),
> +                                  bounds().height() / static_cast<double>(contentBounds().height()));
> +        quadTransformation.translate(-contentBounds().width() / 2.0, -contentBounds().height() / 2.0);
> +    }

In retrospect, I don't think this is always right, but I'm not sure how to reconcile contentsScale which is a single value and the contentBounds/bounds ratio that handles different integer rounding in width and height.

[Dana Jansens]

Comment on attachment 152466 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:154
> +    if (contentBounds().isEmpty())

Do we even need to return anything other than nullptr in this case?

[Shawn Singh]

Comment on attachment 152466 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:160
>> +    }
> 
> In retrospect, I don't think this is always right, but I'm not sure how to reconcile contentsScale which is a single value and the contentBounds/bounds ratio that handles different integer rounding in width and height.

What's an example of a case where this isn't right?  The few times I've encountered this pattern, contentsScale wasn't good enough because of ImageLayers in particular, which have different horizontal and vertical scaling.   But it seemed to me like this code would handle all the cases I was aware of?

[Adrienne Walker]

Comment on attachment 152466 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:154
>> +    if (contentBounds().isEmpty())
> 
> Do we even need to return anything other than nullptr in this case?

Yeah.  The gutter quads use the root layer (the real root, not the NCCH) to generate a shared quad state.  Maybe we should just create that manually.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:160
>>> +    }
>> 
>> In retrospect, I don't think this is always right, but I'm not sure how to reconcile contentsScale which is a single value and the contentBounds/bounds ratio that handles different integer rounding in width and height.
> 
> What's an example of a case where this isn't right?  The few times I've encountered this pattern, contentsScale wasn't good enough because of ImageLayers in particular, which have different horizontal and vertical scaling.   But it seemed to me like this code would handle all the cases I was aware of?

It may just be an issue in tests, where contentBounds is not always up to date with contentsScale.  I'll take another look and see if there's something I can clean up.

[Adrienne Walker]

Created attachment 152586 [details]
Patch

[Adrienne Walker]

Created attachment 152593 [details]
Handle empty bounds in create quad state

[Dana Jansens]

Comment on attachment 152593 [details]
Handle empty bounds in create quad state

lgtm!

[Adrienne Walker]

kbr, senorblanco: Would one of you mind taking a look at this compositor patch that has an unofficial lgtm from danakj?

It's just a refactoring that shouldn't cause any change in behavior.  All layers other than tiled layers have bounds == contentBounds, so the extra code in createSharedQuadState generates the same quadTransform that it used to for both tiled layers and non-tiled layers.

[Kenneth Russell]

Comment on attachment 152593 [details]
Handle empty bounds in create quad state

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

Mostly a rubber stamp, though I did look through the code. One question. Go ahead and cq+ if this isn't an issue.

> Source/WebKit/chromium/tests/CCQuadCullerTest.cpp:202
> +    ASSERT_EQ(quadList.size(), 12u);

Will this get run in non-debug builds?

[Adrienne Walker]

(In reply to comment #11)
> (From update of attachment 152593 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152593&action=review
> 
> Mostly a rubber stamp, though I did look through the code. One question. Go ahead and cq+ if this isn't an issue.

Thanks.  :)

> > Source/WebKit/chromium/tests/CCQuadCullerTest.cpp:202
> > +    ASSERT_EQ(quadList.size(), 12u);
> 
> Will this get run in non-debug builds?

Yeah.  ASSERT_EQ aborts the test if it doesn't pass.  Since later checks reference quadList[i], if that's wrong, then those will assert.  See http://code.google.com/p/googletest/wiki/Primer#Basic_Assertions

[WebKit Review Bot]

Comment on attachment 152593 [details]
Handle empty bounds in create quad state

Clearing flags on attachment: 152593

Committed r122776: <http://trac.webkit.org/changeset/122776>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.