Observe LCD text restrictions during layer animations

Observe LCD text restrictions during layer animations

Drawing LCD text into non-opaque layers is never correct - we should
update DrawProperties.can_use_lcd_text during animations also.

BUG=486892
R=danakj@chromium.org,enne@chromium.org

Committed: https://crrev.com/fcd926a57df8c32f16ff05c8917548ec104943f7
Cr-Commit-Position: refs/heads/master@{#329705}

[f(malita), 2015-05-12 14:55:39]

I don't fully understand what adjust_text_aa was trying to protect against, so
please look closely.

As far as Blink is concerned, changes in layer LCD-ness are completely
uninteresting at this point and should not require re-recording.

[enne (OOO), 2015-05-12 21:22:52]

Is the bug here that the opaque-ness changes on the same frame that an animation
starts, and this code doesn't pick up on this? If so, can you write a test to
verify that particular change?

I think in general the "repaints" pointed out here are about rerastering and
don't have anything to do with Blink.  I think the point of this code is that if
you have a layer that is opaque and then starts rotating or something, then
maybe we just want to animate it and fix it up later, because it's correct
enough and it's better to have the animation start right away than jank as you
wait for new tiles to raster first.

[f(malita), 2015-05-13 14:03:30]

On 2015/05/12 21:22:52, enne wrote:
> Is the bug here that the opaque-ness changes on the same frame that an
animation
> starts, and this code doesn't pick up on this? 

That's exactly right. Minimized test case: http://jsbin.com/kobequ/edit

(fails most reliably when I open just the output panel:
http://output.jsbin.com/kobequ/1)


> If so, can you write a test to
> verify that particular change?

Done.


> I think in general the "repaints" pointed out here are about rerastering and
> don't have anything to do with Blink.  I think the point of this code is that
if
> you have a layer that is opaque and then starts rotating or something, then
> maybe we just want to animate it and fix it up later, because it's correct
> enough and it's better to have the animation start right away than jank as you
> wait for new tiles to raster first.

I see. If the layer is no longer opaque though (and our prev state had LCD text
enabled), we should definitely hold the presses and re-raster to avoid
artifacts.

So the minimal fix would look something like this:

  bool adjust_text_aa =
      (!animating_opacity_to_screen && !animating_transform_to_screen) ||
!layer->contents_opaque();
 
WDYT?

OTOH PictureLayerImpl::UpdateCanUseLCDTextAfterCommit() only allows for a single
LCD-enabled -> LCD-disabled raster source sate transition. Wouldn't that prevent
spurious re-rasterizations on its own, even without the adjust_text_aa latch?
(IOW can we get rid of adjust_text_aa without any negative side-effects?)

[enne (OOO), 2015-05-13 18:17:24]

lgtm

Thanks.  I think this adjust_aa code has been there for nearly forever, and is
not an issue these days with impl-side painting and more insight into whether
animations will adjust scales.

[f(malita), 2015-05-13 20:14:17]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-05-13 20:14:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132253003/20001

[commit-bot: I haz the power, 2015-05-13 20:19:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-13 20:20:30]

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