Bug 66437 – [chromium] aliased render surface edges

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Reveman]

Anti-aliasing is currently only used for tiled layer edges. Render surface edges should be anti-aliased too.

[Adrienne Walker]

Out of curiosity, are you going to address bug 65483 as a part of this (or before this)?

[David Reveman]

(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.

[David Reveman]

Created attachment 104439 [details]
Patch

[James Robinson]

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?

[David Reveman]

(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.

[David Reveman]

Created attachment 105005 [details]
Patch

[David Reveman]

No major changes in latest patch. Just rebased on ToT. I'm still waiting for some more feedback.

[Adrienne Walker]

(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.

[David Reveman]

Created attachment 105209 [details]
Patch

[Adrienne Walker]

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?

[David Reveman]

(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.

[David Reveman]

Created attachment 105634 [details]
Patch

[David Reveman]

Created attachment 106156 [details]
Patch

[David Reveman]

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.

[David Reveman]

Created attachment 106162 [details]
Patch

[David Reveman]

CCLayerQuad.cpp, CCLayerQuad.h missing in previous patch.

[James Robinson]

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?

[David Reveman]

(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.

[David Reveman]

Created attachment 106206 [details]
Patch

[James Robinson]

(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...

[James Robinson]

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?

[David Reveman]

(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..

[David Reveman]

Created attachment 106410 [details]
Patch

[James Robinson]

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.

[David Reveman]

(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.

[David Reveman]

Created attachment 106428 [details]
Patch

[Adrienne Walker]

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?

[David Reveman]

(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.

[David Reveman]

Created attachment 106501 [details]
Patch

[David Reveman]

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.

[James Robinson]

Comment on attachment 106501 [details]
Patch

OK, R=me on the code.  Do you think the baselines in this patch are accurate, or no?

[David Reveman]

(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.

[WebKit Review Bot]

Comment on attachment 106501 [details]
Patch

Clearing flags on attachment: 106501

Committed r94703: <http://trac.webkit.org/changeset/94703>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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?

[David Reveman]

(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.