cc: Calculate "can use lcd text" on the compositor thread

cc: Calculate "can use lcd text" on the compositor thread

Property trees don't calculate lcd text settings on the main thread, so
to move to them, these calculations need to move to the compositor
thread.

There's nothing tied to the main thread, other than only changing lcd
text during a commit.  This restriction avoids the pitfall of tile
versions (i.e. changing out content in place with different settings) by
only changing content and invalidating during a commit, which already
waits for rasterization to occur before displaying that content.

That restriction is satisfied by only updating lcd text on picture
layers during an update on the sync tree on new frame numbers.

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

[enne (OOO), 2015-02-18 00:23:32]

enne@chromium.org changed reviewers:
+ ajuma@chromium.org, danakj@chromium.org, vollick@chromium.org

[danakj, 2015-02-18 01:24:10]

https://codereview.chromium.org/913203006/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/913203006/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:582: UpdateRasterSource(new_raster_source,
&invalidation, nullptr);
what if there is a pending tree, would this happen on both trees independently?
does that leave us not sharing tiles between the two trees, so they'll duplicate
raster work and slow down activation?

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (left):

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:878: UMA_HISTOGRAM_PERCENTAGE(
Are these not being used anymore?

[danakj, 2015-02-18 01:25:52]

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_impl.cc#...
cc/trees/layer_tree_impl.cc:586: // It'd be ideal if this could be done earlier,
but when the raster source
Can you move this out of the ::Occlusion {}? Maybe down with UpdateTiles?

[enne (OOO), 2015-02-18 02:20:02]

https://codereview.chromium.org/913203006/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/913203006/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:582: UpdateRasterSource(new_raster_source,
&invalidation, nullptr);
On 2015/02/18 01:24:10, danakj wrote:
> what if there is a pending tree, would this happen on both trees
independently?
> does that leave us not sharing tiles between the two trees, so they'll
duplicate
> raster work and slow down activation?

This should only ever be called on the sync tree right after a commit, so is
equivalent to the PictureLayer::UpdateRasterSource call.  I can add DCHECKs here
to document those assumptions?

[danakj, 2015-02-18 02:28:13]

Ah I missed that in my quick look thru. Thanks

https://codereview.chromium.org/913203006/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/913203006/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:582: UpdateRasterSource(new_raster_source,
&invalidation, nullptr);
On 2015/02/18 02:20:02, enne wrote:
> On 2015/02/18 01:24:10, danakj wrote:
> > what if there is a pending tree, would this happen on both trees
> independently?
> > does that leave us not sharing tiles between the two trees, so they'll
> duplicate
> > raster work and slow down activation?
> 
> This should only ever be called on the sync tree right after a commit, so is
> equivalent to the PictureLayer::UpdateRasterSource call.  I can add DCHECKs
here
> to document those assumptions?

DCHECK is sync tree sounds good.

[Ian Vollick, 2015-02-18 09:12:10]

lg2m % Dana.

This seems pretty reasonable. IIUC, raster sources now have the ability to be
downgraded-to-not-LCD-text-able, and picture_layer_impl's now have the power to
dynamically downgrade their raster source (w/ hysteresis)?

(Also, nice job with the unit tests; I was worried they'd be a bigger pain to
update.)

https://codereview.chromium.org/913203006/diff/1/cc/resources/display_list_ra...
File cc/resources/display_list_raster_source.h (right):

