Fix miter clipping of borders with large radius opposite a wide edge

Fix miter clipping of borders with large radius opposite a wide edge

The previous clipping approach for anti-aliasing miter joints
between border edges generated concave clipping quads that would
clip out the border itself in cases where a wide border was
opposite a large border radius.

This change switches the code from a parallelogram-derived clip
to a trapezoidal clip that will always include all of the border,
and is not prone to divide by zero under pathological cases.

New test fast/borders/border-large-radius-opposite-wide-edge.html
covers the previously failing cases. Some existing test results
change along single pixel anti-aliased lines due to slightly
different clips for the miters.

R=fmalita@chromium.org
BUG=331004
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2695013011
Cr-Commit-Position: refs/heads/master@{#451889}
Committed: https://chromium.googlesource.com/chromium/src/+/fc37a436628053178de0179e8d4566d3e1ea9800

[Stephen Chennney, 2017-02-17 17:51:49]

Description was changed from

==========
Fix miter clipping of borders with large radius opposite a wide edge

The previous clipping approach for anti-aliasing miter joints
between border edges generated concave clipping quads that would
clip out the border itself in cases where a wide border was
opposite a large border radius.

This change switches the code from a parallelogram-derived clip
to a trapezoidal clip that will always include all of the border,
and is not prone to divide by zero under pathological cases.

New test fast/borders/border-large-radius-opposite-wide-edge.html
covers the previously failing cases. Some existing test results
change along single pixel anti-aliased lines due to slightly
different clips for the miters.

R=fmalita@chromium.org
BUG=331004
==========

to

==========
Fix miter clipping of borders with large radius opposite a wide edge

The previous clipping approach for anti-aliasing miter joints
between border edges generated concave clipping quads that would
clip out the border itself in cases where a wide border was
opposite a large border radius.

This change switches the code from a parallelogram-derived clip
to a trapezoidal clip that will always include all of the border,
and is not prone to divide by zero under pathological cases.

New test fast/borders/border-large-radius-opposite-wide-edge.html
covers the previously failing cases. Some existing test results
change along single pixel anti-aliased lines due to slightly
different clips for the miters.

R=fmalita@chromium.org
BUG=331004
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen Chennney, 2017-02-17 20:51:07]

The test result changes are all anti-aliasing differences due to minor clip
changes. There are quite a few of them, however. Let me know if you would like
an explanation of how this was working and now works.

[f(malita), 2017-02-17 21:43:45]

https://codereview.chromium.org/2695013011/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):

https://codereview.chromium.org/2695013011/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1356: clippingQuad[1]
+= extensionOffset;
I don't fully follow the extension logic.  For e.g. BSTop/left miter, it seems
we would want to shift the left miter edge outward - that implies nudging both
clippingQuad[0] & clippingQuad[1], but instead we're nudging 1 & 2?

[Stephen Chennney, 2017-02-17 21:56:26]

Thanks, you spotted my mistake. I'll get a new version up on Tuesday.

https://codereview.chromium.org/2695013011/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):

https://codereview.chromium.org/2695013011/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1356: clippingQuad[1]
+= extensionOffset;
On 2017/02/17 21:43:45, f(malita) wrote:
> I don't fully follow the extension logic.  For e.g. BSTop/left miter, it seems
> we would want to shift the left miter edge outward - that implies nudging both
> clippingQuad[0] & clippingQuad[1], but instead we're nudging 1 & 2?

You are right. That explains my issues with the amount we need to nudge.

[f(malita), 2017-02-17 21:57:09]

On 2017/02/17 21:56:26, Stephen Chennney wrote:
> Thanks, you spotted my mistake. I'll get a new version up on Tuesday.
> 
>
https://codereview.chromium.org/2695013011/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):
> 
>
https://codereview.chromium.org/2695013011/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1356:
clippingQuad[1]
> += extensionOffset;
> On 2017/02/17 21:43:45, f(malita) wrote:
> > I don't fully follow the extension logic.  For e.g. BSTop/left miter, it
seems
> > we would want to shift the left miter edge outward - that implies nudging
both
> > clippingQuad[0] & clippingQuad[1], but instead we're nudging 1 & 2?
> 
> You are right. That explains my issues with the amount we need to nudge.

Related question: does that tiny extension make a noticeable difference, or
could we just drop it?

Other than that, the diffs look good to me.

[Stephen Chennney, 2017-02-17 22:03:17]

