cc : Snap texture layers to pixel boundary

cc : Snap texture layers to pixel boundary

This is to avoid blurring caused by fractional translation.

BUG=629522
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/2a1718bffdb1d3c9538731498382e9aafd886ceb
Cr-Commit-Position: refs/heads/master@{#427859}

[jaydasika, 2016-10-26 19:42:24]

Description was changed from

==========
cc : Snap texture layers to pixel boundary

This is to avoid blurring caused by fractional translation.

BUG=629522
==========

to

==========
cc : Snap texture layers to pixel boundary

This is to avoid blurring caused by fractional translation.

BUG=629522
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jaydasika, 2016-10-26 19:46:44]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, enne@chromium.org

[jaydasika, 2016-10-26 19:46:44]

PTAL

[jaydasika, 2016-10-26 20:21:09]

The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 20:21:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 20:24:32]

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

[commit-bot: I haz the power, 2016-10-26 20:24:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[enne (OOO), 2016-10-26 20:32:33]

lgtm with a few small nits.  You probably want ajuma to review or at least ack
this, as I'm less familiar with property tree code these days.

https://codereview.chromium.org/2451273003/diff/1/cc/trees/layer_tree_host_co...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/2451273003/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:3548:
fractional_translate.Translate(10.5f, 10.5f);
Can you use a translation in of these components where x and 1-x are not equal
to make sure that there's not a sign error somewhere?

https://codereview.chromium.org/2451273003/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:3563:
EXPECT_TRANSFORMATION_MATRIX_EQ(child_ptr->ScreenSpaceTransform(),
Could you also map this rect into screen space and verify that it's nearly
integers? I have to stare at this to think if fractional translate is right or
it should be the negation of that.

https://codereview.chromium.org/2451273003/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2451273003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:656: // Snapping must be done in screen space (the
pixels we care about).
This comment is not actually true.  Snapping really needs to be done in target
space (and then that render pass snapped as well, if necessary).  I think we've
always done it screen space because it's easier and works most of the time if
there's not an intermediate render pass with a snap-destroying transform.

https://codereview.chromium.org/2451273003/diff/1/cc/trees/property_tree_buil...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2451273003/diff/1/cc/trees/property_tree_buil...
cc/trees/property_tree_builder.cc:527: DCHECK(!is_scrollable || is_snapped);
I wish we had DCHECK_IMPLIES.  :C

[jaydasika, 2016-10-26 20:35:07]

Patchset #2 (id:20001) has been deleted

[ajuma, 2016-10-26 21:19:39]

lgtm too

https://codereview.chromium.org/2451273003/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2451273003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:520: unsnapping.Subtract(current->snap_amount);
Could we DCHECK that the snap_amount is non-zero only if current->scrolls? I
don't think Blink will ever construct a layer tree where a canvas' layer has a
fixed-pos descendant or a scroll child (but if it ever did, we'd need to account
for snapping of those layers too here in order to get the right source_to_parent
value).

[jaydasika, 2016-10-26 21:49:14]

https://codereview.chromium.org/2451273003/diff/1/cc/trees/layer_tree_host_co...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/2451273003/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:3548:
fractional_translate.Translate(10.5f, 10.5f);
On 2016/10/26 20:32:32, enne wrote:
> Can you use a translation in of these components where x and 1-x are not equal
> to make sure that there's not a sign error somewhere?

Done.

https://codereview.chromium.org/2451273003/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:3563:
EXPECT_TRANSFORMATION_MATRIX_EQ(child_ptr->ScreenSpaceTransform(),
On 2016/10/26 20:32:32, enne wrote:
> Could you also map this rect into screen space and verify that it's nearly
> integers? I have to stare at this to think if fractional translate is right or
> it should be the negation of that.

Done.

https://codereview.chromium.org/2451273003/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2451273003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:520: unsnapping.Subtract(current->snap_amount);
On 2016/10/26 21:19:39, ajuma wrote:
> Could we DCHECK that the snap_amount is non-zero only if current->scrolls? I
> don't think Blink will ever construct a layer tree where a canvas' layer has a
> fixed-pos descendant or a scroll child (but if it ever did, we'd need to
account
> for snapping of those layers too here in order to get the right
source_to_parent
> value).

Done.

https://codereview.chromium.org/2451273003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:656: // Snapping must be done in screen space (the
pixels we care about).
On 2016/10/26 20:32:33, enne wrote:
> This comment is not actually true.  Snapping really needs to be done in target
> space (and then that render pass snapped as well, if necessary).  I think
we've
> always done it screen space because it's easier and works most of the time if
> there's not an intermediate render pass with a snap-destroying transform.

Done.

https://codereview.chromium.org/2451273003/diff/1/cc/trees/property_tree_buil...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2451273003/diff/1/cc/trees/property_tree_buil...
cc/trees/property_tree_builder.cc:527: DCHECK(!is_scrollable || is_snapped);
On 2016/10/26 20:32:33, enne wrote:
> I wish we had DCHECK_IMPLIES.  :C

I wish that too. Just in this CL, I have 3 DCHECK(!a || b). DCHECK_IMPLIES is
easier to understand.

[jaydasika, 2016-10-26 21:49:24]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-10-26 21:49:24]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2451273003/#ps60001
(title: "comments")

[commit-bot: I haz the power, 2016-10-26 21:50:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 22:58:37]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-26 23:00:21]

Description was changed from

==========
cc : Snap texture layers to pixel boundary

This is to avoid blurring caused by fractional translation.

BUG=629522
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc : Snap texture layers to pixel boundary

This is to avoid blurring caused by fractional translation.

BUG=629522
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/2a1718bffdb1d3c9538731498382e9aafd886ceb
Cr-Commit-Position: refs/heads/master@{#427859}
==========

[commit-bot: I haz the power, 2016-10-26 23:00:22]

Patchset 3 (id:??) landed as
https://crrev.com/2a1718bffdb1d3c9538731498382e9aafd886ceb
Cr-Commit-Position: refs/heads/master@{#427859}