Bug 65583 – Use PATH_BASED_BORDER_RADIUS_DRAWING for skia

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ben Wells]

A new code path is used to draw borders from some platforms, guarded with PATH_BASED_BORDER_RADIUS_DRAWING. This code path is not used for skia. This new code path handles complex borders with border-radius much better, and is being actively improved and maintained. This change makes skia ports of WebKit use the new border drawing code path.

[Ben Wells]

Created attachment 102728 [details]
Patch

[Ben Wells]

This change doesn't have updated test expectations yet, I need to check a few more tests first.

[WebKit Review Bot]

Comment on attachment 102728 [details]
Patch

Attachment 102728 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9287878

New failing tests:
fast/borders/border-antialiasing.html
fast/box-shadow/basic-shadows.html
fast/box-shadow/inset-with-extraordinary-radii-and-border.html
fast/blockflow/border-radius-clipping-vertical-lr.html
fast/borders/border-radius-inline-flow.html
fast/box-shadow/inset-box-shadows.html
fast/backgrounds/gradient-background-leakage.html
fast/box-shadow/inset-box-shadow-radius.html
fast/borders/border-radius-constraints.html
fast/borders/border-radius-split-inline.html
fast/backgrounds/border-radius-split-background-image.html
fast/blockflow/box-shadow-vertical-rl.html
fast/backgrounds/repeat/negative-offset-repeat-transformed.html
fast/blockflow/box-shadow-horizontal-bt.html
fast/borders/border-radius-huge-assert.html
fast/box-shadow/border-radius-big.html
css2.1/t0805-c5517-brdr-s-00-c.html
http/tests/inspector/resource-tree/resource-tree-reload.html
fast/blockflow/box-shadow-vertical-lr.html
fast/backgrounds/border-radius-split-background.html

[Ben Wells]

Created attachment 102745 [details]
With updated test_expectations for tests which need rebaselining.

[Mike Reed]

Comment on attachment 102745 [details]
With updated test_expectations for tests which need rebaselining.

View in context: https://bugs.webkit.org/attachment.cgi?id=102745&action=review

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:396
> +    if (platformContext()->getXfermodeMode() == SkXfermode::kClear_Mode)

This check seems fragile, since a different call might have the mode set to clear, call clipOut(), and then change modes before drawing. In that case they will get unantialiased clip.

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:400
> +        platformContext()->makeGrContextCurrent();

I wonder if we should move the makecurrent call inside clipPathAntiAliased, just to cut down on all of the call-sites. Just a thought.

[Ben Wells]

