Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(733)

Issue 1531013004: cc: Do solid color analysis before scheduling tiles. (Closed)

Created:
5 years ago by sohanjg
Modified:
4 years, 11 months ago
Reviewers:
vmpstr, ericrk
CC:
chromium-reviews, cc-bugs_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

cc: Do solid color analysis before scheduling tiles. Instead of creating separate analysis task in raster thread for solid color detection, we do it while scheduling and rasterization in cc thread itself. This would save us the thread overhead etc. BUG=553612 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/5b2f22a73e08684db17a2b86a4c2db2ae6ba81ee Cr-Commit-Position: refs/heads/master@{#369263}

Patch Set 1 #

Total comments: 8

Patch Set 2 : move tile check while assigning mem. #

Total comments: 2

Patch Set 3 : cleanup. #

Total comments: 10

Patch Set 4 : review comments updated. #

Total comments: 2

Patch Set 5 : add test. #

Total comments: 8

Patch Set 6 : review comments updated. #

Total comments: 1

Patch Set 7 : use non-solid color for ActivateAndDrawWhenOOM test. #

Total comments: 10

Patch Set 8 : notifytilestate change before skipping raster. #

Total comments: 3

Patch Set 9 : test updated. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+166 lines, -146 lines) Patch
M cc/debug/rasterize_and_record_benchmark_impl.cc View 1 2 3 4 5 1 chunk +4 lines, -5 lines 0 comments Download
M cc/playback/display_list_raster_source.h View 1 2 3 4 5 2 chunks +4 lines, -13 lines 0 comments Download
M cc/playback/display_list_raster_source.cc View 1 2 3 4 5 2 chunks +3 lines, -4 lines 0 comments Download
M cc/playback/display_list_raster_source_unittest.cc View 1 2 3 4 5 6 7 7 chunks +67 lines, -57 lines 0 comments Download
M cc/raster/tile_task_worker_pool_unittest.cc View 1 2 3 4 5 3 chunks +2 lines, -6 lines 0 comments Download
M cc/tiles/tile.h View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M cc/tiles/tile_manager.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -5 lines 0 comments Download
M cc/tiles/tile_manager.cc View 1 2 3 4 5 6 7 10 chunks +25 lines, -54 lines 0 comments Download
M cc/tiles/tile_manager_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +59 lines, -0 lines 0 comments Download

Messages

