1185636 - nsCSSBorderRenderer uses gradients with hard stops to implement corners with mismatched colors, causing platform dependent gradient rendering artifacts

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Lee Salzman [:lsalzman]]

In nsCSSBorderRenderer, to implement the case where the colors of two border sides mismatch at a corner, we set up a gradient with a hard stop running where the two sides meet and render geometry covering the entire corner, so the gradient will produce the necessary edge.

This has the drawback of producing backend-dependent results that are undesirable if the backend does not properly implement hard stops - with respect to Cairo's libpixman fallback handling of gradients with hard stops, see bug 1033375. Attempts to further workaround such backend-dependent behavior can produce yet further regressions - see bug 1183506.

The alternative approach is explicitly create geometry to represent the edge, so that such a corner gets broken up into two triangles or arcs, each of differing colors. A potential downside of this approach is it may cause some visible seam artifacts where anti-aliasing partially blends these edge pixels, not producing the necessary level of opacity along the edge. This artifact, in testing of others browsers, has been observed in both Chrome and Internet Explorer.

One way to workaround this seam artifact is to create a skirt along the edge, extending the geometry of one side underneath the other side, so that any blending errors are hidden. For transparent colors, this can be modified further by estimating a size for this skirt that, when subpixel coverage is considered, produces a desired target level of opacity.

Especially for the opaque case, the skirt approach will almost entirely hide any seam artifacts. For transparent cases, the skirt will mostly hide seams except where rounding and other backend rendering considerations cause the actual subpixel coverage to diverge from the estimated values used to compute skirt size.

Another potential benefit of doing things this way is the per-pixel work will be cheaper (especially with unaccelerated software backends), just filling a single color instead of having to do gradient calculations.

So, I am proposing a set of patches to implement this analytical approach instead of using gradients, given the above considerations.

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: part 1 - Remove hard stop workaround for Cairo due to regressions

Due to regressions observed in bug 1183506, this patch just does away with the workaround that was introduced by bug 1033375.

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: part 2 - Add some utility methods to Point and Size

Add some useful utility methods for doing math on 2d Points and Sizes.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: part 3 - Refactor ArcToBezier so that its implementation can be more easily reused

This refactors ArcToBezier so that most notably PartialArcToBezier is exposed and can be reused for implementing similar specializations like AcuteArcToBezier.

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

This patch finally removes the corner gradient approach in favor of just splitting the geometry along the corner seam. It also implements skirts to hide the seam, trying to estimate the size of the skirt so that it gives a level of subpixel coverage approximating a desired level of opacity along the seam.

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

Fix missing patch content

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: part 5 - fuzz some reftests to compensate for new border rendering approach

Since a border radii may potentially be split in half, depending on backend the overall curve may not line up pixel-perfect with earlier reftest results, so at least 3 reftests must be incidentally fuzzed to compensate.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8636151 [details] [diff] [review]
part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

Can the code that does the actual rendering be split out into a few helper functions? There are a few "} else" branches that are very far away from their "if" condition, and it makes the code hard to read.

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

I tried to do some considerable amount of refactoring where I could so that the rendering function was less monolithic.

Comment 9

[Lee Salzman [:lsalzman]]

Comment on attachment 8636146 [details] [diff] [review]
part 1 - Remove hard stop workaround for Cairo due to regressions

