Use LCD text if the transform IsAlmostIdentityAndIntegerTranslation

Use LCD text if the transform IsAlmostIdentityAndIntegerTranslation

Some components in transforms has tiny errors which come from mixed float and
double calculations of scales and transforms. This almost always prevents
LCD text from being used because the transform almost always doesn't meet the
condition IsIdentityAndIntegerTranslation().

Add gfx::Transform::IsAlmostIdentityAndIntegerTranslation() and check it for
LCD text.

[enne (OOO), 2013-03-07 19:05:17]

I'd really like shawnsingh to chime in here about this rounding since he had
expressed a number of concerns in the other code review.  I have a couple of
initial questions, though:

[enne (OOO), 2013-03-07 19:05:34]

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:748:
roundAlmostIntegerScalesAndTranslationComponents(&layerDrawProperties.target_space_transform);
Is it ok to round the draw transform but not the screen space transform
equivalently? Is that dangerous for damage tracking?

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:796:
roundAlmostIntegerScalesAndTranslationComponents(&layerDrawProperties.target_space_transform);
I don't understand what this change does.  The LCD text setting has already been
set above for this layer and the target space transform is not the value that is
propagated down the subtree.

[Xianzhu, 2013-03-07 19:38:17]

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:748:
roundAlmostIntegerScalesAndTranslationComponents(&layerDrawProperties.target_space_transform);
On 2013/03/07 19:05:34, enne wrote:
> Is it ok to round the draw transform but not the screen space transform
> equivalently? Is that dangerous for damage tracking?

I think the rounding will reflect in screen_space_transform because it includes
target_space_transform.

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:796:
roundAlmostIntegerScalesAndTranslationComponents(&layerDrawProperties.target_space_transform);
On 2013/03/07 19:05:34, enne wrote:
> I don't understand what this change does.  The LCD text setting has already
been
> set above for this layer and the target space transform is not the value that
is
> propagated down the subtree.

Based on the exisiting code, we enable LCD text only when the transform
IsIdentityOrIntegerTranslation, otherwise there might be "color fringing". I
think if this target_spae_transform is not IdentityOrIntegerTranslation, LCD
text may be incorrect (in theory). I don't know much about the underlying
theories :)

[shawnsingh, 2013-03-07 20:13:36]

I'm still uneasy about rounding in calcDrawProperties, but I also can't come up
with any concrete examples of what could go wrong =)   Otherwise this patch
looks good to me. =)

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:748:
roundAlmostIntegerScalesAndTranslationComponents(&layerDrawProperties.target_space_transform);
On 2013/03/07 19:38:17, Xianzhu wrote:
> On 2013/03/07 19:05:34, enne wrote:
> > Is it ok to round the draw transform but not the screen space transform
> > equivalently? Is that dangerous for damage tracking?
> 
> I think the rounding will reflect in screen_space_transform because it
includes
> target_space_transform.

I agree with Xianzhu, it should be OK with respect to damage tracking, because
the screen space transform is computed from the rounded target space transform. 
I think the damage tracker will work fine as long as it uses the exact same
transforms that are used for drawing.

[enne (OOO), 2013-03-07 20:19:36]

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:796:
roundAlmostIntegerScalesAndTranslationComponents(&layerDrawProperties.target_space_transform);
On 2013/03/07 19:38:17, Xianzhu wrote:
> On 2013/03/07 19:05:34, enne wrote:
> > I don't understand what this change does.  The LCD text setting has already
> been
> > set above for this layer and the target space transform is not the value
that
> is
> > propagated down the subtree.
> 
> Based on the exisiting code, we enable LCD text only when the transform
> IsIdentityOrIntegerTranslation, otherwise there might be "color fringing". I
> think if this target_spae_transform is not IdentityOrIntegerTranslation, LCD
> text may be incorrect (in theory). I don't know much about the underlying
> theories :)

What you say is correct.  However, we enable LCD text on line 759 and this is
line 796.  Also, the transform that is passed to children is the
combinedTransform (well, later sublayerMatrix), which does not come from
target_space_transform.  So, setting this value will not change LCD text for
this surface's subtree or for this layer.  That's what I'm trying to say.

