cc: Fix low-res impl-side painting artifacts

cc: Fix low-res impl-side painting artifacts

Instead of clipping cc::Picture on recorded pixel boundaries (which may be a
partial pixel in the final rastered content if content scale < 1), clip only on
integral pixels in the final rastered output.  This requires recording a small
buffer of pixels around every picture, specified via a min contents scale.

NOTRY=true
R=nduca@chromium.org
BUG=167590
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175542

[danakj, 2013-01-05 00:04:58]

Where is the contents_scale being capped by the min_contents_scale? I didn't see
that... I musta missed it?

https://codereview.chromium.org/11678003/diff/1011/cc/picture.cc
File cc/picture.cc (right):

https://codereview.chromium.org/11678003/diff/1011/cc/picture.cc#newcode100
cc/picture.cc:100: canvas->clipRect(gfx::RectToSkRect(content_rect));
I find this ordering weird..

Is the scale the scale used to get to the content_rect? Should it be clipping
after the scale is applied?

https://codereview.chromium.org/11678003/diff/1011/cc/picture.h
File cc/picture.h (right):

https://codereview.chromium.org/11678003/diff/1011/cc/picture.h#newcode43
cc/picture.h:43: // Assumes contentsScale have already been applied.
Is this comment wrong now?

https://codereview.chromium.org/11678003/diff/1011/cc/picture_layer.h
File cc/picture_layer.h (right):

https://codereview.chromium.org/11678003/diff/1011/cc/picture_layer.h#newcode20
cc/picture_layer.h:20: public:
heh, should be 1 space in front of these.

https://codereview.chromium.org/11678003/diff/1011/cc/picture_layer.h#newcode29
cc/picture_layer.h:29: virtual void setLayerTreeHost(LayerTreeHost*) OVERRIDE;
variable name here please (cr style)

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile.cc
File cc/picture_pile.cc (right):

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile.cc#newcode116
cc/picture_pile.cc:116: picture_rect = gfx::Rect(gfx::Rect(size_));
Yo dawg, I heard you like Rects.

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile_impl.cc
File cc/picture_pile_impl.cc (right):

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile_impl.cc#ne...
cc/picture_pile_impl.cc:65: canvas->clipRect(gfx::RectToSkRect(content_rect));
Is this right after the translate() ? Or should this clip rect be at 0,0?

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile_impl.cc#ne...
cc/picture_pile_impl.cc:67: gfx::Rect layer_rect = gfx::ToEnclosingRect(
Why enclosing? We do enclosing when going from layer to content space (ie
Layer::layerRectToContentRect()), so isn't this supposed to be enclosed?

[enne (OOO), 2013-01-06 03:57:18]

You're right that there's no enforcement of minimum.  I added
https://codereview.chromium.org/11777008/ which allows a LayerImpl to modify its
content scale and that allows the content scale to be more easily capped at its
minimum in this patch.

https://codereview.chromium.org/11678003/diff/1011/cc/picture.cc
File cc/picture.cc (right):

https://codereview.chromium.org/11678003/diff/1011/cc/picture.cc#newcode100
cc/picture.cc:100: canvas->clipRect(gfx::RectToSkRect(content_rect));
On 2013/01/05 00:04:58, danakj wrote:
> I find this ordering weird..
> 
> Is the scale the scale used to get to the content_rect? Should it be clipping
> after the scale is applied?

These are applied to the canvas, so any recorded op played back into that canvas
gets translated, scaled, then clipped, in that order.

https://codereview.chromium.org/11678003/diff/1011/cc/picture.h
File cc/picture.h (right):

https://codereview.chromium.org/11678003/diff/1011/cc/picture.h#newcode43
cc/picture.h:43: // Assumes contentsScale have already been applied.
On 2013/01/05 00:04:58, danakj wrote:
> Is this comment wrong now?

Good catch.  Done.

https://codereview.chromium.org/11678003/diff/1011/cc/picture_layer.h
File cc/picture_layer.h (right):

https://codereview.chromium.org/11678003/diff/1011/cc/picture_layer.h#newcode20
cc/picture_layer.h:20: public:
On 2013/01/05 00:04:58, danakj wrote:
> heh, should be 1 space in front of these.

Done.

https://codereview.chromium.org/11678003/diff/1011/cc/picture_layer.h#newcode29
cc/picture_layer.h:29: virtual void setLayerTreeHost(LayerTreeHost*) OVERRIDE;
On 2013/01/05 00:04:58, danakj wrote:
> variable name here please (cr style)

Done.

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile.cc
File cc/picture_pile.cc (right):

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile.cc#newcode116
cc/picture_pile.cc:116: picture_rect = gfx::Rect(gfx::Rect(size_));
On 2013/01/05 00:04:58, danakj wrote:
> Yo dawg, I heard you like Rects.

Oops.

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile_impl.cc
File cc/picture_pile_impl.cc (right):

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile_impl.cc#ne...
cc/picture_pile_impl.cc:65: canvas->clipRect(gfx::RectToSkRect(content_rect));
On 2013/01/05 00:04:58, danakj wrote:
> Is this right after the translate() ? Or should this clip rect be at 0,0?

Hmm.  This should go before.  Done.

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile_impl.cc#ne...
cc/picture_pile_impl.cc:67: gfx::Rect layer_rect = gfx::ToEnclosingRect(
On 2013/01/05 00:04:58, danakj wrote:
> Why enclosing? We do enclosing when going from layer to content space (ie
> Layer::layerRectToContentRect()), so isn't this supposed to be enclosed?

Good question.  I think what I should do is just see if content_clip intersects
with content_rect and not even calculate layer_rect.

[enne (OOO), 2013-01-06 04:30:31]

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile_impl.cc
File cc/picture_pile_impl.cc (right):

https://codereview.chromium.org/11678003/diff/1011/cc/picture_pile_impl.cc#ne...
cc/picture_pile_impl.cc:65: canvas->clipRect(gfx::RectToSkRect(content_rect));
On 2013/01/06 03:57:18, enne wrote:
> On 2013/01/05 00:04:58, danakj wrote:
> > Is this right after the translate() ? Or should this clip rect be at 0,0?
> 
> Hmm.  This should go before.  Done.

I was right the first time, sorry.

The translation is to go from canvas space to content space.  So that things
drawn into content space get correctly translated back to the 0, 0, w, h of the
canvas.  So, the clip comes afterwards.

[danakj, 2013-01-07 20:24:02]

https://codereview.chromium.org/11678003/diff/9013/cc/layer_tree_settings.cc
File cc/layer_tree_settings.cc (right):

https://codereview.chromium.org/11678003/diff/9013/cc/layer_tree_settings.cc#...
cc/layer_tree_settings.cc:30: , minimumContentsScale(0.0625)
0.0625f

https://codereview.chromium.org/11678003/diff/9013/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/11678003/diff/9013/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:183: float max_contents_scale = min_contents_scale;
ah, i see, cool

https://codereview.chromium.org/11678003/diff/9013/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:257: if (contents_scale <
layerTreeImpl()->settings().minimumContentsScale)
should this be a dcheck? (having trouble seeing who calls this from grep..)

[danakj, 2013-01-07 20:25:42]

LGTM

[commit-bot: I haz the power, 2013-01-08 00:22:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11678003/9013

[commit-bot: I haz the power, 2013-01-08 00:35:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11678003/8004

[commit-bot: I haz the power, 2013-01-08 16:41:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11678003/8004

[commit-bot: I haz the power, 2013-01-08 17:11:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11678003/8004

[commit-bot: I haz the power, 2013-01-08 17:12:45]

Change committed as 175542