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

RESOLVED FIXED in Firefox 41

Status

()

Core
Graphics
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: lsalzman, Assigned: lsalzman)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed)

Details

Attachments

(7 attachments, 9 obsolete attachments)

1.96 KB, patch
jrmuizel
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.63 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.89 KB, patch
jrmuizel
: review+
mstange
: review+
Details | Diff | Splinter Review
8.79 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
3.51 KB, patch
mstange
: review+
Details | Diff | Splinter Review
7.10 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
28.89 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8636146 [details] [diff] [review]
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.
Attachment #8636146 - Flags: review?(jmuizelaar)
(Assignee)

Comment 2

3 years ago
Created attachment 8636147 [details] [diff] [review]
part 2 - Add some utility methods to Point and Size

Add some useful utility methods for doing math on 2d Points and Sizes.
Attachment #8636147 - Flags: review?(jmuizelaar)
(Assignee)

Comment 3

3 years ago
Created attachment 8636149 [details] [diff] [review]
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.
Attachment #8636149 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

3 years ago
Created attachment 8636150 [details] [diff] [review]
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.
Attachment #8636150 - Flags: review?(mstange)
Attachment #8636150 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8636150 - Attachment description: Implement CSS border corners by splitting geometry instead of gradients with hard stopspart 4 - → part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops
(Assignee)

Comment 5

3 years ago
Created attachment 8636151 [details] [diff] [review]
part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

Fix missing patch content
Attachment #8636150 - Attachment is obsolete: true
Attachment #8636150 - Flags: review?(mstange)
Attachment #8636150 - Flags: review?(jmuizelaar)
Attachment #8636151 - Flags: review?(mstange)
Attachment #8636151 - Flags: review?(jmuizelaar)
(Assignee)

Comment 6

3 years ago
Created attachment 8636157 [details] [diff] [review]
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.
Attachment #8636157 - Flags: review?(mstange)
Attachment #8636157 - Flags: review?(jmuizelaar)
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.
(Assignee)

Comment 8

3 years ago
Created attachment 8636274 [details] [diff] [review]
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.
Attachment #8636151 - Attachment is obsolete: true
Attachment #8636151 - Flags: review?(mstange)
Attachment #8636151 - Flags: review?(jmuizelaar)
Attachment #8636274 - Flags: review?(mstange)
Attachment #8636274 - Flags: review?(jmuizelaar)
(Assignee)

Comment 9

3 years ago
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
Attachment #8636146 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 10

3 years ago
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
(Assignee)

Comment 11

3 years ago
Created attachment 8637860 [details] [diff] [review]
part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

Fix intersection angle at splits
Attachment #8636274 - Attachment is obsolete: true
Attachment #8636274 - Flags: review?(mstange)
Attachment #8636274 - Flags: review?(jmuizelaar)
Attachment #8637860 - Flags: review?(mstange)
Attachment #8637860 - Flags: review?(jmuizelaar)
(Assignee)

Comment 12

3 years ago
Created attachment 8637864 [details] [diff] [review]
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
Attachment #8637864 - Flags: review?(mstange)
Attachment #8637864 - Flags: review?(jmuizelaar)
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 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?
Attachment #8637860 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 15

3 years ago
Created attachment 8638140 [details] [diff] [review]
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
Attachment #8637860 - Attachment is obsolete: true
Attachment #8637860 - Flags: review?(mstange)
Attachment #8638140 - Flags: review?(mstange)
Attachment #8638140 - Flags: review?(jmuizelaar)
(Assignee)

Comment 16

3 years ago
Created attachment 8638141 [details] [diff] [review]
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.
Attachment #8638141 - Flags: review?(mstange)
Attachment #8638141 - Flags: review?(jmuizelaar)
(Assignee)

Comment 17

3 years ago
Created attachment 8639362 [details] [diff] [review]
part 3 - Refactor ArcToBezier so that its implementation can be more easily reused

Fixed bitrot in ArcToBezier
Attachment #8636149 - Attachment is obsolete: true
Attachment #8639362 - Flags: review+
(Assignee)

Comment 18

3 years ago
Created attachment 8639364 [details] [diff] [review]
part 5 - fuzz some reftests to compensate for new border rendering approach

Fix bitrot in border-radius reftest.list
Attachment #8636157 - Attachment is obsolete: true
Attachment #8636157 - Flags: review?(mstange)
Attachment #8639364 - Flags: review?(mstange)
Lee, I am wondering why patch1 is not in m-c. Please let me know and then I can approve the uplift to Aurora.
Flags: needinfo?(lsalzman)
(Assignee)

Updated

3 years ago
Flags: needinfo?(lsalzman)
Attachment #8636146 - Flags: checkin?
(Assignee)

Comment 20

3 years ago
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.
Flags: needinfo?(ryanvm)
Yes, that's exactly what the checkin? flag is intended for :)
Flags: needinfo?(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
Attachment #8636146 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/2a4f41a5f0be
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Comment 24

3 years ago
Still need to get the other patches checked in before this can be considered fully fixed, though at least the regression has been addressed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8636146 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Attachment #8638140 - Flags: review?(mstange) → review+
Attachment #8638141 - Flags: review?(mstange) → review+
Attachment #8639364 - Flags: review?(mstange) → review+
Attachment #8637864 - Flags: review?(mstange) → review+
(Assignee)

Comment 27

3 years ago
Created attachment 8641766 [details] [diff] [review]
part 4 - Implement CSS border corners by splitting geometry instead of gradients with hard stops

Fixed all issues noted by Markus Stange.
Attachment #8638140 - Attachment is obsolete: true
Attachment #8641766 - Flags: review+
(Assignee)

Comment 28

3 years ago
Created attachment 8641768 [details] [diff] [review]
part 4.5 - don't use a css border skirt when printing

Fixed bitrot from update to part 4.
Attachment #8638141 - Attachment is obsolete: true
Attachment #8641768 - Flags: review+
(Assignee)

Comment 29

3 years ago
Created attachment 8641778 [details] [diff] [review]
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.
Attachment #8641766 - Attachment is obsolete: true
Attachment #8641778 - Flags: review+
(Assignee)

Comment 30

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

Updated

3 years ago
Depends on: 1191609
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1183506
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1033375
(Assignee)

Updated

3 years ago
See Also: → bug 1205793
Duplicate of this bug: 1154865
You need to log in before you can comment on or make changes to this bug.