If you want to affect surfaces, you need to round the combinedTransform up
above, since that's what's used on line 788.

[shawnsingh, 2013-03-07 20:28:39]

combinedTransform is also the value that propagates further down the tree, so
rounding it would cause potentially more error accumulation.   Maybe what we
really want here is to round the surface draw transform if needed.

I actually had a similar question about the combinedTransform rounding for the
scroll rounding patch from earlier https://codereview.chromium.org/12407002/ . 
Won't truncation errors accumulate down the hierarchy, then?  Or is it
reasonable to assume that truncation errors would be no worse than the existing
imprecision?

[Xianzhu, 2013-03-07 21:11:21]

On 2013/03/07 20:19:36, enne wrote:
>
https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common.cc
> File cc/layer_tree_host_common.cc (right):
> 
>
https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common....
> cc/layer_tree_host_common.cc:796:
>
roundAlmostIntegerScalesAndTranslationComponents(&layerDrawProperties.target_space_transform);
> On 2013/03/07 19:38:17, Xianzhu wrote:
> > On 2013/03/07 19:05:34, enne wrote:
> > > I don't understand what this change does.  The LCD text setting has
already
> > been
> > > set above for this layer and the target space transform is not the value
> that
> > is
> > > propagated down the subtree.
> > 
> > Based on the exisiting code, we enable LCD text only when the transform
> > IsIdentityOrIntegerTranslation, otherwise there might be "color fringing". I
> > think if this target_spae_transform is not IdentityOrIntegerTranslation, LCD
> > text may be incorrect (in theory). I don't know much about the underlying
> > theories :)
> 
> What you say is correct.  However, we enable LCD text on line 759 and this is
> line 796.  Also, the transform that is passed to children is the
> combinedTransform (well, later sublayerMatrix), which does not come from
> target_space_transform.  So, setting this value will not change LCD text for
> this surface's subtree or for this layer.  That's what I'm trying to say.
> 
> If you want to affect surfaces, you need to round the combinedTransform up
> above, since that's what's used on line 788.

I don't want to change LCD text for this surface or the subtree, but to ensure
that if LCD text has been set to true, there won't be "color fringing" because
of this otherwise non-identityOrIntegerTranslation target_space_transform.
However actually I don't know if such a transform with tiny errors would cause
"color fringing" for LCD text.

