Fix unexpected blurry text caused by combination of skew and promotion

Fix unexpected blurry text caused by combination of skew and promotion

BUG=631872
TEST=CompositingReasonFinderTest.CompositeSkewWithPromotedSkewedAncestor; third_party/WebKit/LayoutTests/compositing/text-within-skewed-promoted-element.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[yigu, 2017-02-24 23:34:21]

Description was changed from

==========
Fix unexpected blurry text caused by combination of skew and promotion

BUG=631872
==========

to

==========
Fix unexpected blurry text caused by combination of skew and promotion

BUG=631872
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yigu, 2017-02-25 01:31:25]

yigu@chromium.org changed reviewers:
+ flackr@chromium.org

[yigu, 2017-02-25 01:31:26]

Hi Rob, the current implementation works when loading the test page or resize
the window. The skewed text now has its own layer border as expected. However,
upon refresh the page, the separate compositing goes away. Also, based on the
layout test result from try jobs, windows and mac seem to have different
behavior.  PTAL. Thanks!

[flackr, 2017-02-25 15:16:19]

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/compositing/text-within-skewed-promoted-element.html
(right):

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/compositing/text-within-skewed-promoted-element.html:4:
Check that separately compositing text within a skewed promoted element
"Check that we separately composite text within a skewed promoted element to
improve the output quality. See https://crbug.com/631872."

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/compositing/text-within-skewed-promoted-element.html:38:
<div>Text which is part of a skewed layer is skewed after it's rastered
I think we need a unit test to verify the specific promoted layers. See
CompositingReasonFinderTest.cpp.

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:545:
ancestorLayer->getCompositingReasons()) {
Only if the ancestor was also skewed. If the ancestor layer was not skewed then
we don't need to promote skewed descendants. We might need to make this part of
CompositingRequirementsUpdater::RecursionData so that we don't have to search up
the tree every time.

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:548:
layer->layoutObject().styleRef().transform().operations()) {
Move this to TransformOperations::hasSkew.

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/CompositingReasons.h (right):

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/CompositingReasons.h:102: const
uint64_t CompositingReasonLayerForTextWithinSkewedPromotedElement =
nit: how about CompositingReasonSkewWithCompositedSkewedAncestor?

[yigu, 2017-02-27 21:54:44]

Description was changed from

==========
Fix unexpected blurry text caused by combination of skew and promotion

BUG=631872
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix unexpected blurry text caused by combination of skew and promotion

BUG=631872
TEST=CompositingReasonFinderTest.CompositeSkewWithPromotedSkewedAncestor;
third_party/WebKit/LayoutTests/compositing/text-within-skewed-promoted-element.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yigu, 2017-02-27 22:03:42]

Thanks Rob for the feedback. I have to walk down the tree one more time due to
the following situation. Suppose we have:
A(not composited by itself, skewed)
--B(not composited w/o this patch, skewed)
--C(composited).
A is not marked as composited until C has been visited. By the time we visit B
which is skewed, its ancestor A has not yet been marked composited, therefore B
cannot be marked as composited. So I have to update B after A being updated in
another traverse. Not sure how to avoid this. PTAL.

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/compositing/text-within-skewed-promoted-element.html
(right):

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/compositing/text-within-skewed-promoted-element.html:4:
Check that separately compositing text within a skewed promoted element
On 2017/02/25 15:16:18, flackr wrote:
> "Check that we separately composite text within a skewed promoted element to
> improve the output quality. See https://crbug.com/631872.%22

Done.

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/compositing/text-within-skewed-promoted-element.html:38:
<div>Text which is part of a skewed layer is skewed after it's rastered
On 2017/02/25 15:16:18, flackr wrote:
> I think we need a unit test to verify the specific promoted layers. See
> CompositingReasonFinderTest.cpp.

