Bug 91823 - [chromium] Clean up LayerRendererChromium::drawTexturedQuad
Summary: [chromium] Clean up LayerRendererChromium::drawTexturedQuad
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on: 91815
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-19 23:30 PDT by Adrienne Walker
Modified: 2012-07-27 15:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (18.27 KB, patch)
2012-07-19 23:35 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Rebased (17.65 KB, patch)
2012-07-25 18:56 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch for landing (18.00 KB, patch)
2012-07-27 12:42 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-07-19 23:30:33 PDT
[chromium] Clean up LayerRendererChromium::drawTexturedQuad
Comment 1 Adrienne Walker 2012-07-19 23:35:18 PDT
Created attachment 153427 [details]
Patch
Comment 2 Adrienne Walker 2012-07-25 18:56:57 PDT
Created attachment 154515 [details]
Rebased
Comment 3 Adrienne Walker 2012-07-26 22:26:59 PDT
Anybody feel like reviewing this? Officially? Unofficially? *crickets*
Comment 4 Dana Jansens 2012-07-27 10:13:40 PDT
Comment on attachment 154515 [details]
Rebased

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

A few little comments, looks good overall!

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:749
> +    if (useAA)
> +        setShaderFloatQuad(surfaceQuad, shaderQuadLocation);

if useAA is false, then shaderQuadLocation should be -1? Can we do the if inside setShaderFloatQuad, the same as we do with setShaderOpacity?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:944
> +    setShaderFloatQuad(localQuad, uniforms.pointLocation);

The drawTexturedQuad would handle pointLocation == -1 but setShaderFloatQuad doesn't, more reason to have setShaderFloatQuad do that check/early-out?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:947
> +    FloatRect centeredRect(FloatPoint(-0.5 * tileRect.width(), -0.5 * tileRect.height()), tileRect.size());
> +    drawQuadGeometry(frame, quad->quadTransform(), centeredRect, uniforms.matrixLocation);

FIXME to make quadTransform() consistent with other quad types?
Comment 5 Adrienne Walker 2012-07-27 10:42:58 PDT
Comment on attachment 154515 [details]
Rebased

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:749
>> +        setShaderFloatQuad(surfaceQuad, shaderQuadLocation);
> 
> if useAA is false, then shaderQuadLocation should be -1? Can we do the if inside setShaderFloatQuad, the same as we do with setShaderOpacity?

Sure.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:944
>> +    setShaderFloatQuad(localQuad, uniforms.pointLocation);
> 
> The drawTexturedQuad would handle pointLocation == -1 but setShaderFloatQuad doesn't, more reason to have setShaderFloatQuad do that check/early-out?

It's always used for the tile shader, for what it's worth.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:947
>> +    drawQuadGeometry(frame, quad->quadTransform(), centeredRect, uniforms.matrixLocation);
> 
> FIXME to make quadTransform() consistent with other quad types?

This is only because of the way the shader works.  It uses these bounds as a way to figure out how big the texture is supposed to be before applying AA to it.  I don't know if there's really anything to fix? I could add a comment to make it more clear that this is shader is special and behaves unexpectedly with respect to its geometry/transfom inputs.
Comment 6 Dana Jansens 2012-07-27 11:37:51 PDT
Comment on attachment 154515 [details]
Rebased

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

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:947
>>> +    drawQuadGeometry(frame, quad->quadTransform(), centeredRect, uniforms.matrixLocation);
>> 
>> FIXME to make quadTransform() consistent with other quad types?
> 
> This is only because of the way the shader works.  It uses these bounds as a way to figure out how big the texture is supposed to be before applying AA to it.  I don't know if there's really anything to fix? I could add a comment to make it more clear that this is shader is special and behaves unexpectedly with respect to its geometry/transfom inputs.

Oh interesting.. thanks for the explanation. A comment might be nice for when I forget this again in a couple months :)
Comment 7 Adrienne Walker 2012-07-27 12:42:15 PDT
Created attachment 155024 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-07-27 15:31:06 PDT
Comment on attachment 155024 [details]
Patch for landing

Clearing flags on attachment: 155024

Committed r123926: <http://trac.webkit.org/changeset/123926>
Comment 9 WebKit Review Bot 2012-07-27 15:31:10 PDT
All reviewed patches have been landed.  Closing bug.