Large canvas does not honor containing div's border radius

Large canvas does not honor containing div's border radius.

BUG=137818
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158258

[irosca, 2013-06-09 12:21:02]

The border-radius css property does not have any effect on canvas2d > 256px.
This type of canvas gets its own compositing layer and all the calls issued from
JS are sent to the GPU. In this case border-radius is omitted.
When canvas2d is software rendered, the clipping with rounded corners is done in
RenderReplaced(RenderBox)::paint, but this function does nothing for accelerated
canvas2d objects.

The patch I uploaded sets rounded clipping on the accelerated ImageBuffer when
this buffer is created or width/height are changed. All the canvas 2d JS calls
will be clipped to the rounded rect.

The patch is not complete, but I'd like to see other opinions on this, is this
the right way to move forward or not?

[jamesr, 2013-06-10 17:26:23]

Interesting.  Can you construct some test cases showing the issue?

I don't know if we support soft clipping for cases like this (and if we did I'd
bet it'd be very expensive).  Maybe we should handle this with a mask?  Stephen
would probably know better.

[Stephen White, 2013-06-10 17:44:22]

On 2013/06/10 17:26:23, jamesr wrote:
> Interesting.  Can you construct some test cases showing the issue?

There are some test cases on the bug that should probably be turned into a
fast/canvas test. Ideally a script test (ie., no pixel results), or a reftest
failing that.

> I don't know if we support soft clipping for cases like this (and if we did
I'd
> bet it'd be very expensive).

I'm not sure if clipRoundedRect() does correct soft clipping, but I bet
robertphillips@ would know.

I'm also not sure if this plays well with any clipping the canvas itself may do
(e.g, whether resetClip() takes us back to the rounded-rect clip.)  We should
probably have some test cases for that as well.

> Maybe we should handle this with a mask?  Stephen
> would probably know better.

It seems like we should handle it in the compositor, if possible, but I'm not
sure how the performance would compare.  Presumably this affects WebGL as well,
perhaps also <video>, so there may be some opportunity for code sharing.

[robertphillips, 2013-06-11 14:50:55]

>  I don't know if we support soft clipping for cases like this (and if we
>> did I'd
>
>  bet it'd be very expensive).
>>
>
> I'm not sure if clipRoundedRect() does correct soft clipping, but I bet
> robertphillips@ would know.
>

Soft clipping is currently disabled everywhere in Chrome (due to speed
issues).

Note that Skia isn't getting the rounded rect clip at all afaict. The .skp
file I captured from the canvas only contains the rect draw for both the
software and gpu cases.

[Stephen White, 2013-06-11 14:54:08]

On 2013/06/11 14:50:55, robertphillips wrote:
> >  I don't know if we support soft clipping for cases like this (and if we
> >> did I'd
> >
> >  bet it'd be very expensive).
> >>
> >
> > I'm not sure if clipRoundedRect() does correct soft clipping, but I bet
> > robertphillips@ would know.
> >
> 
> Soft clipping is currently disabled everywhere in Chrome (due to speed
> issues).
> 
> Note that Skia isn't getting the rounded rect clip at all afaict. The .skp
> file I captured from the canvas only contains the rect draw for both the
> software and gpu cases.

Perhaps a stupid question, but did you capture the picture with the patch
applied? Without it, I think we're completely ignoring the border radius (ie. no
rounded rect at all).

[robertphillips, 2013-06-11 16:45:18]

I have (now) applied the patch and the clip's still do not seem to be
making it down to Skia.


On Tue, Jun 11, 2013 at 10:54 AM, <senorblanco@chromium.org> wrote:

> On 2013/06/11 14:50:55, robertphillips wrote:
>
>> >  I don't know if we support soft clipping for cases like this (and if we
>> >> did I'd
>> >
>> >  bet it'd be very expensive).
>> >>
>> >
>> > I'm not sure if clipRoundedRect() does correct soft clipping, but I bet
>> > robertphillips@ would know.
>> >
>>
>
>  Soft clipping is currently disabled everywhere in Chrome (due to speed
>> issues).
>>
>
>  Note that Skia isn't getting the rounded rect clip at all afaict. The .skp
>> file I captured from the canvas only contains the rect draw for both the
>> software and gpu cases.
>>
>
> Perhaps a stupid question, but did you capture the picture with the patch
> applied? Without it, I think we're completely ignoring the border radius
> (ie. no
> rounded rect at all).
>
>
https://codereview.chromium.**org/16688004/<https://codereview.chromium.org/1...
>

