Bug 28820 - Chromium: do anti-aliased path clipping
Summary: Chromium: do anti-aliased path clipping
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-28 14:27 PDT by Adam Langley
Modified: 2009-10-15 15:02 PDT (History)
6 users (show)

See Also:


Attachments
patch (8.27 KB, patch)
2009-08-28 14:32 PDT, Adam Langley
agl: review-
Details | Formatted Diff | Diff
patch (14.44 KB, patch)
2009-09-03 16:11 PDT, Adam Langley
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Langley 2009-08-28 14:27:21 PDT
Currently, Skia clip paths are 1-bit. This has resulted in much sadness because our rounded corners look ugly.

This patch makes our path clipping anti-aliased.

http://code.google.com/p/chromium/issues/detail?id=5927
Comment 1 Adam Langley 2009-08-28 14:32:31 PDT
Created attachment 38754 [details]
patch
Comment 2 Eric Seidel (no email) 2009-08-31 03:22:48 PDT
Comment on attachment 38754 [details]
patch

clipAntiAliasPath seems like a strange name to me.  clipPathAndAntiAlias?  Not sure.
Comment 3 Brett Wilson (Google) 2009-09-02 13:04:35 PDT
Do you know why the imageBufferClip not used on Mac, but yours path thing is? I would have expected them to go with one-another.


I'm now remembering a more efficient way to do the clipping without adding the extra layer. Sorry I forgot this from before (this suggestion is from Mike):

Instead of drawing the image inside the path, clear the area outside the path. Use the fill type on SkPath to draw "outside" the path with the transfer mode of clear. Then you can just paint the original (now modified) bitmap to the destination.

It also means you don't need to save the layer for each clipping path. You can just save all the paths, erase the areas outside of each of them from the single extra layer you made, and then composite that down.
Comment 4 Adam Langley 2009-09-02 15:26:32 PDT
Comment on attachment 38754 [details]
patch

After talking with Dean, I'm not sure that this is the correct solution at all. I'm going to try another approach.
Comment 5 Adam Langley 2009-09-03 16:11:54 PDT
Created attachment 39017 [details]
patch
Comment 6 Eric Seidel (no email) 2009-09-03 16:16:52 PDT
Do we have bugs filed about CG/Cairo's "bodged" clipping?
Comment 7 Adam Langley 2009-09-03 16:21:04 PDT
Ojan suggested that we be good citizens and file such bugs, but I've not done so yet. I honestly doubt that they would do anything about it. They would hit the same problems that we would hit with <canvas> and anti-aliased clipping. They could do 1-bit drawing at 4x supersampling and solve all the issues, but I suspect the the performance hit of doing so would be too much.
Comment 8 Eric Seidel (no email) 2009-09-03 16:29:15 PDT
incompatible canvas implementations = teh suck.
Comment 9 Adam Langley 2009-09-03 16:41:41 PDT
I think you're taking the word 'bodge' as too pejorative. The correct application of sensible bodges is called 'engineering'. Doing anti-aliased clipping as you draw is a bodge: you get the wrong answer. But in this case it's a sensible bodge. I rather wish Skia would support it as this point.

<canvas> not having anti-aliased clipping is unfortunate for some, and a requirement for others. I think it's just a manifestation of anything complex in early stages.
Comment 10 Dean McNamee 2009-09-04 03:06:13 PDT
(In reply to comment #9)
> I think you're taking the word 'bodge' as too pejorative. The correct
> application of sensible bodges is called 'engineering'. Doing anti-aliased
> clipping as you draw is a bodge: you get the wrong answer. But in this case
> it's a sensible bodge. I rather wish Skia would support it as this point.
> 
> <canvas> not having anti-aliased clipping is unfortunate for some, and a
> requirement for others. I think it's just a manifestation of anything complex
> in early stages.

Looks like I don't have permission to review the patch.  Anyway, some small feedback.

> +
> +        // This clip function is used only by <canvas> code. It allows
> +        // implementations to handle clipping on the canvas differently since
> +        // the disipline is different.

discipline ?

> +    // If we are currently tracking any anti-alias clip paths, then we already
> +    // have a layer in place and don't need to add another.
> +    bool haveLayerOutstanding = m_state->m_antiAliasClipPaths.size();

How about !isEmpty() ?  Actually, you could flip the logic to noOustandingLayers, and invert the if() also.  You could also just do the append 
and check if size == 1.


> +    for (size_t i = paths.size() - 1; i < paths.size(); --i) {
> +        paths[i].setFillType(SkPath::kInverseWinding_FillType);

Cool man...
Comment 11 Adam Langley 2009-09-15 11:36:43 PDT
Comment on attachment 39017 [details]
patch

(Giving up on Darin)
Comment 12 Eric Seidel (no email) 2009-10-01 15:03:18 PDT
Comment on attachment 39017 [details]
patch

 95     void clipPathAntiAliased(const SkPath& path);
"path" shouldn't be there.

But in general this looks fine.
Comment 13 Adam Langley 2009-10-15 15:02:56 PDT
r49641