Record SkPicture with correct LCD text setting.

Record SkPicture with correct LCD text setting.

We used to always recorded with LCD ON and if needed disabled it with an
SkFilter during rasterization. This is wasteful on platforms where LCD
text is always disabled. This patch records SkPicture with correct
setting. One downside to this approach is that we would need to
invalidate and record anytime the LCD text setting changes. But since
this can only happen once for a layer (all future changes are ignored),
this is not going to be a big performance issue.

On the positive side, this patch will simplify a lot of things on the
rasterization front - no SkDrawFilter, or LCD text tile versions, or
a special rasterization mode for disable LCD.

BUG=368513
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276785

[alokp, 2014-06-06 05:23:52]

To keep the patch small, I am not updating the impl-side. I will do that in a
follow-up patch.

[enne (OOO), 2014-06-06 16:59:32]

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
cc/layers/content_layer_client.h:34: virtual bool PaintsLCDText() const = 0;
Please don't change this.  It should be up to the layer client whether or not
the layer should be invalidated or not.  The ui compositor can't repaint layers
and doesn't want these extra invalidations.

[alokp, 2014-06-06 19:15:05]

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
cc/layers/content_layer_client.h:34: virtual bool PaintsLCDText() const = 0;
On 2014/06/06 16:59:32, enne wrote:
> Please don't change this.  It should be up to the layer client whether or not
> the layer should be invalidated or not.  The ui compositor can't repaint
layers
> and doesn't want these extra invalidations.

That is what this function is supposed to prevent. The ui compositor layers are
NOT invalidated. The layers are only invalidated if this function returns true.

[alokp, 2014-06-06 22:05:40]

Please see the latest patch for unittest that makes sure that layers with
clients that do not paint lcd text are not invalidated when LCD text status
changes.

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
cc/layers/content_layer_client.h:34: virtual bool PaintsLCDText() const = 0;
I realized that there is no test for this. I have added it in
LayerTreeHostTestLCDText.

[alokp, 2014-06-06 22:11:48]

+danakj for ui/compositor.

[enne (OOO), 2014-06-06 22:21:00]

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
cc/layers/content_layer_client.h:34: virtual bool PaintsLCDText() const = 0;
On 2014/06/06 19:15:05, Alok Priyadarshi wrote:
> On 2014/06/06 16:59:32, enne wrote:
> > Please don't change this.  It should be up to the layer client whether or
not
> > the layer should be invalidated or not.  The ui compositor can't repaint
> layers
> > and doesn't want these extra invalidations.
> 
> That is what this function is supposed to prevent. The ui compositor layers
are
> NOT invalidated. The layers are only invalidated if this function returns
true.

Sure, but this also means that ui layers can no longer say that they want LCD
text if they don't want to be invalidated.

Maybe piman can chime in here?

[alokp, 2014-06-06 22:31:27]

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
cc/layers/content_layer_client.h:34: virtual bool PaintsLCDText() const = 0;
On 2014/06/06 22:20:59, enne wrote:
> On 2014/06/06 19:15:05, Alok Priyadarshi wrote:
> > On 2014/06/06 16:59:32, enne wrote:
> > > Please don't change this.  It should be up to the layer client whether or
> not
> > > the layer should be invalidated or not.  The ui compositor can't repaint
> > layers
> > > and doesn't want these extra invalidations.
> > 
> > That is what this function is supposed to prevent. The ui compositor layers
> are
> > NOT invalidated. The layers are only invalidated if this function returns
> true.
> 
> Sure, but this also means that ui layers can no longer say that they want LCD
> text if they don't want to be invalidated.
> 
> Maybe piman can chime in here?

But if you actually paint LCD text and it is no longer allowed, the layer should
be invalidated. Otherwise it will look ugly. Do UI layers paint lcd even when it
is not allowed?

[alokp, 2014-06-10 17:18:59]

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
cc/layers/content_layer_client.h:34: virtual bool PaintsLCDText() const = 0;
piman: Do you see any issues with modifying this API. The ui::layer will still
not be invalidated - it used to ignore this callback anyway. Modifying this API
allows me to avoid tracking the LCD state at two places - ContentLayer and
WebContentLayerImpl. IMO this is much cleaner.

[enne (OOO), 2014-06-10 18:46:09]

https://codereview.chromium.org/315393002/diff/80001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/315393002/diff/80001/cc/layers/picture_layer....
cc/layers/picture_layer.cc:205: SetNeedsDisplay();
Re: "The ui::layer will still not be invalidated"

The ui layer could get invalidated right here.  In the past, it could ignore the
callback and not do an invalidation.

[alokp, 2014-06-10 20:54:43]

https://codereview.chromium.org/315393002/diff/80001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/315393002/diff/80001/cc/layers/picture_layer....
cc/layers/picture_layer.cc:205: SetNeedsDisplay();
On 2014/06/10 18:46:09, enne wrote:
> Re: "The ui::layer will still not be invalidated"
> 
> The ui layer could get invalidated right here.  In the past, it could ignore
the
> callback and not do an invalidation.

Not if PaintsLCDText returns false.

https://codereview.chromium.org/315393002/diff/80001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/315393002/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:668: return false;
ui::Layer::PaintsLCDText returns false. If it returns true and lcd text is not
allowed it is reasonable that the layer is invalidated.