Done.

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:545:
ancestorLayer->getCompositingReasons()) {
On 2017/02/25 15:16:19, flackr wrote:
> Only if the ancestor was also skewed. If the ancestor layer was not skewed
then
> we don't need to promote skewed descendants. We might need to make this part
of
> CompositingRequirementsUpdater::RecursionData so that we don't have to search
up
> the tree every time.

Done.

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:548:
layer->layoutObject().styleRef().transform().operations()) {
On 2017/02/25 15:16:19, flackr wrote:
> Move this to TransformOperations::hasSkew.

Done.

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/CompositingReasons.h (right):

https://codereview.chromium.org/2714283002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/CompositingReasons.h:102: const
uint64_t CompositingReasonLayerForTextWithinSkewedPromotedElement =
On 2017/02/25 15:16:19, flackr wrote:
> nit: how about CompositingReasonSkewWithCompositedSkewedAncestor?

Done.

[flackr, 2017-02-28 21:40:14]

I think there was a slight misunderstanding, given
<div id="a" class="skew">
  <div id="b" class="skew"></div>
</div>
<div id="c" class="composited skew">
  <div id="d" class="skew"></div>
</div>
We expect #c to be composited (because of style) and #d to be composited because
it is a descendant of a composited skewed ancestor. There's also the more
general case:
<div id="c" class="skew">
  <div id="d" class="composited">
    <div id="e" class="skew"></div>
  </div>
</div>
In which case we should also promote #e.

https://codereview.chromium.org/2714283002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp
(right):

https://codereview.chromium.org/2714283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp:332:
EXPECT_EQ(PaintsIntoOwnBacking, compositedLayer->compositingState());
This should not automatically be composited, it's only if #skew is composited
that we would composite its children because they are also skewed.

https://codereview.chromium.org/2714283002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2714283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:522:
// If an skewed element is composited due to its descendants, we have to
We want the opposite behavior, the descendant should be composited if its
ancestor is skewed and was composited.

[yigu, 2017-02-28 22:10:46]

yigu@chromium.org changed reviewers:
+ pdr@chromium.org

[yigu, 2017-02-28 22:10:46]

yigu@chromium.org changed required reviewers:
+ pdr@chromium.org

[yigu, 2017-02-28 22:10:47]

Hi Philip, I'm trying to fix a bug that caused by counter-skewed non-composited
element A within a skewed composited ancestor B. The text would be very blurry
but it could be fixed by separately compositing the counter-skewed element A.
However, if the ancestor B is not composited, there is no need to composite A.
Everything looks nice. Rob's example: jsbin.com/gitige/
 
See crbug.com/631872 for more info.

The problem is that suppose we have:
A(not composited by itself, skewed)
--B(not composited w/o this patch, counter-skewed)
--C(composited due to style).
A is not marked as composited until C has been visited. By the time we visit B
which is counter-skewed, its ancestor A has not yet been marked composited,
therefore B
cannot be marked as composited. So I have to update B after A being updated in
another traversal. But it is very expensive. Any suggestion? Thanks!

[pdr., 2017-03-01 03:05:33]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[pdr., 2017-03-01 03:05:33]

pdr@chromium.org changed required reviewers:
- pdr@chromium.org

[pdr., 2017-03-01 03:05:34]

On 2017/02/28 at 22:10:47, yigu wrote:
> Hi Philip, I'm trying to fix a bug that caused by counter-skewed
non-composited element A within a skewed composited ancestor B. The text would
be very blurry but it could be fixed by separately compositing the
counter-skewed element A. However, if the ancestor B is not composited, there is
no need to composite A. Everything looks nice. Rob's example: jsbin.com/gitige/
>  
> See crbug.com/631872 for more info.
> 
> The problem is that suppose we have:
> A(not composited by itself, skewed)
> --B(not composited w/o this patch, counter-skewed)
> --C(composited due to style).
> A is not marked as composited until C has been visited. By the time we visit B
> which is counter-skewed, its ancestor A has not yet been marked composited,
therefore B
> cannot be marked as composited. So I have to update B after A being updated in
> another traversal. But it is very expensive. Any suggestion? Thanks!

Chrishtr (cc'd) may know more about this area.

What if we track whether a layer has skewed children as part of the recursive
walk, then force a compositing recalc for any skewed layer that changes
compositing and has skewed children?

[flackr, 2017-03-01 15:42:34]

On 2017/03/01 03:05:34, pdr. wrote:
> On 2017/02/28 at 22:10:47, yigu wrote:
> > Hi Philip, I'm trying to fix a bug that caused by counter-skewed
> non-composited element A within a skewed composited ancestor B. The text would
> be very blurry but it could be fixed by separately compositing the
> counter-skewed element A. However, if the ancestor B is not composited, there
is
> no need to composite A. Everything looks nice. Rob's example:
jsbin.com/gitige/
> >  
> > See crbug.com/631872 for more info.
> > 
> > The problem is that suppose we have:
> > A(not composited by itself, skewed)
> > --B(not composited w/o this patch, counter-skewed)
> > --C(composited due to style).
> > A is not marked as composited until C has been visited. By the time we visit
B
> > which is counter-skewed, its ancestor A has not yet been marked composited,
> therefore B
> > cannot be marked as composited. So I have to update B after A being updated
in
> > another traversal. But it is very expensive. Any suggestion? Thanks!
> 
> Chrishtr (cc'd) may know more about this area.
> 
> What if we track whether a layer has skewed children as part of the recursive
> walk, then force a compositing recalc for any skewed layer that changes
> compositing and has skewed children?

That sounds like it'd work without introducing any extra walks. I also think we
have a bug with snapping since the promoted skewed descendant sometimes looks
crisp depending on the particular frame positioning / sizing. In theory it
should look as good as any normal composited text.

[yigu, 2017-03-01 21:19:09]

On 2017/03/01 15:42:34, flackr wrote:
> On 2017/03/01 03:05:34, pdr. wrote:
> > On 2017/02/28 at 22:10:47, yigu wrote:
> > > Hi Philip, I'm trying to fix a bug that caused by counter-skewed
> > non-composited element A within a skewed composited ancestor B. The text
would
> > be very blurry but it could be fixed by separately compositing the
> > counter-skewed element A. However, if the ancestor B is not composited,
there
> is
> > no need to composite A. Everything looks nice. Rob's example:
> jsbin.com/gitige/
> > >  
> > > See crbug.com/631872 for more info.
> > > 
> > > The problem is that suppose we have:
> > > A(not composited by itself, skewed)
> > > --B(not composited w/o this patch, counter-skewed)
> > > --C(composited due to style).
> > > A is not marked as composited until C has been visited. By the time we
visit
> B
> > > which is counter-skewed, its ancestor A has not yet been marked
composited,
> > therefore B
> > > cannot be marked as composited. So I have to update B after A being
updated
> in
> > > another traversal. But it is very expensive. Any suggestion? Thanks!
> > 
> > Chrishtr (cc'd) may know more about this area.
> > 
> > What if we track whether a layer has skewed children as part of the
recursive
> > walk, then force a compositing recalc for any skewed layer that changes
> > compositing and has skewed children?
> 
> That sounds like it'd work without introducing any extra walks. I also think
we
> have a bug with snapping since the promoted skewed descendant sometimes looks
> crisp depending on the particular frame positioning / sizing. In theory it
> should look as good as any normal composited text.

I've made a change regarding the tracking. PTAL.

[yigu, 2017-03-06 18:22:47]

Any feedback? Thanks!

[pdr., 2017-03-06 18:24:26]

On 2017/03/06 at 18:22:47, yigu wrote:
> Any feedback? Thanks!

This appears to still fail tests. Can you fix those?

[yigu, 2017-03-06 18:27:37]

On 2017/03/06 18:24:26, pdr. wrote:
> On 2017/03/06 at 18:22:47, yigu wrote:
> > Any feedback? Thanks!
> 
> This appears to still fail tests. Can you fix those?

Ah, that's the reason. The fails are locally reproducible but I'll fix them
anyway before pinging again. Thanks!

[yigu, 2017-03-06 18:28:08]

On 2017/03/06 18:27:37, yigu wrote:
> On 2017/03/06 18:24:26, pdr. wrote:
> > On 2017/03/06 at 18:22:47, yigu wrote:
> > > Any feedback? Thanks!
> > 
> > This appears to still fail tests. Can you fix those?
> 
> Ah, that's the reason. The fails are locally reproducible but I'll fix them
> anyway before pinging again. Thanks!

*are not
But anyways.

[yigu, 2017-03-06 22:44:29]

On 2017/03/06 18:28:08, yigu wrote:
> On 2017/03/06 18:27:37, yigu wrote:
> > On 2017/03/06 18:24:26, pdr. wrote:
> > > On 2017/03/06 at 18:22:47, yigu wrote:
> > > > Any feedback? Thanks!
> > > 
> > > This appears to still fail tests. Can you fix those?
> > 
> > Ah, that's the reason. The fails are locally reproducible but I'll fix them
> > anyway before pinging again. Thanks!
> 
> *are not
> But anyways.

The failed tests have been fixed. PTAL. Thanks!

[chrishtr, 2017-03-06 22:54:35]

Sorry for not responding earlier, in particular after comment 9 on issue 631872.
I didn't see
the comment go by at the time.

Am I correct that this bug applies to cases where there is both a skew and a
counter-skew?
That seems a very obscure case to justify additional complexity like this. Is
this a very
common situation for some reason?

[flackr, 2017-03-07 20:20:24]

On 2017/03/06 22:54:35, chrishtr wrote:
> Sorry for not responding earlier, in particular after comment 9 on issue
631872.
> I didn't see
> the comment go by at the time.
> 
> Am I correct that this bug applies to cases where there is both a skew and a
> counter-skew?
> That seems a very obscure case to justify additional complexity like this. Is
> this a very
> common situation for some reason?

My thought was that this is one of a class of cases where nesting a
non-composited transform in another transform causes severe degradation of
quality, and that it would make sense to experiment with a single case rather
than trying to handle a more general problem of all quality degrading transforms
up front. Do you think it's not worth handling, or perhaps we should try to
collect metrics first? It was at least important enough that a bug was filed
about a live site.

[chrishtr, 2017-03-08 22:43:17]

On 2017/03/07 at 20:20:24, flackr wrote:
> On 2017/03/06 22:54:35, chrishtr wrote:
> > Sorry for not responding earlier, in particular after comment 9 on issue
631872.
> > I didn't see
> > the comment go by at the time.
> > 
> > Am I correct that this bug applies to cases where there is both a skew and a
> > counter-skew?
> > That seems a very obscure case to justify additional complexity like this.
Is
> > this a very
> > common situation for some reason?
> 
> My thought was that this is one of a class of cases where nesting a
non-composited transform in another transform causes severe degradation of
quality, and that it would make sense to experiment with a single case rather
than trying to handle a more general problem of all quality degrading transforms
up front.

There are other known bugs about transform-inside-transform with bad quality?


> Do you think it's not worth handling, or perhaps we should try to collect
metrics first? It was at least important enough that a bug was filed about a
live site.

My feeling is that think this case is worth the complexity of this patch. It
seems like a very corner case situation. I have seen many more bugs about
blurriness of composited content under various kinds of special transform
(e.g. perspective: crbug.com/320369 or scaling), rather than 2D transform. I
could be wrong though if you have more data.

[flackr, 2017-03-09 15:04:42]

On 2017/03/08 22:43:17, chrishtr wrote:
> On 2017/03/07 at 20:20:24, flackr wrote:
> > On 2017/03/06 22:54:35, chrishtr wrote:
> > > Sorry for not responding earlier, in particular after comment 9 on issue
> 631872.
> > > I didn't see
> > > the comment go by at the time.
> > > 
> > > Am I correct that this bug applies to cases where there is both a skew and
a
> > > counter-skew?
> > > That seems a very obscure case to justify additional complexity like this.
> Is
> > > this a very
> > > common situation for some reason?
> > 
> > My thought was that this is one of a class of cases where nesting a
> non-composited transform in another transform causes severe degradation of
> quality, and that it would make sense to experiment with a single case rather
> than trying to handle a more general problem of all quality degrading
transforms
> up front.
> 
> There are other known bugs about transform-inside-transform with bad quality?
> 
If I understand correctly, any non-promoting transform drawing into a
transformed ancestor layer could lead to poor quality if it's not a simple
translation. I put together a simple page demonstrating this:

http://jsbin.com/tacopur/edit?html,css,output

> 
> > Do you think it's not worth handling, or perhaps we should try to collect
> metrics first? It was at least important enough that a bug was filed about a
> live site.
> 
> My feeling is that think this case is worth the complexity of this patch. It
> seems like a very corner case situation. I have seen many more bugs about
> blurriness of composited content under various kinds of special transform
> (e.g. perspective: crbug.com/320369 or scaling), rather than 2D transform. I
> could be wrong though if you have more data.

I don't have any extra data to suggest how common this is.

[chrishtr, 2017-03-09 17:20:35]

On 2017/03/09 at 15:04:42, flackr wrote:
> On 2017/03/08 22:43:17, chrishtr wrote:
> > On 2017/03/07 at 20:20:24, flackr wrote:
> > > On 2017/03/06 22:54:35, chrishtr wrote:
> > > > Sorry for not responding earlier, in particular after comment 9 on issue
> > 631872.
> > > > I didn't see
> > > > the comment go by at the time.
> > > > 
> > > > Am I correct that this bug applies to cases where there is both a skew
and a
> > > > counter-skew?
> > > > That seems a very obscure case to justify additional complexity like
this.
> > Is
> > > > this a very
> > > > common situation for some reason?
> > > 
> > > My thought was that this is one of a class of cases where nesting a
> > non-composited transform in another transform causes severe degradation of
> > quality, and that it would make sense to experiment with a single case
rather
> > than trying to handle a more general problem of all quality degrading
transforms
> > up front.
> > 
> > There are other known bugs about transform-inside-transform with bad
quality?
> > 
> If I understand correctly, any non-promoting transform drawing into a
transformed ancestor layer could lead to poor quality if it's not a simple
translation. I put together a simple page demonstrating this:
> 
> http://jsbin.com/tacopur/edit?html,css,output

Right, ok.

> 
> > 
> > > Do you think it's not worth handling, or perhaps we should try to collect
> > metrics first? It was at least important enough that a bug was filed about a
> > live site.
> > 
> > My feeling is that think this case is worth the complexity of this patch. It
> > seems like a very corner case situation. I have seen many more bugs about
> > blurriness of composited content under various kinds of special transform
> > (e.g. perspective: crbug.com/320369 or scaling), rather than 2D transform. I
> > could be wrong though if you have more data.
> 
> I don't have any extra data to suggest how common this is.

Given no more data I would prefer not to add complexity.

Note that Tien-Ren pointed out that a possible SPv2 solution could be to raster
everything in screen space. He's been working through that idea conceptually for
a while now.

[chrishtr, 2017-03-17 17:09:52]

Close this CL then, right?

[yigu, 2017-03-17 17:14:45]

On 2017/03/17 17:09:52, chrishtr wrote:
> Close this CL then, right?

Yes. Closed.