766345 - Firefox lacks support for CSS 3D Anti-aliasing with OpenGL

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bobby Richter [:secretrobotron]]

http://k88hudson.github.com/sadcube/

This link will lead you to a simple webpage featuring a spinning cube. On Chrome, it's beautifully anti-aliased, and no jagged edges appear. On Firefox, however, it's pretty jagged.

Comment 1

[Joe Drew (not getting mail)]

Oh my. I guess we just want to ask for a multisample context.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Yup; Chrome does antialias and does use GL there according to about:gpu, so I suppose that's what they do.

2x2 MSAA is a negligible perf hit on decent hardware, but is very costly on Intel integrated graphics. I wonder how Chrome deals with that. In fact, it'd be interesting to see if the Mac is running on integrated or discrete while running a patch with CSS 3D.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Benoit Jacob [:bjacob] from comment #2)
> Yup; Chrome does antialias and does use GL there according to about:gpu, so
> I suppose that's what they do.
> 
> 2x2 MSAA is a negligible perf hit on decent hardware, but is very costly on
> Intel integrated graphics. I wonder how Chrome deals with that. In fact,
> it'd be interesting to see if the Mac is running on integrated or discrete
> while running a patch with CSS 3D.

Core animation does not use MSAA and last I checked Chrome didn't either. They both use analytic anti-aliasing which is pretty easy because we're just drawing quads.

Comment 4

[Bas Schouten (:bas.schouten)]

Also, doing MSAA is not trivial. Multisampled surfaces have certain limitations and would need to be explicitly resolved to draw into a parent container. In addition to that the performance cost can be significant as you need to have a multisampled context for the entire parent layer area.

Finally 2x2 does not exactly give 'beautiful anti-aliased' :)

Comment 5

[Kelsey Gilbert [:jgilbert]]

2xAA isn't beautiful, but it goes a long way towards smoothing ugliness.

Comment 6

[Ralph Thomas]

Here's a write up on the technique used by some of the WebKit ports: http://abandonedwig.info/blog/2013/02/24/edge-distance-anti-aliasing.html

Because the edge distance shader can be expensive, the edges are typically drawn in a separate pass, and the unantialiased quad is offset inwards (and if you're doing that, then you can optimize further and just pass the distance per-vertex so that the fragment shader does almost no math). This result will give much better visual results than MSAA, especially on near-horizontal or near-vertical lines where MSAA runs out of subsamples (so with 2xMSAA you'd have one shade of grey instead of a ramp).

Comment 7

[:kip (Kearwood Gilbert)]

The edge aliasing is especially noticeable when playing the animations at bouncejs.com

Both Chrome and Safari appear to have implemented edge anti-aliasing.  Has anyone started work on a Firefox implementation?

Comment 8

[PrinceN]

Hello, is there anything new with this problem? 

Because I do think it is a big issue today when you have 3 major webkit browsers that allow very interesting 3D rendering effects without flaw.. It is a major problem with Gecko

Comment 9

[:kip (Kearwood Gilbert)]

To reduce the overhead of the distance-to-edge shader, we can selectively apply it where it will have an effect.  We only need to apply the shader on transformed geometry.

If want to wrench the last bit of perf out of it, we could also experiment with applying the shader to just the edge pixels; however, we might find that the draw call cost of doing the edges separately might be more expensive than applying the distance-to-edge shader to the entire poly.

Comment 10

[:kip (Kearwood Gilbert)]

I'm planning to take this bug once my priority bugs are cleared.  Please NI me on any questions.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Apple was running the shader over the whole quad when I looked last. (5 years ago or something like that) We're probably safe doing the same.

Comment 12

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers

This is an experimental implementation of DEAA, which works for simple rotations only.

Final implementation will need to handle perspective transforms, expand the quads vertices by 0.5px post-transformation, and adjust texture UV's to compensate.  Will need to implement support for multiple quads rendered in one draw call (current OGL compositor supports up to 4).  Also will be necessary to handle both pre-multiplied alpha and non-pre-multiplied alpha.