(In reply to comment #5)
> (From update of attachment 102745 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102745&action=review
> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:396
> > +    if (platformContext()->getXfermodeMode() == SkXfermode::kClear_Mode)
> 
> This check seems fragile, since a different call might have the mode set to clear, call clipOut(), and then change modes before drawing. In that case they will get unantialiased clip.

I agree. Perhaps we can add a DCHECK or webkit equivalent that the mode isn't changed to clear mode while we have antialiased clip paths.

Another option is to have a clearModeClipOut in the graphics context. I'm worried this is polluting the graphics context with a special case.

Is there a way to make the antialiasing clip work with clear mode without major surgery?

> 
> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:400
> > +        platformContext()->makeGrContextCurrent();
> 
> I wonder if we should move the makecurrent call inside clipPathAntiAliased, just to cut down on all of the call-sites. Just a thought.

That's a good thought, it's easy to miss these.

[Mike Reed]

I think the notion just breaks the API model of GraphicsContext, that you can know the xfermode before you actually draw something (or in this case, before you draw everything). I think we should definitely consider a specialty function 

    void clearOut(Path);

We can always implement it in terms of the current pattern, but allow ports to perform this more efficiently if they wish (e.g. skia can definitely do this more efficiently in both raster and gpu).

[Ben Wells]

(In reply to comment #7)
> I think the notion just breaks the API model of GraphicsContext, that you can know the xfermode before you actually draw something (or in this case, before you draw everything). I think we should definitely consider a specialty function 
> 
>     void clearOut(Path);
> 
> We can always implement it in terms of the current pattern, but allow ports to perform this more efficiently if they wish (e.g. skia can definitely do this more efficiently in both raster and gpu).

How about fillOutside(Path), which if called with clear mode does clearOut? This feels more in harmony with the rest of the graphics context API and still solves our problem. Skia can implement this easily with its inverse path settings and the other implementations can do what they do already.

[Ben Wells]

(In reply to comment #8)
> (In reply to comment #7)
> > I think the notion just breaks the API model of GraphicsContext, that you can know the xfermode before you actually draw something (or in this case, before you draw everything). I think we should definitely consider a specialty function 
> > 
> >     void clearOut(Path);
> > 
> > We can always implement it in terms of the current pattern, but allow ports to perform this more efficiently if they wish (e.g. skia can definitely do this more efficiently in both raster and gpu).
> 
> How about fillOutside(Path), which if called with clear mode does clearOut? This feels more in harmony with the rest of the graphics context API and still solves our problem. Skia can implement this easily with its inverse path settings and the other implementations can do what they do already.

BTW either approach of clearOut or fillOutside will probably need path and rect versions.

[James Robinson]

I still wonder if simply removing the callers to clipOut() that set the mode to clear is a better option, since the only caller I know of is displayTransparencyElsewhere(), which is fundamentally broken.

[Ben Wells]

(In reply to comment #10)
> I still wonder if simply removing the callers to clipOut() that set the mode to clear is a better option, since the only caller I know of is displayTransparencyElsewhere(), which is fundamentally broken.

OK sounds like a good plan as that needs to change anyway, and won't be using clipOut long ter. I'll put this change on hold and look at the canvas compositing.

[Mike Reed]

(late to the party)

I'm fine with fillOut(rect or path)

[Ben Wells]

Created attachment 104775 [details]
Patch

[Ben Wells]

Comment on attachment 104775 [details]
Patch

Updated to remove hacky change to clipOut that is no longer needed (html canvas implementation no longer uses clipOut).

[James Robinson]

Looks surprisingly simple.  Stephen, Mike - any thoughts on this?

[Ben Wells]

Just a note: there are some problems with the new code with dashed and dotted borders. Details can be seen in bug 58711.

Dashed / dotted borders are a little worse in skia though, which seems to be due to differences in where the dashes / dots are placed. I'd like to look at that in another bug, as there are different approaches to fixing it and it is probably to as important as getting the new path in.

[Stephen White]

Comment on attachment 104775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104775&action=review

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:483
> +    if (!isPathSkiaSafe(getCTM(), path))

Just FYI, this is not necessary anymore.  ENSURE_VALUE_SAFETY_FOR_SKIA is not defined, since Skia doesn't blow up on large coordinates anymore (we should probably rip out all these calls at some point).

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:490
> +    if (antialiased)
> +        platformContext()->clipPathAntiAliased(path);
> +    else
> +        platformContext()->canvas()->clipPath(path);

Perhaps we should have rename PlatformContextSkia::clipPathAntiAliased() to PlatformContextSkia::clipPath(), and give it a parameter to specify antialiasing or not.  Could be done in a future CL.

[Stephen White]

Leaving for Mike to take a look.

[Mike Reed]

works for me

[James Robinson]

Comment on attachment 104775 [details]
Patch

Let's boogie, then.

[WebKit Review Bot]

Comment on attachment 104775 [details]
Patch

Rejecting attachment 104775 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:
ast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE
  svg/custom/inline-svg-in-xhtml.xml = IMAGE
  tables/mozilla/bugs/bug138725.html = IMAGE
  tables/mozilla_expected_failures/collapsing_borders/bug41262-5.html = IMAGE
  tables/mozilla_expected_failures/collapsing_borders/bug41262-6.html = IMAGE

Regressions: Unexpected image and text mismatch : (1)
  svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT



Full output: http://queues.webkit.org/results/9495602

[Ben Wells]

Created attachment 105292 [details]
Patch

[Ben Wells]

Comment on attachment 105292 [details]
Patch

Not sure what went wrong with the last patch. The test reported as failing fails for me with and without my change, but not when I try using git try.

I did find one extra test which needs rebaselining and have added it to the expectations.

[WebKit Review Bot]

Comment on attachment 105292 [details]
Patch

Clearing flags on attachment: 105292

Committed r93850: <http://trac.webkit.org/changeset/93850>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Pavel Podivilov]

I've rebaselined some fast/border/* tests that looked fine, see r93869. 

These ones looks strange however:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fborders%2FborderRadiusDashed06.html%2Cfast%2Fborders%2FborderRadiusDotted05.html&showExpectations=true.

[Aaron Colwell]

Are you planning on rebaselining the other tests as well? The media/ ones look like they should just be rebased.

(In reply to comment #26)
> I've rebaselined some fast/border/* tests that looked fine, see r93869. 
> 
> These ones looks strange however:
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fborders%2FborderRadiusDashed06.html%2Cfast%2Fborders%2FborderRadiusDotted05.html&showExpectations=true.

[Ben Wells]

(In reply to comment #27)
> Are you planning on rebaselining the other tests as well? The media/ ones look like they should just be rebased.
> 
> (In reply to comment #26)
> > I've rebaselined some fast/border/* tests that looked fine, see r93869. 
> > 
> > These ones looks strange however:
> > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fborders%2FborderRadiusDashed06.html%2Cfast%2Fborders%2FborderRadiusDotted05.html&showExpectations=true.

BorderRadiusDotted06 still has some issues, which are due to the problems noted on bug 58711. These problems manifest a bit differently on skia due to differences in where dashes and dots show.

I was planning on addressing those problems in a separate patch.

There are lots of tests that need rebaselining after this change, I was planning on doing this next week.

[David Levin]

(In reply to comment #28)
> There are lots of tests that need rebaselining after this change, I was planning on doing this next week.

If you know about these failures when you check in a change, it is ideal if you add them to test-expectation.txt so that gardeners don't have to sort it all out. :)

(Now, I'll also admit from experience I also know that sometimes we don't always realize when we will break things.)

[Ben Wells]

> (In reply to comment #28)
> > There are lots of tests that need rebaselining after this change, I was planning on doing this next week.
> 
> If you know about these failures when you check in a change, it is ideal if you add them to test-expectation.txt so that gardeners don't have to sort it all out. :)
> 
> (Now, I'll also admit from experience I also know that sometimes we don't always realize when we will break things.)

Sorry, I thought I had updated all the expectations, and the bots all looked green. This is some of the expectations I checked in, let me know if I'm not doing it right.

// Need new baselines due to border drawing now using the PATH_BASED_BORDER_RADIUS_DRAWING
// code path.
.....
BUGWK65583 LINUX WIN : fast/borders/borderRadiusDotted05.html = IMAGE