Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(9)

Issue 2668873002: cc: Add checker-imaging support to TileManager. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 3 weeks ago by Khushal
Modified:
4 months, 1 week ago
Reviewers:
vmpstr, enne
CC:
cc-bugs_chromium.org, chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

cc: Add checker-imaging support to TileManager. This change adds the CheckerImageManager to selectively defer image decodes to the Image Decode Service and not block raster for dependent tiles on these decodes. The CheckerImageTracker does the work of filtering these images, requesting decodes and requesting impl-side invalidations once the image decode is finished. BUG=686267 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2668873002 Cr-Commit-Position: refs/heads/master@{#451482} Committed: https://chromium.googlesource.com/chromium/src/+/d3b8827d06088880374054960d4285a9f0c754c6

Patch Set 1 #

Total comments: 45

Patch Set 2 : addressed comments. #

Patch Set 3 : Rebase #

Total comments: 44

Patch Set 4 : Rebase #

Patch Set 5 : addressed comments #

Patch Set 6 : all tests #

Total comments: 36

Patch Set 7 : addressed comments #

Patch Set 8 : rebase #

Total comments: 19

Patch Set 9 : addressed comments #

Total comments: 4

Patch Set 10 : addressed comments #

Patch Set 11 : remove include #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1076 lines, -32 lines) Patch
M cc/BUILD.gn View 1 2 3 4 3 chunks +4 lines, -0 lines 0 comments Download
M cc/layers/picture_layer_impl.h View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -0 lines 0 comments Download
M cc/layers/picture_layer_impl.cc View 1 2 3 4 5 6 7 8 1 chunk +23 lines, -0 lines 0 comments Download
M cc/playback/image_hijack_canvas.h View 1 2 3 4 3 chunks +9 lines, -1 line 0 comments Download
M cc/playback/image_hijack_canvas.cc View 1 2 3 4 9 chunks +42 lines, -2 lines 0 comments Download
M cc/playback/image_hijack_canvas_unittest.cc View 1 2 3 4 5 3 chunks +21 lines, -1 line 0 comments Download
M cc/playback/image_id.h View 1 2 3 4 5 6 7 8 1 chunk +6 lines, -0 lines 0 comments Download
M cc/playback/raster_source.h View 1 2 3 4 2 chunks +9 lines, -0 lines 0 comments Download
M cc/playback/raster_source.cc View 1 2 3 4 2 chunks +9 lines, -2 lines 0 comments Download
M cc/test/fake_layer_tree_host_impl.h View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
M cc/test/fake_layer_tree_host_impl.cc View 1 2 3 4 5 2 chunks +11 lines, -1 line 0 comments Download
M cc/test/fake_layer_tree_host_impl_client.h View 1 chunk +1 line, -0 lines 0 comments Download
M cc/test/fake_tile_manager.cc View 1 1 chunk +1 line, -2 lines 0 comments Download
M cc/test/fake_tile_manager_client.h View 1 chunk +1 line, -0 lines 0 comments Download
A cc/tiles/checker_image_tracker.h View 1 2 3 4 5 6 7 8 1 chunk +100 lines, -0 lines 0 comments Download
A cc/tiles/checker_image_tracker.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +152 lines, -0 lines 0 comments Download
A cc/tiles/checker_image_tracker_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +296 lines, -0 lines 0 comments Download
M cc/tiles/image_controller.h View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -1 line 0 comments Download
M cc/tiles/image_controller.cc View 1 2 3 4 5 6 9 1 chunk +2 lines, -2 lines 0 comments Download
M cc/tiles/picture_layer_tiling.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M cc/tiles/picture_layer_tiling_set.h View 1 chunk +2 lines, -0 lines 0 comments Download
M cc/tiles/picture_layer_tiling_set.cc View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
M cc/tiles/tile_manager.h View 1 2 3 4 5 6 7 8 8 chunks +19 lines, -6 lines 0 comments Download
M cc/tiles/tile_manager.cc View 1 2 3 4 5 6 7 8 7 chunks +29 lines, -8 lines 0 comments Download
A cc/tiles/tile_manager_settings.h View 1 2 3 4 5 6 7 8 1 chunk +20 lines, -0 lines 0 comments Download
M cc/tiles/tile_manager_unittest.cc View 1 2 3 4 5 6 7 4 chunks +120 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_host_impl.h View 1 2 3 4 5 6 7 4 chunks +12 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_host_impl.cc View 1 2 3 4 5 6 7 6 chunks +22 lines, -4 lines 0 comments Download
M cc/trees/layer_tree_host_impl_unittest.cc View 1 2 3 4 5 6 7 8 chunks +99 lines, -2 lines 0 comments Download
M cc/trees/layer_tree_impl.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_impl.cc View 1 2 3 4 5 6 7 8 1 chunk +11 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_settings.h View 1 2 3 4 5 6 7 3 chunks +7 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_settings.cc View 1 2 3 4 5 6 7 1 chunk +9 lines, -0 lines 0 comments Download
M cc/trees/proxy_impl.h View 1 chunk +1 line, -0 lines 0 comments Download
M cc/trees/proxy_impl.cc View 1 2 3 4 5 6 7 8 1 chunk +7 lines, -0 lines 0 comments Download
M cc/trees/single_thread_proxy.h View 1 chunk +1 line, -0 lines 0 comments Download
M cc/trees/single_thread_proxy.cc View 1 2 3 4 5 6 7 8 1 chunk +6 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 49 (22 generated)
Khushal
Still need to add tests for this but this pulls out and cleans up bulk ...
4 months, 3 weeks ago (2017-02-01 00:16:31 UTC) #3
Khushal
pingy.
4 months, 3 weeks ago (2017-02-03 21:24:24 UTC) #4
enne
https://codereview.chromium.org/2668873002/diff/1/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2668873002/diff/1/cc/trees/layer_tree_host_impl.cc#newcode241 cc/trees/layer_tree_host_impl.cc:241: settings.enable_checker_imaging), Maybe we should just pass settings through at ...
4 months, 3 weeks ago (2017-02-03 23:07:40 UTC) #5
vmpstr
https://codereview.chromium.org/2668873002/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2668873002/diff/1/cc/layers/picture_layer_impl.cc#newcode1388 cc/layers/picture_layer_impl.cc:1388: layer_tree_impl()->AddLayerShouldPushProperties(this); SetNeedsPushProperties(); https://codereview.chromium.org/2668873002/diff/1/cc/playback/raster_source.h File cc/playback/raster_source.h (right): https://codereview.chromium.org/2668873002/diff/1/cc/playback/raster_source.h#newcode50 cc/playback/raster_source.h:50: std::unordered_set<ImageId> ...
4 months, 3 weeks ago (2017-02-03 23:42:33 UTC) #6
Khushal
Tests in progress. https://codereview.chromium.org/2668873002/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2668873002/diff/1/cc/layers/picture_layer_impl.cc#newcode1388 cc/layers/picture_layer_impl.cc:1388: layer_tree_impl()->AddLayerShouldPushProperties(this); On 2017/02/03 23:42:31, vmpstr wrote: ...
4 months, 2 weeks ago (2017-02-07 00:25:33 UTC) #7
vmpstr
https://codereview.chromium.org/2668873002/diff/40001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2668873002/diff/40001/cc/layers/picture_layer_impl.cc#newcode1379 cc/layers/picture_layer_impl.cc:1379: if (image_invalidation.IsEmpty()) Should we just check if (images_to_invalidate.empty()) and ...
4 months, 2 weeks ago (2017-02-10 19:25:43 UTC) #8
Khushal
https://codereview.chromium.org/2668873002/diff/40001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2668873002/diff/40001/cc/layers/picture_layer_impl.cc#newcode1379 cc/layers/picture_layer_impl.cc:1379: if (image_invalidation.IsEmpty()) On 2017/02/10 19:25:42, vmpstr wrote: > Should ...
4 months, 2 weeks ago (2017-02-10 22:09:19 UTC) #9
Khushal
And this should have all the tests now too.
4 months, 2 weeks ago (2017-02-13 21:30:03 UTC) #10
vmpstr
https://codereview.chromium.org/2668873002/diff/100001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2668873002/diff/100001/cc/layers/picture_layer_impl.cc#newcode1380 cc/layers/picture_layer_impl.cc:1380: TRACE_EVENT_END1("cc", "PictureLayerImpl::InvalidateRegionForImages", Hmm, this doesn't capture the duration of ...
4 months, 2 weeks ago (2017-02-13 22:49:31 UTC) #11
Khushal
https://codereview.chromium.org/2668873002/diff/100001/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2668873002/diff/100001/cc/layers/picture_layer_impl.cc#newcode1380 cc/layers/picture_layer_impl.cc:1380: TRACE_EVENT_END1("cc", "PictureLayerImpl::InvalidateRegionForImages", On 2017/02/13 22:49:30, vmpstr wrote: > Hmm, ...
4 months, 1 week ago (2017-02-14 05:10:23 UTC) #12
vmpstr
lgtm % an extra unittest and enne's review https://codereview.chromium.org/2668873002/diff/140001/cc/tiles/checker_image_tracker.cc File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2668873002/diff/140001/cc/tiles/checker_image_tracker.cc#newcode52 cc/tiles/checker_image_tracker.cc:52: DCHECK(invalidated_images_on_current_sync_tree_.empty()) ...
4 months, 1 week ago (2017-02-16 20:59:14 UTC) #21
enne
lgtm https://codereview.chromium.org/2668873002/diff/140001/cc/tiles/checker_image_tracker.cc File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2668873002/diff/140001/cc/tiles/checker_image_tracker.cc#newcode43 cc/tiles/checker_image_tracker.cc:43: it = images->erase(it); std::vector::erase <_< How about: swap ...
4 months, 1 week ago (2017-02-16 21:41:32 UTC) #22
vmpstr
https://codereview.chromium.org/2668873002/diff/140001/cc/tiles/checker_image_tracker.cc File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2668873002/diff/140001/cc/tiles/checker_image_tracker.cc#newcode119 cc/tiles/checker_image_tracker.cc:119: base::CheckedNumeric<size_t> checked_size = 4; On 2017/02/16 21:41:32, enne wrote: ...
4 months, 1 week ago (2017-02-16 22:01:27 UTC) #23
Khushal
+thakis for skia/ext. https://codereview.chromium.org/2668873002/diff/140001/cc/tiles/checker_image_tracker.cc File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2668873002/diff/140001/cc/tiles/checker_image_tracker.cc#newcode43 cc/tiles/checker_image_tracker.cc:43: it = images->erase(it); On 2017/02/16 21:41:32, ...
4 months, 1 week ago (2017-02-17 19:05:39 UTC) #25
vmpstr
https://codereview.chromium.org/2668873002/diff/160001/skia/ext/skia_utils_base.cc File skia/ext/skia_utils_base.cc (right): https://codereview.chromium.org/2668873002/diff/160001/skia/ext/skia_utils_base.cc#newcode105 skia/ext/skia_utils_base.cc:105: size_t SafeSizeOfImage(const SkImage* image) { Can you extract this ...
4 months, 1 week ago (2017-02-17 19:08:49 UTC) #26
Khushal
https://codereview.chromium.org/2668873002/diff/160001/skia/ext/skia_utils_base.cc File skia/ext/skia_utils_base.cc (right): https://codereview.chromium.org/2668873002/diff/160001/skia/ext/skia_utils_base.cc#newcode105 skia/ext/skia_utils_base.cc:105: size_t SafeSizeOfImage(const SkImage* image) { On 2017/02/17 19:08:49, vmpstr ...
4 months, 1 week ago (2017-02-17 19:10:29 UTC) #27
enne
lgtm https://codereview.chromium.org/2668873002/diff/140001/cc/tiles/checker_image_tracker.h File cc/tiles/checker_image_tracker.h (right): https://codereview.chromium.org/2668873002/diff/140001/cc/tiles/checker_image_tracker.h#newcode52 cc/tiles/checker_image_tracker.h:52: const std::unordered_set<ImageId>& TakeImagesToInvalidateOnSyncTree(); On 2017/02/17 at 19:05:38, Khushal ...
4 months, 1 week ago (2017-02-17 19:12:51 UTC) #28
vmpstr
On 2017/02/17 19:10:29, Khushal wrote: > https://codereview.chromium.org/2668873002/diff/160001/skia/ext/skia_utils_base.cc > File skia/ext/skia_utils_base.cc (right): > > https://codereview.chromium.org/2668873002/diff/160001/skia/ext/skia_utils_base.cc#newcode105 > ...
4 months, 1 week ago (2017-02-17 19:20:56 UTC) #29
Khushal
Sounds good. New patch it is for the skia/ change. -thakis
4 months, 1 week ago (2017-02-17 19:34:30 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2668873002/200001
4 months, 1 week ago (2017-02-17 19:36:08 UTC) #34
commit-bot: I haz the power
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/312190)
4 months, 1 week ago (2017-02-17 20:38:27 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2668873002/200001
4 months, 1 week ago (2017-02-17 22:08:04 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on ...
4 months, 1 week ago (2017-02-18 00:11:39 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2668873002/200001
4 months, 1 week ago (2017-02-18 00:13:19 UTC) #42
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on ...
4 months, 1 week ago (2017-02-18 02:15:57 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2668873002/200001
4 months, 1 week ago (2017-02-18 18:37:51 UTC) #46
commit-bot: I haz the power
4 months, 1 week ago (2017-02-18 18:43:38 UTC) #49
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/d3b8827d06088880374054960d42...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318