[Ian Vollick, 2013-03-07 21:18:36]

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:564: if (transform->IsScaleOrTranslation() &&
!transform->IsIdentityOrTranslation()) {
Unless I'm reading this wrong, it looks like we'll opt in if (scale ||
translation) && (!identity && !translation) -- i.e., a straight translation
can't be rounded. Isn't that the the most important case?

https://codereview.chromium.org/12541006/diff/2001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:748:
roundAlmostIntegerScalesAndTranslationComponents(&layerDrawProperties.target_space_transform);
On 2013/03/07 20:13:36, shawnsingh wrote:
> On 2013/03/07 19:38:17, Xianzhu wrote:
> > On 2013/03/07 19:05:34, enne wrote:
> > > Is it ok to round the draw transform but not the screen space transform
> > > equivalently? Is that dangerous for damage tracking?
> > 
> > I think the rounding will reflect in screen_space_transform because it
> includes
> > target_space_transform.
> 
> I agree with Xianzhu, it should be OK with respect to damage tracking, because
> the screen space transform is computed from the rounded target space
transform. 
> I think the damage tracker will work fine as long as it uses the exact same
> transforms that are used for drawing.

I'd considered tackling this sort of rounding, too. At that time, I'd thought
that the safest spot was at the last possible moment -- in gl_renderer.cc right
before we hand the matrix off to gl. This would protect us from a mistake where
we modify the screen space transform after rounding it and busting our LCD text.
Is that an option?

[Xianzhu, 2013-03-07 21:26:30]

Actually we can't round combinedTransform or surface draw transform because they
are not normally "almost integer" so we don't know the errors. Only for
components of target_surface_transform sometimes we expect they are
identityOrIntegerTranslation, and if they are almost integer (distance <0.00001
to the nearest integer) we know the errors and eliminate them by rounding.

Another method is not to round the transforms, but add a new method
gfx::Transform::IsAlmostIdentityOrIntegerTranslate() so that we can apply LCD
text correctly.

A recent bug (crbug.com/180101) let me notice the importance of correct LCD text
setting. Though the bug itself is about correct gamma setting, not about LCD
text setting, but on some platforms, LCD text setting directly affects gamma
setting, and in turn affects the appearance of text.

About the previous rounding of translation for scroll offsets, as they only
apply to the transform as if we are adjusting the scroll offsets, I think they
won't bring real inaccuracy to the transforms.

[Xianzhu, 2013-03-07 21:37:18]

How about Patch Set 2 (for your preview, not finished yet)?

[enne (OOO), 2013-03-07 21:52:15]

https://codereview.chromium.org/12541006/diff/16001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12541006/diff/16001/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:734:
layerDrawProperties.target_space_transform.IsAlmostIdentityOrIntegerTranslation();
I think it's fine to do the check here, but then I think you also need to round
the combinedTransform if this is true, otherwise you'll get noticeable fringing
as the text is stretched ever-so-slightly.

[Xianzhu, 2013-03-07 21:55:35]

https://codereview.chromium.org/12541006/diff/16001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12541006/diff/16001/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:734:
layerDrawProperties.target_space_transform.IsAlmostIdentityOrIntegerTranslation();
On 2013/03/07 21:52:15, enne wrote:
> I think it's fine to do the check here, but then I think you also need to
round
> the combinedTransform if this is true, otherwise you'll get noticeable
fringing
> as the text is stretched ever-so-slightly.

I have no idea how to round combinedTransform because normally there aren't
"almost integers" in it. Compensating the errors back into it may cause more
errors.

[Xianzhu, 2013-03-07 22:09:29]

I also want to confirm, could errors < 0.00001 (might be even much smaller)
really cause color fringing?

[enne (OOO), 2013-03-07 22:14:28]

On 2013/03/07 22:09:29, Xianzhu wrote:
> I also want to confirm, could errors < 0.00001 (might be even much smaller)
> really cause color fringing?

http://crbug.com/178273 was about scaling of 1 + x / layer size, where x < 1 and
folks were complaining about blurry text.

If you feel like it won't cause fringing, I'm willing to be convinced with
examples.

[Xianzhu, 2013-03-07 22:39:23]

On 2013/03/07 22:14:28, enne wrote:
> On 2013/03/07 22:09:29, Xianzhu wrote:
> > I also want to confirm, could errors < 0.00001 (might be even much smaller)
> > really cause color fringing?
> 
> http://crbug.com/178273 was about scaling of 1 + x / layer size, where x < 1
and
> folks were complaining about blurry text.
> 
> If you feel like it won't cause fringing, I'm willing to be convinced with
> examples.

I know that bug and change. However, that error (about 10^-3) coming from
artificial extra half pixel is orders of magnitude higher than the error caused
by mixed float/double calculation (about 10^-8).

The error in the transform scale comes from the following calculations:

1.0 * pageScale * deviceScale / (1.0 / float(pageScale * deviceScale)) != 1.0

All the operations are double operations. The main factor is "float(pageScale *
deviceScale)" that is sometimes the contentsScale of the layer stored as a
float.

Will test how 10^-8 ~ 10^-6 scaling errors affect text appearance.

[Xianzhu, 2013-03-08 02:22:42]

Haven't find a testing environment directly supporting LCD text. Forced LCD
rendering on chromium-linux but I'm not sure the LCD color ordering is correct
and my presbyopia eyes can't see any difference.

However, about blurriness, we have tested on Android and verified that a scale
error like 1e-3 can cause obvious blurriness on Nexus 7, but 1e-8 doesn't cause
any visible blurriness.

I would abandon this change because of my limited experience on LCD text and my
poor eyes.

[jamesr, 2013-03-08 02:25:26]

