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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 2 weeks ago by Khushal
Modified:
5 months, 4 weeks ago
Reviewers:
vmpstr, enne (OOO)
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 ...
6 months, 2 weeks ago (2017-02-01 00:16:31 UTC) #3
Khushal
pingy.
6 months, 2 weeks ago (2017-02-03 21:24:24 UTC) #4
enne (OOO)
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 ...
6 months, 2 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> ...
6 months, 2 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: ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week ago (2017-02-10 22:09:19 UTC) #9
Khushal
And this should have all the tests now too.
6 months 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 ...
6 months 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, ...
6 months 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()) ...
6 months ago (2017-02-16 20:59:14 UTC) #21
enne (OOO)
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 ...
6 months 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: ...
6 months 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, ...
6 months 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 ...
6 months 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 ...
6 months ago (2017-02-17 19:10:29 UTC) #27
enne (OOO)
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 ...
6 months 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 > ...
6 months ago (2017-02-17 19:20:56 UTC) #29
Khushal
Sounds good. New patch it is for the skia/ change. -thakis
6 months 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
6 months 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)
6 months 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
6 months 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 ...
6 months 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
6 months 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 ...
6 months 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
5 months, 4 weeks ago (2017-02-18 18:37:51 UTC) #46
commit-bot: I haz the power
5 months, 4 weeks 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 b40b6558b