Comment 13

[Andy Mercer]

This is an example of this bug. Test in current FF and current Chrome, the difference in almost cringe-worthy. http://codepen.io/ImBobby/pen/uKbnl

Comment 14

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers

Work in progress..  Works on all of the examples listed in the comments and all other sites that I have tested it on.

- Backfacing polygons are now rendered correctly.
- Subpixel positioning of textured content and rounded corner masks now correct
- Need to correct artifact on edges between tiles of transformed layers
- Needs cleanup, pruning of experimental code

Comment 15

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers

- Almost all artifacts corrected, tested on pages:
http://codepen.io/ImBobby/full/uKbnl/
http://codepen.io/TheDutchCoder/full/bfAho/
http://bouncejs.com
http://codepen.io/keithclark/full/dvigs/
http://codepen.io/noeldelgado/full/gcKJL/

The Sad Cube example still has a black line appearing in the shadow (http://k88hudson.github.com/sadcube/)

May need to correct polygon dilation to only dilate external edges of tiles to avoid artifacts between tiles on semi-transparent layers and partially covered AA edges between tiles.

Comment 16

[:kip (Kearwood Gilbert)]

Attachment: DEAA_Disabled.png

Screenshot from site in Comment 13, with DEAA Disabled (Before)

http://codepen.io/ImBobby/pen/uKbnl

Comment 17

[:kip (Kearwood Gilbert)]

Attachment: DEAA_Enabled.png

Screenshot from site in Comment 13, with DEAA Enabled (After)

http://codepen.io/ImBobby/pen/uKbnl

Comment 18

[Andy Mercer]

Kip, this looks great!

Comment 19

[:kip (Kearwood Gilbert)]

(In reply to :kip (Kearwood Gilbert) from comment #15)
> Created attachment 8585815 [details] [diff] [review]
> Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers
> 
> - Almost all artifacts corrected, tested on pages:
> http://codepen.io/ImBobby/full/uKbnl/
> http://codepen.io/TheDutchCoder/full/bfAho/
> http://bouncejs.com
> http://codepen.io/keithclark/full/dvigs/
> http://codepen.io/noeldelgado/full/gcKJL/
> 
> The Sad Cube example still has a black line appearing in the shadow
> (http://k88hudson.github.com/sadcube/)
> 
> May need to correct polygon dilation to only dilate external edges of tiles
> to avoid artifacts between tiles on semi-transparent layers and partially
> covered AA edges between tiles.
I have confirmed that the black line in the shadow of the Sad Cube example was due to dilating inner edges of layers.  Once I've fixed this, I'll update the patch and assign for review

Comment 20

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers

Once Vlad has reviewed this and I have addressed any issues he may have, I'll pass this by Bas as well.

Comment 21

[:kip (Kearwood Gilbert)]

The "Part 1" patch only implements anti-aliasing for the OpenGL Compositor.  The DirectX compositor will need a similar shader and may share some of the functionality in this patch.

Perhaps this bug should be split up / made to a meta bug to track the separate implementations.

Comment 22

[:kip (Kearwood Gilbert)]

I have updated the description and user story to reflect that this bug is tracking only the OpenGL implementation.

I have added Bug 1151543 to track the DirectX implementation, and will be adding a meta bug to track the feature.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8587757 [details] [diff] [review]
Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers

Review of attachment 8587757 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall; some nits, and one comment to think about as far as the number of shaders we need to generate.  I think we can just have one shader for DEAA enabled and one for it disabled, instead of 4 separate edge-count shaders.  I'll r+ because it's OK as it is, but let's talk about that first before you land.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +897,5 @@
>                            srcAlphaBlend, dstAlphaBlend);
>    return true;
>  }
>  
> +gfx::Point3D CompositorOGL::GetLineCoefficients(const gfx::Point3D& aPoint1,

nit: return type on its own line

@@ +1069,5 @@
> +    quadVerts[0] = Point4D(aVisibleRect.x, aVisibleRect.y, 0.0, 1.0);
> +    quadVerts[1] = Point4D(aVisibleRect.x + aVisibleRect.width, aVisibleRect.y, 0.0, 1.0);
> +    quadVerts[2] = Point4D(aVisibleRect.x + aVisibleRect.width, aVisibleRect.y + aVisibleRect.height, 0.0, 1.0);
> +    quadVerts[3] = Point4D(aVisibleRect.x, aVisibleRect.y + aVisibleRect.height, 0.0, 1.0);
> +    for(int i=0; i<4; i++) {

nit: spaces (after for, around = and <)

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +153,5 @@
> +
> +  SetFeature(ENABLE_DEAA_1EDGE, aEdgeCount == 1);
> +  SetFeature(ENABLE_DEAA_2EDGE, aEdgeCount == 2);
> +  SetFeature(ENABLE_DEAA_3EDGE, aEdgeCount == 3);
> +  SetFeature(ENABLE_DEAA_4EDGE, aEdgeCount == 4);

I'm assuming that we're going to do DEAA on desktop, and off on mobile (except maybe on recent mobile hardware).  In that case, it may be simpler to just have FEATURE_DEAA be a single on/off toggle for the shader, and always configure with 4 edges.  Doing that would potentially avoid shader changes, though is the assumption that we'll generally be using the 4-edge case anyway?

I guess one reason to not do this is that it would mean always doing 4 dot products in the fragment shader instead of 1-4.  But if the assumption is always 4 or 0 (turn off DEAA for fully-inner tiles), the perf hit for the 1-3 case is likely going to be very minor, and we could do something smart like render inner tiles first and outer tiles last to avoid shader changes.  Also given that we know all the edges, can we generate coefficients such that the dot product will always be clamped to 1.0 for an edge that shouldn't be antialiased?  That would avoid the need for any dynamic if()s, at the expense of 1-3 unnecessary dot products (in the 1 2 or 3 edge case).

@@ +211,5 @@
> +    //           using dynamic vertex buffers instead of sending everything
> +    //           in through uniforms.  This would enable passing information
> +    //           about how to dilate each vertex explicitly and eliminate the
> +    //           need to extrapolate this with the sub-pixel coverage
> +    //           calculation in the vertex shader.

No reason why we can't do this -- want to file a followup?  We have one constant vertex buffer right now that we scale/translate in the shader, but I'm increasingly of the opinion that that causes more problems than it's worth.

@@ +443,5 @@
> +    // Calculate the sub-pixel coverage of the pixel and modulate its opacity
> +    // by that amount to perform DEAA.
> +    fs << "  vec3 ssPos = vec3(gl_FragCoord.xy, 1.0);" << endl;
> +    fs << "  float deaaCoverage = 1.0;" << endl;
> +    for(int i=0; i<deaaEdgeCount; i++) {

nit: spacing etc.

::: gfx/layers/opengl/OGLShaderProgram.h
@@ +179,5 @@
> +    }
> +
> +    float fp[12];
> +    float *d = fp;
> +    for(int i=0; i < cnt; i++) {

nit: space after for, space around = in i=0

::: gfx/thebes/gfxPrefs.h
@@ +300,5 @@
>    DECL_GFX_PREF(Once, "layers.offmainthreadcomposition.enabled", LayersOffMainThreadCompositionEnabled, bool, false);
>    DECL_GFX_PREF(Once, "layers.offmainthreadcomposition.force-enabled", LayersOffMainThreadCompositionForceEnabled, bool, false);
>    DECL_GFX_PREF(Live, "layers.offmainthreadcomposition.frame-rate", LayersCompositionFrameRate, int32_t,-1);
>    DECL_GFX_PREF(Once, "layers.offmainthreadcomposition.testing.enabled", LayersOffMainThreadCompositionTestingEnabled, bool, false);
> +  DECL_GFX_PREF(Live, "layers.opengl.deaa.enabled",            LayersOpenGLDEAAEnabled, bool, false);

Just change this to layers.deaa.enabled.  No need to have an OpenGL specific pref -- the pref can apply to the future D3D implementation as well, given that we'll only have one accelerated layer type in use at a time.

::: modules/libpref/init/all.js
@@ +4090,5 @@
>  // platform and are the optimal surface type.
>  pref("layers.gralloc.disable", false);
>  
> +// Enable DEAA antialiasing for transformed layers in the OpenGL Compositor
> +pref("layers.opengl.deaa.enabled", false);

layers.deaa.enabled

Comment 24

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers (v2 Patch), r=vladimir

v2 Patch:
- Updated with review in Comment 23
- Un-bitrotted
- Now only generating a 4-edge DEAA shader, as per Comment 23

Comment 25

[:kip (Kearwood Gilbert)]

Comment on attachment 8589329 [details] [diff] [review]
Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers (v2 Patch), r=vladimir

Vlad has given the first round of review, now sending to Bas to get his input.

Comment 26

[:kip (Kearwood Gilbert)]

I have pushed to try to test for any regressions or tests that need updating to accommodate anti-aliasing:

DEAA patch applied, not enabled by default:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2773fd36f3a4

DEAA patch applied, enabled by default on all platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9378337b3121

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8589329 [details] [diff] [review]
Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers (v2 Patch), r=vladimir

Bas is a bit under the gun for 37,38, have Dan take a look.

Comment 28

[u480271]

Comment on attachment 8589329 [details] [diff] [review]
Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers (v2 Patch), r=vladimir

Review of attachment 8589329 [details] [diff] [review]:
-----------------------------------------------------------------

With nits.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1026,5 @@
>        static_cast<EffectBlendMode*>(aEffectChain.mSecondaryEffects[EffectTypes::BLEND_MODE].get());
>      blendMode = blendEffect->mBlendMode;
>    }
>  
> +  // Only apply DEAA to uads that have been transformed such that aliasing

uads? Should that be quads?

@@ +1035,4 @@
>    bool colorMatrix = aEffectChain.mSecondaryEffects[EffectTypes::COLOR_MATRIX];
> +  ShaderConfigOGL config = GetShaderConfigFor(aEffectChain.mPrimaryEffect,
> +                                              maskType, blendMode, colorMatrix,
> +                                              bEnableAA ? 4 : 0);

Magic constants?

::: gfx/layers/opengl/CompositorOGL.h
@@ +325,5 @@
>    gfx::IntSize mWidgetSize;
>    nsRefPtr<GLContext> mGLContext;
>    UniquePtr<GLBlitTextureImageHelper> mBlitTextureImageHelper;
>    gfx::Matrix4x4 mProjMatrix;
> +  gfx::Matrix4x4 mProjMatrixNormalized;

Is this matrix used at all?

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +159,5 @@
>    ostringstream fs, vs;
>  
>    AddUniforms(result);
>  
> +

Extra whitespace

Comment 29

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers (v4 Patch), r=vladimir, r=djg

V4 Patch:
- Updated with review in Comment 28

Comment 30

[:kip (Kearwood Gilbert)]

Try push revealed a 3d-transform reftest failure:

layout/reftests/transform-3d/1035611-1.html

An example of a page that would break for this:

http://fff.cmiscm.com/#!/section/universe

An updated patch will be added shortly, to handle this case.

Comment 31

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 2: Implement Matrix4x4::ProjectAndClipLine

- Implemented Matrix4x4::ProjectAndClipLine, which emulates
the frustum clipping behavior of the GPU.

Comment 32

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 2: Implement Matrix4x4::TransformAndClipRect

- Implemented Matrix4x4::TransformAndClipRect, which emulates
the frustum clipping behavior of the GPU.

This is the same function added in Bug 1072898, which Matt has already reviewed.  I have split the function into this separate patch, as this bug may be landing earlier.

Comment 33

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 3: Implement correct clipping of DEAA edges for quads that cross the w=0 plane

- Use Matrix4x4::ProjectAndClipRect for accurate calculation of DEAA edge
line coefficients of quads clipped by the view frustum.

This corrects the DEAA antialiasing to work for cases commonly used in CSS transform based cubemap rendering and CSS transforms in WebVR.  Previously, the DEAA edges would incorrectly crop layers that are clipped by the view frustum.

Comment 34

[:kip (Kearwood Gilbert)]

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88f27bc29ad2

Comment 35

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 4: Adjust reftests

- Added fuzzy to some reftests, to address sub-pixel AA and float precision
differences that resulted in non-perceptable pixel differences on edges.

Comment 36

[:kip (Kearwood Gilbert)]

I have pushed this to try again to verify that the fuzzy() values set in part 4 patch will allow the reftests to pass:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6ba51192b4e

Comment 37

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 3: Implement correct clipping of DEAA edges for quads that cross the w=0 plane (v2 Patch)

- Use Matrix4x4::ProjectAndClipRect for accurate calculation of DEAA edge
line coefficients of quads clipped by the view frustum.

v2 Patch:
- Added / updated some comments

Comment 38

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 4: Adjust reftests (v2 patch)

- Added fuzzy to some reftests, to address sub-pixel AA and float precision
differences that resulted in non-perceptible pixel differences on edges.

v4 Patch:

- Adjusted fuzzy pixel count in reftests

Comment 39

[:kip (Kearwood Gilbert)]

I have pushed again to try to verify that the fuzzy pixel counts in the reftest manifest is sufficient:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f7d2b618d72

Comment 40

[:kip (Kearwood Gilbert)]

Comment on attachment 8604375 [details] [diff] [review]
Bug 766345 - Part 4: Adjust reftests (v2 patch)

Reftests passed in the last try run, just needed to allow some fuzzy() matching for the subpixel AA edges

Comment 41

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers (v5 Patch), r=vladimir, r=djg

Bug 766345 - Part 1: Implement DEAA Antialiasing for transformed layers (v5 Patch), r=vladimir, r=djg

v5 Patch:
- Un-bitrotted

Comment 42

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 2: Implement Matrix4x4::TransformAndClipRect (v2 Patch)

- Implemented Matrix4x4::TransformAndClipRect, which emulates
the frustum clipping behavior of the GPU.

v2 Patch:
- Un-bitrotted

Comment 43

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 3: Implement correct clipping of DEAA edges for quads that cross the w=0 plane (v3 Patch), r=vladimir

- Use Matrix4x4::ProjectAndClipRect for accurate calculation of DEAA edge
line coefficients of quads clipped by the view frustum.

v3 patch:
- un-bitrotted

Comment 44

[:kip (Kearwood Gilbert)]

Attachment: Bug 766345 - Part 4: Adjust reftests (v3 patch), r=vladimir

- Added fuzzy to some reftests, to address sub-pixel AA and float precision
differences that resulted in non-perceptable pixel differences on edges.

v3 Patch:
- Un-bitrotted

Comment 45

[:kip (Kearwood Gilbert)]

Try run for reftests is green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=240775c4bc0e

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3d1111cf166
https://hg.mozilla.org/integration/mozilla-inbound/rev/34415a83b7d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/201aa5e028fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4a60fc3eaf

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f3d1111cf166
https://hg.mozilla.org/mozilla-central/rev/34415a83b7d7
https://hg.mozilla.org/mozilla-central/rev/201aa5e028fb
https://hg.mozilla.org/mozilla-central/rev/5e4a60fc3eaf