Approval Request Comment
[Feature/regressing bug #]: The regression was introduced in a patch for bug 1033375, and the regression was noted in bug 1183506.
[User impact if declined]: Users of the Cairo backend (non-D2D Windows, some OSX users, most Linux) have reported overly thick borders on sites that attempt to create 1px triangles via CSS.
[Describe test coverage new/current, TreeHerder]: Mochitests and reftests
[Risks and why]: No risk other than going back to the status quo in bug 1033375, which has been the case ever since we rendered CSS border corners with gradients. This individual patch does not add or change anything, merely removes a section of code that was added as part of the fix for bug 1033375, in preparation for later changes within this particular border rendering rewrite, but can function individually as a backout.
[String/UUID change made/needed]: None

Comment 10

[Lee Salzman [:lsalzman]]

Try results for just the first 4 patches without the fuzzing patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df7b7fb7643c

Try results with fuzz patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b7b92d97db1

Comment 11

[Lee Salzman [:lsalzman]]

Attachment: part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

Fix intersection angle at splits

Comment 12

[Lee Salzman [:lsalzman]]

Attachment: part 6 - add reftest for border-radius splits

This adds a reftest for checking that border radii are split in approximately the right places, exercising the border radius intersection code.

It works around some possible backend-dependent rendering issues with various backends by overlaying some SVG strokes on the test results, so that if edges are interpolated a bit differently it won't require fuzzing the test. This workaround is also present in some pre-existing border-radius reftests.

Try results for all patches including new reftest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2f14fbf7169

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8637860 [details] [diff] [review]
part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

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

It would be nice to not have the skirt when printing.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8637860 [details] [diff] [review]
part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

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

Seems sane to me.

::: layout/base/nsCSSRenderingBorders.cpp
@@ +1155,5 @@
> +  aSizeResult =
> +    slopeScale -
> +      sqrt(slopeScale*slopeScale +
> +           (1 - aAlpha2 / (aAlpha1 * (1.0f - 0.49f * aAlpha2)))
> +             / slope);

Are there any chances of things going bad here? i.e division by 0 or sqrt of negative?

Is it worth putting a isNaN at the end just in case?

Is the use of sqrt vs sqrtf intentional?

Comment 15

[Lee Salzman [:lsalzman]]

Attachment: part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

Fixed ComputeCornerSkirtSize to check for a negative discriminant and also be a bit more strict about absolute value of slopes

Comment 16

[Lee Salzman [:lsalzman]]

Attachment: part 4.5 - don't use a css border skirt when printing

This passes in the PresContextType to nsCSSBorderRenderer so that we can check if we're printing. When printing, there is no anti-aliasing, so skirts should be disabled because they don't do any good here.

Comment 17

[Lee Salzman [:lsalzman]]

Attachment: part 3 - Refactor ArcToBezier so that its implementation can be more easily reused

Fixed bitrot in ArcToBezier

Comment 18

[Lee Salzman [:lsalzman]]

Attachment: part 5 - fuzz some reftests to compensate for new border rendering approach

Fix bitrot in border-radius reftest.list

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Lee, I am wondering why patch1 is not in m-c. Please let me know and then I can approve the uplift to Aurora.

Comment 20

[Lee Salzman [:lsalzman]]

Comment on attachment 8636146 [details] [diff] [review]
part 1 - Remove hard stop workaround for Cairo due to regressions

Can the part1 patch get checked in to central, while the other parts are still waiting on review? I would like to at least not let the regression slide into beta, so need to get part1 in central for uplifting to aurora.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Yes, that's exactly what the checkin? flag is intended for :)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8636146 [details] [diff] [review]
part 1 - Remove hard stop workaround for Cairo due to regressions

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a4f41a5f0be

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2a4f41a5f0be

Comment 24

[Lee Salzman [:lsalzman]]

Still need to get the other patches checked in before this can be considered fully fixed, though at least the regression has been addressed.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8636146 [details] [diff] [review]
part 1 - Remove hard stop workaround for Cairo due to regressions

Seems safe as it's mostly commenting out some special-case handling on Cairo. Let's uplift to Aurora.

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 8638140 [details] [diff] [review]
part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

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

Thanks for the refactoring, I had an easier time reading the patch this time around. Sorry for taking so long, it's still a very hard patch to review.

::: gfx/2d/Types.h
@@ +270,5 @@
> +    return r == aColor.r && g == aColor.g && b == aColor.b && a == aColor.a;
> +  }
> +
> +  bool operator!=(const Color& aColor) const {
> +    return r != aColor.r || g != aColor.g || b != aColor.b || a != aColor.a;

How about "return !(*this == aColor);"

::: layout/base/nsCSSRenderingBorders.cpp
@@ +1171,5 @@
> +DrawBorderRadius(DrawTarget* aDrawTarget,
> +                 const Point& aSideStart, Float aSideWidth,
> +                 mozilla::css::Corner c,
> +                 const Point& aOuterCorner, const Point& aInnerCorner,
> +                 const twoFloats& aCornerMultX, const twoFloats& aCornerMultY,

Are "X" and "Y" appropriate here? This looks more like a Before/After to me. I could be wrong though.

@@ +1294,5 @@
> +  }
> +}
> +
> +static void
> +DrawCorner(DrawTarget* aDrawTarget,

Please add a comment that says that this not only draws the corner, but also previous side.

@@ +1300,5 @@
> +           mozilla::css::Corner c,
> +           const Point& aOuterCorner, const Point& aInnerCorner,
> +           const twoFloats& aCornerMultX, const twoFloats& aCornerMultY,
> +           const Size& aCornerDims,
> +           const ColorPattern& aFirstPat, const ColorPattern& aSecondPat,

I suggest passing just the color here, and creating ColorPattern objects only once you call the DrawTarget drawing methods. Then you'll be able to drop all the ".mColor" accesses below.

@@ +1382,4 @@
>    NS_FOR_CSS_CORNERS(i) {
>        // the corner index -- either 1 2 3 0 (cw) or 0 3 2 1 (ccw)
>      mozilla::css::Corner c = mozilla::css::Corner((i+1) % 4);
>      mozilla::css::Corner prevCorner = mozilla::css::Corner(i);

Can you add a comment here stating what parts of what side + what corner needs to be drawn during the current iteration of the loop?

@@ +1419,4 @@
>  
> +    ColorPattern firstPat(ToDeviceColor(firstColor)),
> +                 secondPat(firstColor == secondColor ?
> +                             firstPat.mColor : ToDeviceColor(secondColor));

This optimization is probably not worth doing. Just always use ToDeviceColor(secondColor).

@@ +1450,5 @@
> +      RefPtr<PathBuilder> builder = mDrawTarget->CreatePathBuilder();
> +      builder->MoveTo(sideStart + centerAdjusts[i] * sideWidth);
> +      builder->LineTo(sideEnd + centerAdjusts[i] * sideWidth);
> +      RefPtr<Path> path = builder->Finish();
> +      mDrawTarget->Stroke(path, firstPat, StrokeOptions(sideWidth));

Can this be done using mDrawTarget->StrokeLine instead?

Comment 27

[Lee Salzman [:lsalzman]]

Attachment: part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

Fixed all issues noted by Markus Stange.

Comment 28

[Lee Salzman [:lsalzman]]

Attachment: part 4.5 - don't use a css border skirt when printing

Fixed bitrot from update to part 4.

Comment 29

[Lee Salzman [:lsalzman]]

Attachment: part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

As per Markus Stange's request, I added one more comment documenting what exactly gets drawn on each iteration of the border rendering loop.

Comment 30

[Lee Salzman [:lsalzman]]

Did one last try run to make sure everything was still okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a40008502fa

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe577ec4e56
https://hg.mozilla.org/integration/mozilla-inbound/rev/09b212388126
https://hg.mozilla.org/integration/mozilla-inbound/rev/942beac221e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ba60c89c282
https://hg.mozilla.org/integration/mozilla-inbound/rev/706f21f9e349
https://hg.mozilla.org/integration/mozilla-inbound/rev/54c95895d377

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dfe577ec4e56
https://hg.mozilla.org/mozilla-central/rev/09b212388126
https://hg.mozilla.org/mozilla-central/rev/942beac221e4
https://hg.mozilla.org/mozilla-central/rev/0ba60c89c282
https://hg.mozilla.org/mozilla-central/rev/706f21f9e349
https://hg.mozilla.org/mozilla-central/rev/54c95895d377

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d07c2f504fd7