https://codereview.chromium.org/913203006/diff/1/cc/resources/display_list_ra...
cc/resources/display_list_raster_source.h:59: explicit
DisplayListRasterSource(const DisplayListRecordingSource* other,
nit: don't need the explicits.

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (left):

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_host.h#o...
cc/trees/layer_tree_host.h:463: LCDTextMetrics lcd_text_metrics_;
Could this be moved to LTHI now that LCD text stuff is being calculated there?

[enne (OOO), 2015-02-18 19:22:23]

https://codereview.chromium.org/913203006/diff/1/cc/resources/display_list_ra...
File cc/resources/display_list_raster_source.h (right):

https://codereview.chromium.org/913203006/diff/1/cc/resources/display_list_ra...
cc/resources/display_list_raster_source.h:59: explicit
DisplayListRasterSource(const DisplayListRecordingSource* other,
On 2015/02/18 09:12:10, vollick wrote:
> nit: don't need the explicits.

Welcome to C++11!

I'll un-explicit them for consistency, but I half feel like we should just
explicit all of our constructors for ease of review and future-proofing against
uniform initialization (even if we will likely not ever have it in Chromium).

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (left):

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_host.h#o...
cc/trees/layer_tree_host.h:463: LCDTextMetrics lcd_text_metrics_;
On 2015/02/18 09:12:10, vollick wrote:
> Could this be moved to LTHI now that LCD text stuff is being calculated there?

We could, but do you think we need this data still? I'm going to go with no, and
we can add it back in later post-cc-layerization if anybody wants it again.

[enne (OOO), 2015-02-18 19:43:05]

PTAL

On 2015/02/18 at 09:12:10, vollick wrote:

> This seems pretty reasonable. IIUC, raster sources now have the ability to be
downgraded-to-not-LCD-text-able, and picture_layer_impl's now have the power to
dynamically downgrade their raster source (w/ hysteresis)?

Yes, exactly.  Although, it's less about hysteresis (in the sense of just making
it happen less frequently) and more about timing invalidations coming from the
compositor thread to occur at the same time as invalidations from the main
thread so that they don't need to be handled in any special way to prevent
flashing.

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_impl.cc#...
cc/trees/layer_tree_impl.cc:586: // It'd be ideal if this could be done earlier,
but when the raster source
On 2015/02/18 01:25:52, danakj wrote:
> Can you move this out of the ::Occlusion {}? Maybe down with UpdateTiles?

Done.

[danakj, 2015-02-18 19:54:48]

https://codereview.chromium.org/913203006/diff/1/cc/test/fake_picture_pile_im...
File cc/test/fake_picture_pile_impl.cc (right):

https://codereview.chromium.org/913203006/diff/1/cc/test/fake_picture_pile_im...
cc/test/fake_picture_pile_impl.cc:24: : PicturePileImpl(other, true),
/* can_use_lcd_text */ ?

https://codereview.chromium.org/913203006/diff/20001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/913203006/diff/20001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:664: source_frame_number_ !=
last_update_draw_properties_frame_number_) {
Mmmmmmh. What if we instead pass a bool to UDP() to do this walk and only pass
true from
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
? Would that be slightly more obvious?

[Ian Vollick, 2015-02-18 19:58:14]

https://codereview.chromium.org/913203006/diff/1/cc/resources/display_list_ra...
File cc/resources/display_list_raster_source.h (right):

https://codereview.chromium.org/913203006/diff/1/cc/resources/display_list_ra...
cc/resources/display_list_raster_source.h:59: explicit
DisplayListRasterSource(const DisplayListRecordingSource* other,
On 2015/02/18 19:22:23, enne wrote:
> On 2015/02/18 09:12:10, vollick wrote:
> > nit: don't need the explicits.
> 
> Welcome to C++11!
> 
> I'll un-explicit them for consistency, but I half feel like we should just
> explicit all of our constructors for ease of review and future-proofing
against
> uniform initialization (even if we will likely not ever have it in Chromium).

Oh cool! Sorry, I'd assumed this was an oversight and was reviewing on
autopilot. Uniform initialization looks neat.

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (left):

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_host.h#o...
cc/trees/layer_tree_host.h:463: LCDTextMetrics lcd_text_metrics_;
On 2015/02/18 19:22:23, enne wrote:
> On 2015/02/18 09:12:10, vollick wrote:
> > Could this be moved to LTHI now that LCD text stuff is being calculated
there?
> 
> We could, but do you think we need this data still? I'm going to go with no,
and
> we can add it back in later post-cc-layerization if anybody wants it again.

sg. Should the histogram be removed in this case?

[enne (OOO), 2015-02-18 19:59:05]

https://codereview.chromium.org/913203006/diff/1/cc/test/fake_picture_pile_im...
File cc/test/fake_picture_pile_impl.cc (right):

https://codereview.chromium.org/913203006/diff/1/cc/test/fake_picture_pile_im...
cc/test/fake_picture_pile_impl.cc:24: : PicturePileImpl(other, true),
On 2015/02/18 19:54:47, danakj wrote:
> /* can_use_lcd_text */ ?

The value here is irrelevant, so I don't think the comment adds anything.

https://codereview.chromium.org/913203006/diff/20001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/913203006/diff/20001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:664: source_frame_number_ !=
last_update_draw_properties_frame_number_) {
On 2015/02/18 19:54:47, danakj wrote:
> Mmmmmmh. What if we instead pass a bool to UDP() to do this walk and only pass
> true from
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> ? Would that be slightly more obvious?

Since there's UDP called at different times on different trees, I think it seems
safer to keep the frame number on the tree itself.

I also think "last_foo_frame_number" is a well-trodden pattern?

[enne (OOO), 2015-02-18 20:01:44]

enne@chromium.org changed reviewers:
+ isherman@chromium.org

[enne (OOO), 2015-02-18 20:01:45]

+isherman for histogram removal

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (left):

https://codereview.chromium.org/913203006/diff/1/cc/trees/layer_tree_host.h#o...
cc/trees/layer_tree_host.h:463: LCDTextMetrics lcd_text_metrics_;
On 2015/02/18 19:58:13, vollick wrote:
> On 2015/02/18 19:22:23, enne wrote:
> > On 2015/02/18 09:12:10, vollick wrote:
> > > Could this be moved to LTHI now that LCD text stuff is being calculated
> there?
> > 
> > We could, but do you think we need this data still? I'm going to go with no,
> and
> > we can add it back in later post-cc-layerization if anybody wants it again.
> 
> sg. Should the histogram be removed in this case?

Done.

[danakj, 2015-02-18 20:01:50]

On 2015/02/18 19:59:05, enne wrote:
>
https://codereview.chromium.org/913203006/diff/1/cc/test/fake_picture_pile_im...
> File cc/test/fake_picture_pile_impl.cc (right):
> 
>
https://codereview.chromium.org/913203006/diff/1/cc/test/fake_picture_pile_im...
> cc/test/fake_picture_pile_impl.cc:24: : PicturePileImpl(other, true),
> On 2015/02/18 19:54:47, danakj wrote:
> > /* can_use_lcd_text */ ?
> 
> The value here is irrelevant, so I don't think the comment adds anything.
> 
>
https://codereview.chromium.org/913203006/diff/20001/cc/trees/layer_tree_impl.cc
> File cc/trees/layer_tree_impl.cc (right):
> 
>
https://codereview.chromium.org/913203006/diff/20001/cc/trees/layer_tree_impl...
> cc/trees/layer_tree_impl.cc:664: source_frame_number_ !=
> last_update_draw_properties_frame_number_) {
> On 2015/02/18 19:54:47, danakj wrote:
> > Mmmmmmh. What if we instead pass a bool to UDP() to do this walk and only
pass
> > true from
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> > ? Would that be slightly more obvious?
> 
> Since there's UDP called at different times on different trees, I think it
seems
> safer to keep the frame number on the tree itself.
> 
> I also think "last_foo_frame_number" is a well-trodden pattern?

Well, in this case you actually want it to only run in that one call site right?
It should be the first UDP after commit on the sync tree. You guarantee that
over here, but for someone other than me and you they'd have no idea which call
to UDP will be the one to trigger this I think. I'm worried about that.

[enne (OOO), 2015-02-18 21:29:45]

> Well, in this case you actually want it to only run in that one call site
right? It should be the first UDP after commit on the sync tree. You guarantee
that over here, but for someone other than me and you they'd have no idea which
call to UDP will be the one to trigger this I think. I'm worried about that.

Done.

[enne (OOO), 2015-02-18 21:30:26]

On 2015/02/18 at 20:01:45, enne wrote:
> +isherman for histogram removal

isherman: If a histogram isn't being used anymore, should it just get removed
from histograms.xml? Or should I deprecate it?

[Ilya Sherman, 2015-02-18 21:44:00]

Please mark the histograms as <obsolete> rather than removing them from the XML
file.  This has two benefits: (1) it allows previously collected data to remain
viewable; and (2) it reduces the chances that somebody might accidentally reuse
the same histogram name for logging unrelated data in the future.

[danakj, 2015-02-18 21:51:46]

https://codereview.chromium.org/913203006/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/913203006/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:311: bool update_lcd_text = true;
Mind leaving a comment why it's true here?

https://codereview.chromium.org/913203006/diff/60001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/913203006/diff/60001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:662: if (this == sync_tree && update_lcd_text) {
Can DCHECK this == sync_tree when update_lcd_text is true?

https://codereview.chromium.org/913203006/diff/60001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/913203006/diff/60001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:535: bool update_lcd_text = true;
Won't this cause us to check LCD text twice?

[enne (OOO), 2015-02-18 22:16:08]

Changed histograms to just be deprecated and addressed other comments.  PTAL.

https://codereview.chromium.org/913203006/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/913203006/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:311: bool update_lcd_text = true;
On 2015/02/18 21:51:46, danakj wrote:
> Mind leaving a comment why it's true here?

Done.

https://codereview.chromium.org/913203006/diff/60001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/913203006/diff/60001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:662: if (this == sync_tree && update_lcd_text) {
On 2015/02/18 21:51:46, danakj wrote:
> Can DCHECK this == sync_tree when update_lcd_text is true?

Done.

https://codereview.chromium.org/913203006/diff/60001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/913203006/diff/60001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:535: bool update_lcd_text = true;
On 2015/02/18 21:51:46, danakj wrote:
> Won't this cause us to check LCD text twice?

Ok, removed.

[danakj, 2015-02-18 22:20:17]

LGTM

[Ilya Sherman, 2015-02-18 22:20:37]

Histograms LGTM.  Thanks :)

[enne (OOO), 2015-02-18 22:23:32]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2015-02-18 22:28:31]

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

[commit-bot: I haz the power, 2015-02-18 22:37:38]

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

[commit-bot: I haz the power, 2015-02-18 22:37:39]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2015-02-18 23:17:16]

New patchsets have been uploaded after l-g-t-m from
danakj@chromium.org,isherman@chromium.org

[enne (OOO), 2015-02-18 23:17:20]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2015-02-18 23:18:51]

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

[commit-bot: I haz the power, 2015-02-18 23:35:07]

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

[commit-bot: I haz the power, 2015-02-18 23:35:08]

Try jobs failed on following builders:
  android_rel_tests_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_rel_tes...)

[enne (OOO), 2015-02-18 23:42:53]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2015-02-18 23:44:03]

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

[commit-bot: I haz the power, 2015-02-19 01:28:02]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-19 01:28:46]

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