Bug 66437 - [chromium] aliased render surface edges
Summary: [chromium] aliased render surface edges
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-17 17:40 PDT by David Reveman
Modified: 2013-03-07 20:55 PST (History)
4 users (show)

See Also:


Attachments
Patch (50.28 KB, patch)
2011-08-18 19:36 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (50.00 KB, patch)
2011-08-24 09:04 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (58.79 KB, patch)
2011-08-25 10:22 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (59.98 KB, patch)
2011-08-30 08:32 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (517.15 KB, patch)
2011-09-02 11:00 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (524.53 KB, patch)
2011-09-02 11:13 PDT, David Reveman
jamesr: review-
Details | Formatted Diff | Diff
Patch (528.50 KB, patch)
2011-09-02 15:19 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (445.90 KB, patch)
2011-09-06 06:16 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (445.99 KB, patch)
2011-09-06 09:33 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (449.16 KB, patch)
2011-09-06 15:36 PDT, David Reveman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2011-08-17 17:40:13 PDT
Anti-aliasing is currently only used for tiled layer edges. Render surface edges should be anti-aliased too.
Comment 1 Adrienne Walker 2011-08-18 10:20:48 PDT
Out of curiosity, are you going to address bug 65483 as a part of this (or before this)?
Comment 2 David Reveman 2011-08-18 11:19:17 PDT
(In reply to comment #1)
> Out of curiosity, are you going to address bug 65483 as a part of this (or before this)?

Just updated 65483. I don't have a good solution for that bug yet.

I'm currently working on a solution to this bug that doesn't rely on 65483.
Comment 3 David Reveman 2011-08-18 19:36:49 PDT
Created attachment 104439 [details]
Patch
Comment 4 James Robinson 2011-08-18 19:51:43 PDT
Comment on attachment 104439 [details]
Patch

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

It seems like we're duplicating some logic between RenderSurface and TiledLayer rendering now, which makes me sad.  Is there any way to share more code between these two?

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:241
> +    typedef FloatPoint3D Edge;
> +
> +    static bool isCCW(const FloatQuad&);
> +    static Edge computeEdge(const FloatPoint&, const FloatPoint&, float);
> +    static FloatPoint intersect(const Edge&, const Edge&);
> +    static void toGLVec3(float*, const Edge&);
> +
> +    struct Quad {
> +        Quad(const FloatQuad &);
> +
> +        void inflateX(float dx) { left.move(0, 0, dx); right.move(0, 0, dx); }
> +        void inflateY(float dy) { top.move(0, 0, dy); bottom.move(0, 0, dy); }
> +        void inflate(float d) { inflateX(d); inflateY(d); }
> +
> +        FloatQuad floatQuad();
> +
> +        Edge left;
> +        Edge top;
> +        Edge right;
> +        Edge bottom;
> +    };
> +
> +    // This method removes Z component from matrix. Useful when matrix
> +    // is only used for transforming X/Y components.
> +    static TransformationMatrix noZMatrix(const TransformationMatrix&);
> +

This is not good, none of this belongs on LayerChromium.  Draw-time concerns shouldn't be on LayerChromium (and we need to move drawTexturedQuad at some point) and I have no idea what any of these have to do with any Layer type.  Are they just math utilities?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:148
> +                shaderSamplerLocation = maskProgramAA->fragmentShader().samplerLocation();
> +                shaderMaskSamplerLocation = maskProgramAA->fragmentShader().maskSamplerLocation();

Are you querying these on every draw? That's going to be ridiculously slow, isn't it?
Comment 5 David Reveman 2011-08-19 10:17:52 PDT
(In reply to comment #4)
> (From update of attachment 104439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104439&action=review
> 
> It seems like we're duplicating some logic between RenderSurface and TiledLayer rendering now, which makes me sad.  Is there any way to share more code between these two?

There shouldn't be any duplication of logic except maybe the "useAA = !drawTransform.isIntegerTranslation()" part which is a one liner once the chromeos driver workaround is gone. Any specific part you're thinking of?

> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:241
> > +    typedef FloatPoint3D Edge;
> > +
> > +    static bool isCCW(const FloatQuad&);
> > +    static Edge computeEdge(const FloatPoint&, const FloatPoint&, float);
> > +    static FloatPoint intersect(const Edge&, const Edge&);
> > +    static void toGLVec3(float*, const Edge&);
> > +
> > +    struct Quad {
> > +        Quad(const FloatQuad &);
> > +
> > +        void inflateX(float dx) { left.move(0, 0, dx); right.move(0, 0, dx); }
> > +        void inflateY(float dy) { top.move(0, 0, dy); bottom.move(0, 0, dy); }
> > +        void inflate(float d) { inflateX(d); inflateY(d); }
> > +
> > +        FloatQuad floatQuad();
> > +
> > +        Edge left;
> > +        Edge top;
> > +        Edge right;
> > +        Edge bottom;
> > +    };
> > +
> > +    // This method removes Z component from matrix. Useful when matrix
> > +    // is only used for transforming X/Y components.
> > +    static TransformationMatrix noZMatrix(const TransformationMatrix&);
> > +
> 
> This is not good, none of this belongs on LayerChromium.  Draw-time concerns shouldn't be on LayerChromium (and we need to move drawTexturedQuad at some point) and I have no idea what any of these have to do with any Layer type.  Are they just math utilities?

Yes, they are just utility functions for anti-aliased edge computations. Where would be a better place for these?

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:148
> > +                shaderSamplerLocation = maskProgramAA->fragmentShader().samplerLocation();
> > +                shaderMaskSamplerLocation = maskProgramAA->fragmentShader().maskSamplerLocation();
> 
> Are you querying these on every draw? That's going to be ridiculously slow, isn't it?

All Location() functions should be free. They just return a variable set when initializing the shader.
Comment 6 David Reveman 2011-08-24 09:04:21 PDT
Created attachment 105005 [details]
Patch
Comment 7 David Reveman 2011-08-24 09:10:25 PDT
No major changes in latest patch. Just rebased on ToT. I'm still waiting for some more feedback.
Comment 8 Adrienne Walker 2011-08-24 11:01:32 PDT
(In reply to comment #7)
> No major changes in latest patch. Just rebased on ToT. I'm still waiting for some more feedback.

I will second jamesr's previous feedback. Draw-time functionality shouldn't be on LayerChromium.

If you're trying to move common functionality out of CCTiledLayerImpl, I would suggest making some common CCEdge class (with a computeEdge and intersect function) and move isCCW into the FloatQuad class itself.  Then both CCTiledLayerImpl and CCRenderSurface can use this class.
Comment 9 David Reveman 2011-08-25 10:22:09 PDT
Created attachment 105209 [details]
Patch
Comment 10 Adrienne Walker 2011-08-29 17:42:44 PDT
Comment on attachment 105209 [details]
Patch

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

This is looking good.  Thanks for the cleanup.  :)

Are the image failures platform-specific? Can you add (at least one platform's) pixel results to this patch?

> Source/WebCore/platform/graphics/FloatQuad.h:122
> +    // Tests whether points are in clock-wise, or counter clock-wise order.
> +    bool isCCW() const;

I know this function has existed for a while, but can you not abbreviate counterclockwise? I'd prefer to drop the comment and just have a more informative function name.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerQuad.h:117
> +    void toGL(float*);

Can you call this something more descriptive like toFloatArray and have it take a float[12] instead of a float*?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:118
>      const CCRenderSurface::Program* program = layerRenderer()->renderSurfaceProgram();

Can you move this down into the scope where program is used?
Comment 11 David Reveman 2011-08-30 08:25:25 PDT
(In reply to comment #10)
> (From update of attachment 105209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105209&action=review
> 
> This is looking good.  Thanks for the cleanup.  :)
> 
> Are the image failures platform-specific? Can you add (at least one platform's) pixel results to this patch?

They shouldn't be platform specific. webkit-patch seem to have trouble with large patches so I'll upload a new patch without pixel results first.

> 
> > Source/WebCore/platform/graphics/FloatQuad.h:122
> > +    // Tests whether points are in clock-wise, or counter clock-wise order.
> > +    bool isCCW() const;
> 
> I know this function has existed for a while, but can you not abbreviate counterclockwise? I'd prefer to drop the comment and just have a more informative function name.

Done.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerQuad.h:117
> > +    void toGL(float*);
> 
> Can you call this something more descriptive like toFloatArray and have it take a float[12] instead of a float*?

Done.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:118
> >      const CCRenderSurface::Program* program = layerRenderer()->renderSurfaceProgram();
> 
> Can you move this down into the scope where program is used?

Done.
Comment 12 David Reveman 2011-08-30 08:32:39 PDT
Created attachment 105634 [details]
Patch
Comment 13 David Reveman 2011-09-02 11:00:13 PDT
Created attachment 106156 [details]
Patch
Comment 14 David Reveman 2011-09-02 11:07:38 PDT
Latest patch has been rebased and includes new baselines for linux. As always with these larger patches I'm not sure the new expected images were included correctly so I didn't obsolete the old patch.
Comment 15 David Reveman 2011-09-02 11:13:36 PDT
Created attachment 106162 [details]
Patch
Comment 16 David Reveman 2011-09-02 11:15:34 PDT
CCLayerQuad.cpp, CCLayerQuad.h missing in previous patch.
Comment 17 James Robinson 2011-09-02 13:58:15 PDT
Comment on attachment 106162 [details]
Patch

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

The uniform stuff seems very bug-prone, there's 4 nearly identical repetitions in CCRenderSurface.cpp. I also suspect that you're leaking programs.

> Source/WebCore/platform/graphics/FloatQuad.h:121
> +    // Tests whether points are in clock-wise, or counter clock-wise order.

Can you document what this does when all points are colinear?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:225
> +    OwnPtr<CCRenderSurface::MaskProgramAA> m_renderSurfaceMaskProgramAA;
> +    OwnPtr<CCRenderSurface::ProgramAA> m_renderSurfaceProgramAA;

if you're adding new programs you need to add cleanup code to LayerRendererChromium::cleanupSharedObjects(). You should hit ASSERTs in debug mode, have you run the tests in debug?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:151
> +                shaderSamplerLocation = maskProgramAA->fragmentShader().samplerLocation();
> +                shaderMaskSamplerLocation = maskProgramAA->fragmentShader().maskSamplerLocation();
> +                shaderMatrixLocation = maskProgramAA->vertexShader().matrixLocation();
> +                shaderQuadLocation = maskProgramAA->vertexShader().pointLocation();
> +                shaderAlphaLocation = maskProgramAA->fragmentShader().alphaLocation();
> +                shaderEdgeLocation = maskProgramAA->fragmentShader().edgeLocation();

this is so horribly repetitive and error-prone. is there any better way to do this?
Comment 18 David Reveman 2011-09-02 15:19:34 PDT
(In reply to comment #17)
> (From update of attachment 106162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106162&action=review
> 
> The uniform stuff seems very bug-prone, there's 4 nearly identical repetitions in CCRenderSurface.cpp. I also suspect that you're leaking programs.
> 
> > Source/WebCore/platform/graphics/FloatQuad.h:121
> > +    // Tests whether points are in clock-wise, or counter clock-wise order.
> 
> Can you document what this does when all points are colinear?

Done.

> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:225
> > +    OwnPtr<CCRenderSurface::MaskProgramAA> m_renderSurfaceMaskProgramAA;
> > +    OwnPtr<CCRenderSurface::ProgramAA> m_renderSurfaceProgramAA;
> 
> if you're adding new programs you need to add cleanup code to LayerRendererChromium::cleanupSharedObjects(). You should hit ASSERTs in debug mode, have you run the tests in debug?

Good catch. Fixed. I didn't hit any ASSERTs from either the unit tests or layout test. Is there some more test I should be running?

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:151
> > +                shaderSamplerLocation = maskProgramAA->fragmentShader().samplerLocation();
> > +                shaderMaskSamplerLocation = maskProgramAA->fragmentShader().maskSamplerLocation();
> > +                shaderMatrixLocation = maskProgramAA->vertexShader().matrixLocation();
> > +                shaderQuadLocation = maskProgramAA->vertexShader().pointLocation();
> > +                shaderAlphaLocation = maskProgramAA->fragmentShader().alphaLocation();
> > +                shaderEdgeLocation = maskProgramAA->fragmentShader().edgeLocation();
> 
> this is so horribly repetitive and error-prone. is there any better way to do this?

I agree. Added a template class for the program like we use in CCTiledLayerImpl. I think this improves it a bit.
Comment 19 David Reveman 2011-09-02 15:19:52 PDT
Created attachment 106206 [details]
Patch
Comment 20 James Robinson 2011-09-02 15:50:14 PDT
(In reply to comment #18)
> > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:225
> > > +    OwnPtr<CCRenderSurface::MaskProgramAA> m_renderSurfaceMaskProgramAA;
> > > +    OwnPtr<CCRenderSurface::ProgramAA> m_renderSurfaceProgramAA;
> > 
> > if you're adding new programs you need to add cleanup code to LayerRendererChromium::cleanupSharedObjects(). You should hit ASSERTs in debug mode, have you run the tests in debug?
> 
> Good catch. Fixed. I didn't hit any ASSERTs from either the unit tests or layout test. Is there some more test I should be running?
> 
Interesting, running the layout tests should be sufficient.  I wonder if our assertions are broken then...
Comment 21 James Robinson 2011-09-02 16:31:38 PDT
Comment on attachment 106206 [details]
Patch

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

Code wise, this is looking pretty good.  I think some of your baselines aren't necessary any more (for example compositing/reflections/animation-inside-reflection-expected.png), so when you update please double check those.  This also seems to not apply to ToT, you'll have to shuffle some stuff over.

I think Enne or Vangelis should look over the math since I don't feel entirely comfortable verifying that bit, but that aside I'm ready to r+

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:110
> +template <class T>

nit: T -> Program ?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:174
> +            drawSurface(maskLayer, drawTransform, layerRenderer()->renderSurfaceMaskProgramAA(), layerRenderer()->renderSurfaceMaskProgramAA()->fragmentShader().maskSamplerLocation(), layerRenderer()->renderSurfaceMaskProgramAA()->vertexShader().pointLocation(), layerRenderer()->renderSurfaceMaskProgramAA()->fragmentShader().edgeLocation());

this is so much better. it could be even shorter if you stored layerRenderer()->renderSurfaceMaskProgram() in a temporary (same goes below)

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:102
> +    template <class T>
> +    void drawSurface(CCLayerImpl*, const TransformationMatrix&, const T*, int, int, int);

nit: instead of T, could it be Program?
Comment 22 David Reveman 2011-09-06 06:12:51 PDT
(In reply to comment #21)
> (From update of attachment 106206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106206&action=review
> 
> Code wise, this is looking pretty good.  I think some of your baselines aren't necessary any more (for example compositing/reflections/animation-inside-reflection-expected.png), so when you update please double check those.  This also seems to not apply to ToT, you'll have to shuffle some stuff over.

I noticed. Hopefully I got it right in my latest patch.

> 
> I think Enne or Vangelis should look over the math since I don't feel entirely comfortable verifying that bit, but that aside I'm ready to r+

The math hasn't really changed. The code has just been moved around. But still, a second set of eyes to verify that I didn't make any typos wouldn't hurt.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:110
> > +template <class T>
> 
> nit: T -> Program ?

"Program" name is already used for the shaders. 'T' for templates seem to be widely used in WebCore so I'd prefer to stick to that unless you have a better suggestion.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:174
> > +            drawSurface(maskLayer, drawTransform, layerRenderer()->renderSurfaceMaskProgramAA(), layerRenderer()->renderSurfaceMaskProgramAA()->fragmentShader().maskSamplerLocation(), layerRenderer()->renderSurfaceMaskProgramAA()->vertexShader().pointLocation(), layerRenderer()->renderSurfaceMaskProgramAA()->fragmentShader().edgeLocation());
> 
> this is so much better. it could be even shorter if you stored layerRenderer()->renderSurfaceMaskProgram() in a temporary (same goes below)

Done. Changed CCTiledLayerImpl.cpp as well for consistency.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:102
> > +    template <class T>
> > +    void drawSurface(CCLayerImpl*, const TransformationMatrix&, const T*, int, int, int);
> 
> nit: instead of T, could it be Program?

Same as above. Having parameter names in the header as CCTiledLayerImpl.h would make this less obscure but that would go against the style guidelines..
Comment 23 David Reveman 2011-09-06 06:16:54 PDT
Created attachment 106410 [details]
Patch
Comment 24 James Robinson 2011-09-06 09:02:24 PDT
You can have parameter names in the header if they are informative, just not if they are redundant with the type name or the function name. If you have a bunch of int parameters please do name them in the header.
Comment 25 David Reveman 2011-09-06 09:32:48 PDT
(In reply to comment #24)
> You can have parameter names in the header if they are informative, just not if they are redundant with the type name or the function name. If you have a bunch of int parameters please do name them in the header.

Thanks for clarifying that. Uploading a new patch.
Comment 26 David Reveman 2011-09-06 09:33:25 PDT
Created attachment 106428 [details]
Patch
Comment 27 Adrienne Walker 2011-09-06 11:41:11 PDT
I'll unofficially review the math part of this and say that it looks good to me.  There really isn't much change here, just some refactoring to use it in a different place and a slight difference in shader logic between tiles with border texels and render surfaces without.

However, my only confusion is some of the test results.  Thanks for posting them.  Some of them look great (e.g. nested-reflection-transformed!).  However, I'm a little confused why there's any rendering change for nested-reflection-opacity and deeply-nested-reflections.  Is it the negative scale causing this shader to be used? Is it worth trying to catch this case too?
Comment 28 David Reveman 2011-09-06 12:01:55 PDT
(In reply to comment #27)
> I'll unofficially review the math part of this and say that it looks good to me.  There really isn't much change here, just some refactoring to use it in a different place and a slight difference in shader logic between tiles with border texels and render surfaces without.
> 
> However, my only confusion is some of the test results.  Thanks for posting them.  Some of them look great (e.g. nested-reflection-transformed!).  However, I'm a little confused why there's any rendering change for nested-reflection-opacity and deeply-nested-reflections.  Is it the negative scale causing this shader to be used? Is it worth trying to catch this case too?

Yes, the AA shader is used for all reflections even though they might not need it. I was planning to improve the useAA check to cover all cases where anti-aliasing isn't necessary in a subsequent patch. Might be a better idea to just include that with this patch. I'll see what I can do.
Comment 29 David Reveman 2011-09-06 15:36:44 PDT
Created attachment 106501 [details]
Patch
Comment 30 David Reveman 2011-09-06 15:43:31 PDT
Latest patch avoids using anti-aliasing shaders whenever possible. Which means some of the expected images that changed from previous AA patches will now need to be rebaselined again. This patch also fixed a few missing const modifiers in CCLayerQuad.h and adds CCLayerQuad::inflateAntiAliasingDistance to get rid of the magic 0.5f that existed in two places.
Comment 31 James Robinson 2011-09-06 19:15:10 PDT
Comment on attachment 106501 [details]
Patch

OK, R=me on the code.  Do you think the baselines in this patch are accurate, or no?
Comment 32 David Reveman 2011-09-06 19:27:23 PDT
(In reply to comment #31)
> (From update of attachment 106501 [details])
> OK, R=me on the code.  Do you think the baselines in this patch are accurate, or no?

I just copied the *-actual.png images generated from running the layout tests with "--debug --platform=chromium-gpu" on linux. I haven't verified that the non render surface related baseline changes are actually the same as before the first AA patch landed. They should be. Feel free to commit the patch without those updated baselines if you like.
Comment 33 WebKit Review Bot 2011-09-07 13:00:08 PDT
Comment on attachment 106501 [details]
Patch

Clearing flags on attachment: 106501

Committed r94703: <http://trac.webkit.org/changeset/94703>
Comment 34 WebKit Review Bot 2011-09-07 13:00:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Simon Fraser (smfr) 2013-03-07 16:14:56 PST
Comment on attachment 106501 [details]
Patch

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

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:349
> +    TransformationMatrix to2dTransform() const;

What was wrong with toAffineTransform() which already existed?
Comment 36 David Reveman 2013-03-07 20:55:29 PST
(In reply to comment #35)
> (From update of attachment 106501 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106501&action=review
> 
> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:349
> > +    TransformationMatrix to2dTransform() const;
> 
> What was wrong with toAffineTransform() which already existed?

to2dTransform() is not the best name. This was used by anti-aliasing code in the chromium compositor and it flattens the transform by setting 3rd row and 3rd column to (0, 0, 1, 0). Not sure this is used anymore now that the compositor lives in chromium.