[rosca, 2013-06-15 20:54:14]

On 2013/06/11 16:45:18, robertphillips wrote:
> I have (now) applied the patch and the clip's still do not seem to be
> making it down to Skia.

Thanks for all your comments. I uploaded a test showing this issue.

There are 2 layers behind an accelerated (> 256x256px) canvas element having box
decorations (box-shadow, border):
1) cc::PictureLayerImpl, which is responsible for drawing box decorations. This
layer is aware of border-radius and draws corners correctly. Also, this layer
shows up in .skp files with all the details like rounded clip rect.
2) cc::TextureLayerImpl (child of 1), which draws de actual canvas content. The
skia object used for painting inside the canvas has a Ganesh backend, which
generates GL commands instead of software rasterizing into a bitmap (immediate
mode). This is why it does not appear in .skp files. I'm not sure if
clipRoundedRect on this object works in software mode or it's sent to GPU. If it
goes to GPU, it should not be expensive. Is there another way to clip a layer
being painted with the immediate mode?

I've tried to apply a black mask on the canvas object, and it sets a new layer
over the two canvas layers. It masks everything around the border (the outer
rounded rect). The problem is that canvas still draws over its border and the
canvas shadow is cut down by the mask. Is there a way to apply the mask just for
the canvas layer (TextureLayerImpl, Skia with Ganesh backend)?

[rosca, 2013-06-18 21:37:50]

I uploaded a set of files implementing rounded corners clipping using a mask.
It should work for all the layers having external contents (canvas 2d, 3d,
video, accelerated embedded objects). The two layout tests show rounded borders
working with canvas2d and video.

There are some other remaining things to be done like fixing rounded corners on
selection, zoom out for masks (even css masks), ...

Please review this patch. It affects only accelerated canvas, video, embedded
elements having a border-radius.

[jamesr, 2013-06-18 21:41:53]