Total messages: 44 (14 generated)
sohanjg
Vlad@, sorry got caught up elsewhere. Can you please have a look, if this is ...
5 years ago (2015-12-17 12:12:13 UTC) #3
vmpstr
https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_raster_source.cc File cc/playback/display_list_raster_source.cc (right): https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_raster_source.cc#newcode220 cc/playback/display_list_raster_source.cc:220: bool DisplayListRasterSource::IsSolidColorTile(gfx::Rect content_rect, Can you use PerformSolidColorAnalysis instead? That ...
5 years ago (2015-12-17 18:55:47 UTC) #4
sohanjg
Please take a look, thanks. https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_raster_source.cc File cc/playback/display_list_raster_source.cc (right): https://codereview.chromium.org/1531013004/diff/1/cc/playback/display_list_raster_source.cc#newcode220 cc/playback/display_list_raster_source.cc:220: bool DisplayListRasterSource::IsSolidColorTile(gfx::Rect content_rect, On ...
5 years ago (2015-12-18 12:50:00 UTC) #5
vmpstr
There are a bunch of things that could be cleaned up in this patch as ...
5 years ago (2015-12-18 19:03:06 UTC) #7
sohanjg
On 2015/12/18 19:03:06, vmpstr wrote: > There are a bunch of things that could be ...
5 years ago (2015-12-23 12:26:13 UTC) #8
vmpstr
I think it's worth cleaning this patch up and landing it. I would still prefer ...
4 years, 12 months ago (2015-12-28 21:22:32 UTC) #9
sohanjg
Please take a look. We can cleanup SolidColorAnalysis usage completely, should we do it here ...
4 years, 11 months ago (2015-12-29 14:29:47 UTC) #11
vmpstr
On 2015/12/29 14:29:47, sohanjg wrote: > Please take a look. > We can cleanup SolidColorAnalysis ...
4 years, 11 months ago (2015-12-29 21:24:23 UTC) #12
sohanjg
Please take a look, thanks. https://codereview.chromium.org/1531013004/diff/60001/cc/debug/rasterize_and_record_benchmark_impl.cc File cc/debug/rasterize_and_record_benchmark_impl.cc (right): https://codereview.chromium.org/1531013004/diff/60001/cc/debug/rasterize_and_record_benchmark_impl.cc#newcode52 cc/debug/rasterize_and_record_benchmark_impl.cc:52: *is_solid_color = raster_source->PerformSolidColorAnalysis( On ...
4 years, 11 months ago (2015-12-30 12:45:48 UTC) #13
vmpstr
This looks good. Can you see if you add a test somewhere that ensures that ...
4 years, 11 months ago (2015-12-30 20:42:38 UTC) #14
sohanjg
Sorry for the delay on this, was OOO. Please take a look, thanks. https://codereview.chromium.org/1531013004/diff/80001/cc/tiles/tile_manager.cc File ...
4 years, 11 months ago (2016-01-06 11:50:27 UTC) #15
vmpstr
Can you describe the CL a bit more in the description field please? https://codereview.chromium.org/1531013004/diff/100001/cc/playback/display_list_raster_source.h File ...
4 years, 11 months ago (2016-01-06 19:35:03 UTC) #16
sohanjg
Please take a look, thanks. https://codereview.chromium.org/1531013004/diff/100001/cc/playback/display_list_raster_source.h File cc/playback/display_list_raster_source.h (right): https://codereview.chromium.org/1531013004/diff/100001/cc/playback/display_list_raster_source.h#newcode50 cc/playback/display_list_raster_source.h:50: bool PerformSolidColorAnalysis(const gfx::Rect& content_rect, ...
4 years, 11 months ago (2016-01-07 10:13:00 UTC) #18
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531013004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531013004/120001
4 years, 11 months ago (2016-01-07 10:13:55 UTC) #20
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/98755)
4 years, 11 months ago (2016-01-07 10:55:49 UTC) #22
sohanjg
Can you give your opinion for this ? https://codereview.chromium.org/1531013004/diff/120001/cc/tiles/tile_manager_unittest.cc File cc/tiles/tile_manager_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/120001/cc/tiles/tile_manager_unittest.cc#newcode1480 cc/tiles/tile_manager_unittest.cc:1480: PictureLayerTiling* ...
4 years, 11 months ago (2016-01-07 12:32:16 UTC) #23
sohanjg
I have updated the ActivateAndDrawWhenOOM test, it was getting solid color src and notifytilestate was ...
4 years, 11 months ago (2016-01-07 14:51:45 UTC) #24
ericrk
https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/1531013004/diff/140001/cc/tiles/tile_manager.cc#newcode576 cc/tiles/tile_manager.cc:576: prioritized_tile.raster_source()->PerformSolidColorAnalysis( There's a comment in the bug that we ...
4 years, 11 months ago (2016-01-07 18:18:11 UTC) #26
vmpstr
https://codereview.chromium.org/1531013004/diff/140001/cc/playback/display_list_raster_source_unittest.cc File cc/playback/display_list_raster_source_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/140001/cc/playback/display_list_raster_source_unittest.cc#newcode67 cc/playback/display_list_raster_source_unittest.cc:67: is_solid_color = false; nit: you don't need to reset ...
4 years, 11 months ago (2016-01-07 22:36:02 UTC) #27
sohanjg
Please take a look, thanks. https://codereview.chromium.org/1531013004/diff/140001/cc/playback/display_list_raster_source_unittest.cc File cc/playback/display_list_raster_source_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/140001/cc/playback/display_list_raster_source_unittest.cc#newcode67 cc/playback/display_list_raster_source_unittest.cc:67: is_solid_color = false; On ...
4 years, 11 months ago (2016-01-08 11:47:57 UTC) #28
vmpstr
lgtm % ericrk https://codereview.chromium.org/1531013004/diff/160001/cc/tiles/tile_manager_unittest.cc File cc/tiles/tile_manager_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/160001/cc/tiles/tile_manager_unittest.cc#newcode1480 cc/tiles/tile_manager_unittest.cc:1480: PictureLayerTiling* tiling = tiling_set->AddTiling(1.0f, raster_source); On ...
4 years, 11 months ago (2016-01-08 18:51:41 UTC) #29
sohanjg
Updated the test, can you please take a look, thanks. https://codereview.chromium.org/1531013004/diff/160001/cc/tiles/tile_manager_unittest.cc File cc/tiles/tile_manager_unittest.cc (right): https://codereview.chromium.org/1531013004/diff/160001/cc/tiles/tile_manager_unittest.cc#newcode1480 ...
4 years, 11 months ago (2016-01-11 11:46:59 UTC) #30
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531013004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531013004/180001
4 years, 11 months ago (2016-01-11 11:47:15 UTC) #32
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-11 12:49:00 UTC) #34
sohanjg
On 2016/01/11 12:49:00, commit-bot: I haz the power wrote: > Dry run: This issue passed ...
4 years, 11 months ago (2016-01-12 05:07:17 UTC) #35
ericrk
On 2016/01/12 05:07:17, sohanjg wrote: > On 2016/01/11 12:49:00, commit-bot: I haz the power wrote: ...
4 years, 11 months ago (2016-01-12 22:12:58 UTC) #36
sohanjg
On 2016/01/12 22:12:58, ericrk wrote: > On 2016/01/12 05:07:17, sohanjg wrote: > > On 2016/01/11 ...
4 years, 11 months ago (2016-01-13 16:36:03 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531013004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531013004/180001
4 years, 11 months ago (2016-01-13 19:23:31 UTC) #40
commit-bot: I haz the power
Committed patchset #9 (id:180001)
4 years, 11 months ago (2016-01-13 20:38:48 UTC) #42
commit-bot: I haz the power
4 years, 11 months ago (2016-01-13 20:40:04 UTC) #44
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/5b2f22a73e08684db17a2b86a4c2db2ae6ba81ee
Cr-Commit-Position: refs/heads/master@{#369263}

Powered by Google App Engine
This is Rietveld 408576698