On 2013/03/08 02:22:42, Xianzhu wrote:
> Haven't find a testing environment directly supporting LCD text. Forced LCD
> rendering on chromium-linux but I'm not sure the LCD color ordering is correct
> and my presbyopia eyes can't see any difference.

On 'linux' try using the 'xmag' utility.

[Xianzhu, 2013-03-08 17:19:47]

On 2013/03/08 02:25:26, jamesr wrote:
> On 2013/03/08 02:22:42, Xianzhu wrote:
> > Haven't find a testing environment directly supporting LCD text. Forced LCD
> > rendering on chromium-linux but I'm not sure the LCD color ordering is
correct
> > and my presbyopia eyes can't see any difference.
> 
> On 'linux' try using the 'xmag' utility.

I think the LCD RGB order is based on how RGB subpixels are physically arranged
within the pixel elements. Correctly applied LCD masks (no visible color
fringing) may produce 'color fringing' when magnified. However I'm not sure
about the correctness of my knowledge.

[Xianzhu, 2013-03-08 17:32:03]

Uploaded a new patch and changed the title. Now it doesn't round the transform
but loose the condition of LCD text.

[alokp, 2013-03-08 19:40:06]

https://codereview.chromium.org/12541006/diff/29001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12541006/diff/29001/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:734:
layerDrawProperties.target_space_transform.IsAlmostIdentityOrIntegerTranslation();
If you make a decision based on rounded transform, but use the actual transform
to do the drawing, I am not sure if the pixels will line up as expected. I also
do not know how to compute the epsilon that lets you do this.

I will be more comfortable if we were also modifying the target-space-transform,
but I am sure it will cause other problems.

https://codereview.chromium.org/12541006/diff/29001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

https://codereview.chromium.org/12541006/diff/29001/ui/gfx/transform.cc#newco...
ui/gfx/transform.cc:34: const double kErrorThreshold = 1e-6;
The error threshold should really depend on the transformation matrix itself. My
intuition suggests that is should be scaled with the determinant of the matrix.

For now it can stay fixed like you have here because you are only using it for
almost identity matrices. But may be a comment may be good for posterity.

[Xianzhu, 2013-03-08 23:36:55]

https://codereview.chromium.org/12541006/diff/29001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12541006/diff/29001/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:734:
layerDrawProperties.target_space_transform.IsAlmostIdentityOrIntegerTranslation();
On 2013/03/08 19:40:06, Alok Priyadarshi wrote:
> If you make a decision based on rounded transform, but use the actual
transform
> to do the drawing, I am not sure if the pixels will line up as expected. I
also
> do not know how to compute the epsilon that lets you do this.
> 

Do you have experience about how much pixel displacement would cause color
fringing? For example, would 1/1000 displacement cause it? 1/1000 is the
displacement of the bottom-right-most pixel when we use 1e-6 epsilon for a
1000x1000 screen.

About blurriness, based on my tests, >1/10 pixel displacement will cause visible
blurriness. Given a 1000x1000 screen, a scale error >1e-4 (1/10 * 1/1000) will
cause the bottom-right-most pixel to get a displacement >1/10 pixels.

> I will be more comfortable if we were also modifying the
target-space-transform,
> but I am sure it will cause other problems.

Patch Set 1 modifies target_space_transform, which got also many concerns.

https://codereview.chromium.org/12541006/diff/29001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

https://codereview.chromium.org/12541006/diff/29001/ui/gfx/transform.cc#newco...
ui/gfx/transform.cc:34: const double kErrorThreshold = 1e-6;
On 2013/03/08 19:40:06, Alok Priyadarshi wrote:
> The error threshold should really depend on the transformation matrix itself.
My
> intuition suggests that is should be scaled with the determinant of the
matrix.
> 
> For now it can stay fixed like you have here because you are only using it for
> almost identity matrices. But may be a comment may be good for posterity.

My another thought is not to add these methods but add a new local function in
layer_tree_host_common.cc named IsTransformAcceptableForLCDText() so that we can
just use a special rule for that purpose. See my another comment.

[Xianzhu, 2013-03-11 15:59:08]

I'm closing this issue. Will pickup if users complain about the incorrect LCD
text settings and/or incorrect gamma correct caused by it.