[piman, 2014-06-10 22:57:45]

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
cc/layers/content_layer_client.h:34: virtual bool PaintsLCDText() const = 0;
On 2014/06/10 17:18:59, Alok Priyadarshi wrote:
> piman: Do you see any issues with modifying this API. The ui::layer will still
> not be invalidated - it used to ignore this callback anyway. Modifying this
API
> allows me to avoid tracking the LCD state at two places - ContentLayer and
> WebContentLayerImpl. IMO this is much cleaner.

I can't tell for sure if that happens.
What is true though is that there are states where the UI layers want to avoid
invalidations because they can't repaint any more (during closing animations and
the like). When that is the case, it'd be preferable to have somewhat incorrect
LCD text rather than black.

[alokp, 2014-06-10 23:33:47]

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
File cc/layers/content_layer_client.h (right):

https://codereview.chromium.org/315393002/diff/20001/cc/layers/content_layer_...
cc/layers/content_layer_client.h:34: virtual bool PaintsLCDText() const = 0;
AFAICT in the current world ui::Layer is never invalidated due to change in LCD
text status. This patch does not change that. I guess I do not see how ui::Layer
may get invalidated. Are you worried about the case in future when
ui::Layer::PaintsLCDText may want to return true?

[piman, 2014-06-10 23:48:31]

https://codereview.chromium.org/315393002/diff/80001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/315393002/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:668: return false;
On 2014/06/10 20:54:43, Alok Priyadarshi wrote:
> ui::Layer::PaintsLCDText returns false. If it returns true and lcd text is not
> allowed it is reasonable that the layer is invalidated.

I'm pretty sure the UI paints LCD text today. Is it ok to return false here?

[alokp, 2014-06-10 23:59:41]

https://codereview.chromium.org/315393002/diff/80001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/315393002/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:668: return false;
On 2014/06/10 23:48:31, piman wrote:
> On 2014/06/10 20:54:43, Alok Priyadarshi wrote:
> > ui::Layer::PaintsLCDText returns false. If it returns true and lcd text is
not
> > allowed it is reasonable that the layer is invalidated.
> 
> I'm pretty sure the UI paints LCD text today.

Irrespective of cc::Layer::can_paint_lcd_text? If it does take that into
account, we would need to plumb the new can_paint_lcd_text argument on
ui::Layer::PaintContents (see above).

> Is it ok to return false here?

[piman, 2014-06-11 00:02:27]

https://codereview.chromium.org/315393002/diff/80001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/315393002/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:668: return false;
On 2014/06/10 23:59:41, Alok Priyadarshi wrote:
> On 2014/06/10 23:48:31, piman wrote:
> > On 2014/06/10 20:54:43, Alok Priyadarshi wrote:
> > > ui::Layer::PaintsLCDText returns false. If it returns true and lcd text is
> not
> > > allowed it is reasonable that the layer is invalidated.
> > 
> > I'm pretty sure the UI paints LCD text today.
> 
> Irrespective of cc::Layer::can_paint_lcd_text?

Yes. I'm pretty sure that when that was added to cc::Layer, it was not plumbed
into the UI. The UI may be making the (slightly incorrect) assumption that
things are opaque and will work.

> If it does take that into
> account, we would need to plumb the new can_paint_lcd_text argument on
> ui::Layer::PaintContents (see above).
> 
> > Is it ok to return false here?
>

[alokp, 2014-06-11 01:01:36]

https://codereview.chromium.org/315393002/diff/80001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/315393002/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:668: return false;
Hmm. In that case I agree that it is better to keep
ContentLayerClient::DidChangeLayerCanUseLCDText. I will update the patch.

[alokp, 2014-06-11 06:45:34]

enne: PTAL

[enne (OOO), 2014-06-11 17:22:22]

lgtm

[alokp, 2014-06-11 17:49:56]

enne: Sorry for the trouble. When I updated my patch last night, by mistake I
reverted changes in PictureLayer and the associated unittest. I have added them
back in the latest patch. Could you please take a look?

The code in PictureLayer is the exact copy of that in ContentLayer. Now the
PictureLayer needs to inform ContentLayerClient of LCD status change the same
way as the ContentLayer.

[enne (OOO), 2014-06-11 17:52:43]

lgtm

[alokp, 2014-06-11 17:55:03]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-06-11 17:59:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alokp@chromium.org/315393002/140001

[commit-bot: I haz the power, 2014-06-11 21:18:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 21:25:08]

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

[commit-bot: I haz the power, 2014-06-11 21:25:09]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[alokp, 2014-06-11 21:40:48]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:43:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alokp@chromium.org/315393002/140001

[commit-bot: I haz the power, 2014-06-11 21:47:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 21:52:15]

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

[commit-bot: I haz the power, 2014-06-11 21:52:16]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[alokp, 2014-06-11 22:05:11]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:07:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alokp@chromium.org/315393002/140001

[commit-bot: I haz the power, 2014-06-11 22:38:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 22:40:35]

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

[commit-bot: I haz the power, 2014-06-11 22:40:36]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[alokp, 2014-06-11 22:51:48]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:52:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alokp@chromium.org/315393002/140001

[commit-bot: I haz the power, 2014-06-11 22:57:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 22:59:48]

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

[commit-bot: I haz the power, 2014-06-11 22:59:49]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[alokp, 2014-06-12 17:12:15]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-06-12 17:14:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alokp@chromium.org/315393002/140001

[commit-bot: I haz the power, 2014-06-12 20:26:22]

Change committed as 276785