Drawing bounds should not be pixel-snapped.

Drawing bounds should not be pixel-snapped.

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

[jbroman, 2015-08-24 15:58:46]

jbroman@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[jbroman, 2015-08-24 15:58:46]

Noticed this while investigating underinvalidation issues exposed by partial
raster. I wasn't able to definitively tie this to any particular reproducible
issue, but it seems clearly wrong to me.

In particular, enclosingIntRect produces a rectangle that encloses the provided
LayoutRect, but pixelSnappedIntRect does not: it just snaps to the closest
corners on the pixel grid, which may make the rectangle /smaller/.

https://codereview.chromium.org/1313633003/diff/1/Source/core/paint/LayoutObj...
File Source/core/paint/LayoutObjectDrawingRecorder.h (right):

https://codereview.chromium.org/1313633003/diff/1/Source/core/paint/LayoutObj...
Source/core/paint/LayoutObjectDrawingRecorder.h:46: :
LayoutObjectDrawingRecorder(context, layoutObject, type, enclosingIntRect(clip))
{ }
Aside: This could arguably just be "clip", which does a conversion to FloatRect.
It's not clear to me why going to the integer pixel grid here is helpful.

So far I've just used enclosingIntRect because it seems like the smaller change.

[Xianzhu, 2015-08-24 16:14:33]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, schenney@chromium.org

[Xianzhu, 2015-08-24 16:14:33]

I also don't know why we are doing pixel-snapping. I guess it was inherited from
old painting code. It was added in https://codereview.chromium.org/963873002.

+chrishtr@ who is the author of the change;
+schenney@ who did some experiments about pixel snapping of cull rect.

[chrishtr, 2015-08-24 18:55:01]

The contents of the drawing are pixel snapped, why shouldn't the cull rect also
be?

[jbroman, 2015-08-24 19:09:58]

On 2015/08/24 at 18:55:01, chrishtr wrote:
> The contents of the drawing are pixel snapped, why shouldn't the cull rect
also be?

But the contents of drawings aren't pixel-snapped. As far as I know, we try to
snap certain types of elements to pixels, but, for instance, you can have
shadows with a subpixel offset, as in this page:

data:text/html,<div style="box-shadow: 1.4px 1.4px black; width: 100px; height:
100px">

Notably, this renders differently from a 1px-offset shadow, and also differently
from a 2px-offset shadow. Pixel-snapping the visual overflow rect of this box is
incorrect: it gives it a width of 101px which misses 0.4px of the shadow.

[chrishtr, 2015-08-24 23:13:07]

On 2015/08/24 at 19:09:58, jbroman wrote:
> On 2015/08/24 at 18:55:01, chrishtr wrote:
> > The contents of the drawing are pixel snapped, why shouldn't the cull rect
also be?
> 
> But the contents of drawings aren't pixel-snapped. As far as I know, we try to
snap certain types of elements to pixels, but, for instance, you can have
shadows with a subpixel offset, as in this page:
> 
> data:text/html,<div style="box-shadow: 1.4px 1.4px black; width: 100px;
height: 100px">
> 
> Notably, this renders differently from a 1px-offset shadow, and also
differently from a 2px-offset shadow. Pixel-snapping the visual overflow rect of
this box is incorrect: it gives it a width of 101px which misses 0.4px of the
shadow.

Good point. That is a real such case. I agree with you now that the current code
is wrong. Thanks for explaining.

Please add that as a layout test that draws incorrectly without your change
(e.g. by positioning it at the edge of a tile). And includes the invalidation
rect in the output.

[jbroman, 2015-08-25 17:54:20]

On 2015/08/24 at 23:13:07, chrishtr wrote:
> On 2015/08/24 at 19:09:58, jbroman wrote:
> > On 2015/08/24 at 18:55:01, chrishtr wrote:
> > > The contents of the drawing are pixel snapped, why shouldn't the cull rect
also be?
> > 
> > But the contents of drawings aren't pixel-snapped. As far as I know, we try
to snap certain types of elements to pixels, but, for instance, you can have
shadows with a subpixel offset, as in this page:
> > 
> > data:text/html,<div style="box-shadow: 1.4px 1.4px black; width: 100px;
height: 100px">
> > 
> > Notably, this renders differently from a 1px-offset shadow, and also
differently from a 2px-offset shadow. Pixel-snapping the visual overflow rect of
this box is incorrect: it gives it a width of 101px which misses 0.4px of the
shadow.
> 
> Good point. That is a real such case. I agree with you now that the current
code is wrong. Thanks for explaining.
> 
> Please add that as a layout test that draws incorrectly without your change
(e.g. by positioning it at the edge of a tile). And includes the invalidation
rect in the output.

Heh, unfortunately the tiling-invalidation code adds 1px to the rect, so I can't
manage to actually make cc draw wrong as a result of this today (cc will, if you
ask it to invalidate exactly one tile, invalidate up to nine of them).

(A 1px slop in the partial raster code would presumably have the same effect,
but it's probably not great to be covering up our bugs like this.)

I can probably write a unit test that captures this, though.

[chrishtr, 2015-08-26 17:11:46]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-08-26 17:11:46]

lgtm

[commit-bot: I haz the power, 2015-08-26 17:11:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313633003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313633003/60001

[commit-bot: I haz the power, 2015-08-26 17:51:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-26 17:51:00]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbroman, 2015-08-26 19:12:36]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2015-08-26 19:12:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313633003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313633003/60001

[commit-bot: I haz the power, 2015-08-26 19:47:47]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201243