Bug 65583 - Use PATH_BASED_BORDER_RADIUS_DRAWING for skia
Summary: Use PATH_BASED_BORDER_RADIUS_DRAWING for skia
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ben Wells
URL:
Keywords:
Depends on: 66036
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-02 16:55 PDT by Ben Wells
Modified: 2011-08-28 23:03 PDT (History)
13 users (show)

See Also:


Attachments
Patch (6.50 KB, patch)
2011-08-02 18:55 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
With updated test_expectations for tests which need rebaselining. (12.06 KB, patch)
2011-08-03 00:16 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch (16.08 KB, patch)
2011-08-22 17:38 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch (16.15 KB, patch)
2011-08-25 19:08 PDT, Ben Wells
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Wells 2011-08-02 16:55:58 PDT
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.
Comment 1 Ben Wells 2011-08-02 18:55:32 PDT
Created attachment 102728 [details]
Patch
Comment 2 Ben Wells 2011-08-02 18:58:48 PDT
This change doesn't have updated test expectations yet, I need to check a few more tests first.
Comment 3 WebKit Review Bot 2011-08-02 19:26:22 PDT
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
Comment 4 Ben Wells 2011-08-03 00:16:24 PDT
Created attachment 102745 [details]
With updated test_expectations for tests which need rebaselining.
Comment 5 Mike Reed 2011-08-03 05:28:31 PDT
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.
Comment 6 Ben Wells 2011-08-03 05:59:35 PDT
(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.
Comment 7 Mike Reed 2011-08-03 06:59:06 PDT
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).
Comment 8 Ben Wells 2011-08-03 15:18:43 PDT
(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.
Comment 9 Ben Wells 2011-08-03 15:26:50 PDT
(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.
Comment 10 James Robinson 2011-08-03 15:40:15 PDT
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.
Comment 11 Ben Wells 2011-08-03 17:31:55 PDT
(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.
Comment 12 Mike Reed 2011-08-04 06:10:59 PDT
(late to the party)

I'm fine with fillOut(rect or path)
Comment 13 Ben Wells 2011-08-22 17:38:40 PDT
Created attachment 104775 [details]
Patch
Comment 14 Ben Wells 2011-08-22 17:40:06 PDT
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).
Comment 15 James Robinson 2011-08-22 17:41:25 PDT
Looks surprisingly simple.  Stephen, Mike - any thoughts on this?
Comment 16 Ben Wells 2011-08-22 17:47:54 PDT
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.
Comment 17 Stephen White 2011-08-25 08:11:41 PDT
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.
Comment 18 Stephen White 2011-08-25 09:59:46 PDT
Leaving for Mike to take a look.
Comment 19 Mike Reed 2011-08-25 10:14:34 PDT
works for me
Comment 20 James Robinson 2011-08-25 10:15:25 PDT
Comment on attachment 104775 [details]
Patch

Let's boogie, then.
Comment 21 WebKit Review Bot 2011-08-25 13:46:06 PDT
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
Comment 22 Ben Wells 2011-08-25 19:08:43 PDT
Created attachment 105292 [details]
Patch
Comment 23 Ben Wells 2011-08-25 19:10:38 PDT
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.
Comment 24 WebKit Review Bot 2011-08-25 21:27:15 PDT
Comment on attachment 105292 [details]
Patch

Clearing flags on attachment: 105292

Committed r93850: <http://trac.webkit.org/changeset/93850>
Comment 25 WebKit Review Bot 2011-08-25 21:27:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Pavel Podivilov 2011-08-26 10:35:33 PDT
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.
Comment 27 Aaron Colwell 2011-08-26 13:31:40 PDT
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.
Comment 28 Ben Wells 2011-08-26 15:28:03 PDT
(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.
Comment 29 David Levin 2011-08-26 15:30:54 PDT
(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.)
Comment 30 Ben Wells 2011-08-26 15:56:54 PDT
> (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