On 2017/02/17 21:57:09, f(malita) wrote:
> On 2017/02/17 21:56:26, Stephen Chennney wrote:
> > Thanks, you spotted my mistake. I'll get a new version up on Tuesday.
> > 
> >
>
https://codereview.chromium.org/2695013011/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2695013011/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:1356:
> clippingQuad[1]
> > += extensionOffset;
> > On 2017/02/17 21:43:45, f(malita) wrote:
> > > I don't fully follow the extension logic.  For e.g. BSTop/left miter, it
> seems
> > > we would want to shift the left miter edge outward - that implies nudging
> both
> > > clippingQuad[0] & clippingQuad[1], but instead we're nudging 1 & 2?
> > 
> > You are right. That explains my issues with the amount we need to nudge.
> 
> Related question: does that tiny extension make a noticeable difference, or
> could we just drop it?
> 
> Other than that, the diffs look good to me.

The extension is needed. Otherwise we get cracks in some cases when the color is
the same (hard clip).

[Stephen Chennney, 2017-02-21 18:47:35]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 18:48:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stephen Chennney, 2017-02-21 18:48:26]

New baselines are in. No crack visible so I think it's ready to go. The trybots
will tell us if I got the rebaselines correct.

[f(malita), 2017-02-21 20:04:25]

On 2017/02/21 18:48:26, Stephen Chennney wrote:
> New baselines are in. No crack visible so I think it's ready to go. The
trybots
> will tell us if I got the rebaselines correct.

LGTM

Would be nice if we could unify those branches at some point, but I recall
looking into it in the past and it wasn't trivial.

[Stephen Chennney, 2017-02-21 20:23:44]

The CQ bit was unchecked by schenney@chromium.org

[Stephen Chennney, 2017-02-21 20:23:51]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2017-02-21 20:25:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stephen Chennney, 2017-02-21 20:25:41]

On 2017/02/21 20:04:25, f(malita) wrote:
> On 2017/02/21 18:48:26, Stephen Chennney wrote:
> > New baselines are in. No crack visible so I think it's ready to go. The
> trybots
> > will tell us if I got the rebaselines correct.
> 
> LGTM
> 
> Would be nice if we could unify those branches at some point, but I recall
> looking into it in the past and it wasn't trivial.

I thought about it too. The issues, as you probably noticed, are that we would
have to encode the neighboring corner properties for style lookup of border
radius, and x-y are switched/reversed at times so we would have to index the
appropriate coordinates somehow. It seemed better to just leave it as is.

[commit-bot: I haz the power, 2017-02-21 20:52:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 20:52:02]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Stephen Chennney, 2017-02-21 20:56:25]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2017-02-21 20:57:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 23:00:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 23:00:02]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[f(malita), 2017-02-22 03:33:47]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2017-02-22 03:34:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 05:45:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487734427731540,
"parent_rev": "d30e259ee12ed576f29ca8a8d414ab9e5c8d66ba", "commit_rev":
"fc37a436628053178de0179e8d4566d3e1ea9800"}

[commit-bot: I haz the power, 2017-02-22 05:46:03]

Description was changed from

==========
Fix miter clipping of borders with large radius opposite a wide edge

The previous clipping approach for anti-aliasing miter joints
between border edges generated concave clipping quads that would
clip out the border itself in cases where a wide border was
opposite a large border radius.

This change switches the code from a parallelogram-derived clip
to a trapezoidal clip that will always include all of the border,
and is not prone to divide by zero under pathological cases.

New test fast/borders/border-large-radius-opposite-wide-edge.html
covers the previously failing cases. Some existing test results
change along single pixel anti-aliased lines due to slightly
different clips for the miters.

R=fmalita@chromium.org
BUG=331004
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix miter clipping of borders with large radius opposite a wide edge

The previous clipping approach for anti-aliasing miter joints
between border edges generated concave clipping quads that would
clip out the border itself in cases where a wide border was
opposite a large border radius.

This change switches the code from a parallelogram-derived clip
to a trapezoidal clip that will always include all of the border,
and is not prone to divide by zero under pathological cases.

New test fast/borders/border-large-radius-opposite-wide-edge.html
covers the previously failing cases. Some existing test results
change along single pixel anti-aliased lines due to slightly
different clips for the miters.

R=fmalita@chromium.org
BUG=331004
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2695013011
Cr-Commit-Position: refs/heads/master@{#451889}
Committed:
https://chromium.googlesource.com/chromium/src/+/fc37a436628053178de0179e8d45...
==========

[commit-bot: I haz the power, 2017-02-22 05:46:06]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/fc37a436628053178de0179e8d45...