Borders can be far more complicated than just a rounded rectangle.  Could you
add tests for the more interesting cases?  Take a look at fast/borders/* for
inspiration.

[Justin Novosad, 2013-06-19 17:47:45]

On 2013/06/18 21:41:53, jamesr wrote:
> Borders can be far more complicated than just a rounded rectangle.  Could you
> add tests for the more interesting cases?  Take a look at fast/borders/* for
> inspiration.

You should probably also cover padding and margin.

[alokp, 2013-06-19 21:59:11]

On 2013/06/19 17:47:45, junov wrote:
> On 2013/06/18 21:41:53, jamesr wrote:
> > Borders can be far more complicated than just a rounded rectangle.  Could
you
> > add tests for the more interesting cases?  Take a look at fast/borders/* for
> > inspiration.
> 
> You should probably also cover padding and margin.

A similar solution is needed to fix
https://code.google.com/p/chromium/issues/detail?id=157218.

The clipping div needs to push a border mask. It may be possible to generalize
this to cover both cases.

[irosca, 2013-06-19 22:56:08]

On 2013/06/19 21:59:11, Alok Priyadarshi wrote:
> On 2013/06/19 17:47:45, junov wrote:
> > On 2013/06/18 21:41:53, jamesr wrote:
> > > Borders can be far more complicated than just a rounded rectangle.  Could
> you
> > > add tests for the more interesting cases?  Take a look at fast/borders/*
for
> > > inspiration.

I've just applied all kind of borders to an accelerated canvas with rounded
corners and nothing seems to get it surprised, comparing to a non-accelerated
canvas. This patch does not touch the layer containing borders, shadow,
background etc, therefore - no bad thing should happen there. It just applies a
mask over the contents area, not allowing it to draw corners outside the inner
border rect. The inner border rect can be either a regular rectangle, or a
rounded rectangle, disregarding various border styles.
The rounded border mask is painted in the same renderLayer instance as the
background & borders are painted, so this patch doesn't bring any new thing in
how border metrics are computed.
I will add some more tests with different border radiuses on each corner. Do you
think other tests might raise some issues?

> > 
> > You should probably also cover padding and margin.
> 
Yes, this is not covered yet, it is visible on a video which doesn't scale to
the element's size. I will add a test for this too. First I would like to figure
out if this solution with masking is correct.

> A similar solution is needed to fix
> https://code.google.com/p/chromium/issues/detail?id=157218.
> 
> The clipping div needs to push a border mask. It may be possible to generalize
> this to cover both cases.
Thanks, I will take this into account.

[rosca, 2013-07-02 17:11:12]

Hi!

I uploaded a new set of files fixed the issues with padding and borders and
adding several tests covering accelerated canvas and video.
Some of the canvas tests have been done using references to the non-accelerated
canvas version of test. The other canvas tests had some minor issues regarding
anti-aliasing for rounded corners and I uploaded png files for them.

Please review this patch.

[jamesr, 2013-07-03 21:34:00]

Thanks!  The compositor interactions seem quite sane.  I don't feel quite
qualified to check that the generated mask is correct - maybe Stephen Chenney or
one of his merry band could give that a look?

https://codereview.chromium.org/16688004/diff/17001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/16688004/diff/17001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:88: &&
(isAcceleratedCanvas(renderer)
i'm not sure i understand the purpose of these checks. if an object has a border
radius, why does it matter what type of content it is for the purpose of
clipping? we should clip canvas, plugins, videos and any other sort of
composited content the same way, shouldn't we?

[Stephen White, 2013-07-03 21:37:37]

+junov, in case he has any comments

[rosca, 2013-07-04 07:51:50]

Thanks, James!

https://codereview.chromium.org/16688004/diff/17001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/16688004/diff/17001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:88: &&
(isAcceleratedCanvas(renderer)
On 2013/07/03 21:34:01, jamesr wrote:
> i'm not sure i understand the purpose of these checks. if an object has a
border
> radius, why does it matter what type of content it is for the purpose of
> clipping? we should clip canvas, plugins, videos and any other sort of
> composited content the same way, shouldn't we?

This is the first case, where the content being clipped is rendered from an
external accelerated source (m_graphicsLayer->setContentsTo[Canvas|Media]). The
mask is set up to the external WebLayer through m_graphicsLayer.
The second case is when we should clip regular compositing layers as described
in http://crbug.com/157218. We can use the same mask for this case, but it
should be set up on a different layer (I think it's m_childContainmentLayer).
I would prefer to create another review for this case.

[rosca, 2013-07-15 22:56:45]

I rebased the patch and I've made some changes on rendering side (not yet
reviewed), so that the patch for http://crbug.com/157218 will not require
changes there.
Please review.

[Justin Novosad, 2013-09-11 20:39:38]

On 2013/07/15 22:56:45, Rosca wrote:
> I rebased the patch and I've made some changes on rendering side (not yet
> reviewed), so that the patch for http://crbug.com/157218 will not require
> changes there.
> Please review.

There has been no traffic on this review in a while.  Is this still active? 
Would be great to get this fix landed.

[jamesr, 2013-09-11 20:46:24]

On 2013/09/11 20:39:38, junov wrote:
> On 2013/07/15 22:56:45, Rosca wrote:
> > I rebased the patch and I've made some changes on rendering side (not yet
> > reviewed), so that the patch for http://crbug.com/157218 will not require
> > changes there.
> > Please review.
> 
> There has been no traffic on this review in a while.  Is this still active? 
> Would be great to get this fix landed.

Well, the patch is waiting for you or Stephen - isn't it?

[Justin Novosad, 2013-09-12 20:30:27]

On 2013/09/11 20:46:24, jamesr wrote:
> On 2013/09/11 20:39:38, junov wrote:
> > On 2013/07/15 22:56:45, Rosca wrote:
> > > I rebased the patch and I've made some changes on rendering side (not yet
> > > reviewed), so that the patch for http://crbug.com/157218 will not require
> > > changes there.
> > > Please review.
> > 
> > There has been no traffic on this review in a while.  Is this still active? 
> > Would be great to get this fix landed.
> 
> Well, the patch is waiting for you or Stephen - isn't it?

I already said what I had to say, and I am not an owner.  Stephen?

[Stephen White, 2013-09-12 20:36:37]

On 2013/09/12 20:30:27, junov wrote:
> On 2013/09/11 20:46:24, jamesr wrote:
> > On 2013/09/11 20:39:38, junov wrote:
> > > On 2013/07/15 22:56:45, Rosca wrote:
> > > > I rebased the patch and I've made some changes on rendering side (not
yet
> > > > reviewed), so that the patch for http://crbug.com/157218 will not
require
> > > > changes there.
> > > > Please review.
> > > 
> > > There has been no traffic on this review in a while.  Is this still
active? 
> > > Would be great to get this fix landed.
> > 
> > Well, the patch is waiting for you or Stephen - isn't it?
> 
> I already said what I had to say, and I am not an owner.  Stephen?

LGTM, although it's mostly a rubber-stamp. This is not an area of Blink I'm
particularly familiar with.

[Justin Novosad, 2013-09-12 20:40:09]

On 2013/09/12 20:36:37, Stephen White wrote:
> 
> LGTM, although it's mostly a rubber-stamp. This is not an area of Blink I'm
> particularly familiar with.

Then we could upload the patch to WebKit to see what their reviewers say ;-)

[Justin Novosad, 2013-09-18 19:08:22]

On 2013/09/12 20:40:09, junov wrote:
> On 2013/09/12 20:36:37, Stephen White wrote:
> > 
> > LGTM, although it's mostly a rubber-stamp. This is not an area of Blink I'm
> > particularly familiar with.
> 
> Then we could upload the patch to WebKit to see what their reviewers say ;-)

No response...  I can rebase and commit rosca's behalf.  Any objections?

[commit-bot: I haz the power, 2013-09-18 20:54:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rosca@adobe.com/16688004/73001

[commit-bot: I haz the power, 2013-09-18 21:21:34]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[Justin Novosad, 2013-09-19 13:43:59]

Patch needs approval from an OWNER of Source/web
jamesr?

[rosca, 2013-09-19 13:52:32]

On 2013/09/18 19:08:22, junov wrote:
> On 2013/09/12 20:40:09, junov wrote:
> > On 2013/09/12 20:36:37, Stephen White wrote:
> > > 
> > > LGTM, although it's mostly a rubber-stamp. This is not an area of Blink
I'm
> > > particularly familiar with.
> > 
> > Then we could upload the patch to WebKit to see what their reviewers say ;-)
> 
> No response...  I can rebase and commit rosca's behalf.  Any objections?

Thanks junov. I'm back. I will apply the patch on my side to see if works (you
probably did that already). Tell me if there is something I can do. I will try
to upload this fix to WebKit soon.

[Justin Novosad, 2013-09-19 14:49:19]

On 2013/09/19 13:52:32, Rosca wrote:
> On 2013/09/18 19:08:22, junov wrote:
> > On 2013/09/12 20:40:09, junov wrote:
> > > On 2013/09/12 20:36:37, Stephen White wrote:
> > > > 
> > > > LGTM, although it's mostly a rubber-stamp. This is not an area of Blink
> I'm
> > > > particularly familiar with.
> > > 
> > > Then we could upload the patch to WebKit to see what their reviewers say
;-)
> > 
> > No response...  I can rebase and commit rosca's behalf.  Any objections?
> 
> Thanks junov. I'm back. I will apply the patch on my side to see if works (you
> probably did that already). Tell me if there is something I can do. I will try
> to upload this fix to WebKit soon.

Hey, welcome back!  There have been a few refactors in files touched by this
change so the rebase required a bit of manual labor.  I recommend downloading
the latest patch set and applying it locally, rather than you going through the
same manual rebase I had to do.  BTW, there is no need to upload layout test
baselines to Blink anymore, so you don't have to worry about patching-in the png
files.  You just mark the test expectations as "NeedsRebaseline" and the
rebaseline bot will commit baselines for all platforms on your behalf.

[jamesr, 2013-09-23 19:27:34]

Source/web/ lgtm

[commit-bot: I haz the power, 2013-09-24 08:05:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rosca@adobe.com/16688004/73001

[commit-bot: I haz the power, 2013-09-24 09:46:06]

Change committed as 158258