Bug 28820 – Chromium: do anti-aliased path clipping

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Adam Langley]

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

[Adam Langley]

Created attachment 38754 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 38754 [details]
patch

clipAntiAliasPath seems like a strange name to me.  clipPathAndAntiAlias?  Not sure.

[Brett Wilson (Google)]

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.

[Adam Langley]

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.

[Adam Langley]

Created attachment 39017 [details]
patch

[Eric Seidel (no email)]

Do we have bugs filed about CG/Cairo's "bodged" clipping?

[Adam Langley]

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.

[Eric Seidel (no email)]

incompatible canvas implementations = teh suck.

[Adam Langley]

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.

[Dean McNamee]

(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...

[Adam Langley]

Comment on attachment 39017 [details]
patch

(Giving up on Darin)

[Eric Seidel (no email)]

Comment on attachment 39017 [details]
patch

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

But in general this looks fine.

[Adam Langley]

r49641