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

Issue 2717533005: cc: RenderSurfaceImpl tile mask layer. (Closed)

Created:
3 years, 10 months ago by sunxd
Modified:
3 years, 8 months ago
CC:
cc-bugs_chromium.org, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

cc: RenderSurfaceImpl tile mask layer. This is the core change of mask tiling. In this CL, we use PictureLayerImpl::AppendQuads logic to generate Tiled Quads out of mask layer, and convert them into RPDQs. This change involves several space conversions and can be confusing. But we tried our best to clarify spaces by naming them correctly. This CL depends on the renderer change and should be commited after that. BUG=567293 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2717533005 Cr-Commit-Position: refs/heads/master@{#462215} Committed: https://chromium.googlesource.com/chromium/src/+/bb0b3ae9224e1883dd491b6b6810b6bfe0da8480

Patch Set 1 #

Patch Set 2 : Disable mask tiling again. #

Patch Set 3 : Fixing unit tests. #

Patch Set 4 : Fix unit tests. #

Total comments: 6

Patch Set 5 : Addressing comments. #

Total comments: 2

Patch Set 6 : Paint flags change. #

Patch Set 7 : Remove log statements. #

Patch Set 8 : Rebase #

Patch Set 9 : Fix init problems on mac. #

Total comments: 25

Patch Set 10 : Nit change in RSI. #

Patch Set 11 : Add a unit test #

Patch Set 12 : Prevent single_texture_mask to multi_texture_mask conversion. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+388 lines, -66 lines) Patch
M cc/layers/picture_layer.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -0 lines 0 comments Download
M cc/layers/render_surface_impl.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -0 lines 0 comments Download
M cc/layers/render_surface_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +130 lines, -9 lines 0 comments Download
M cc/layers/render_surface_impl_unittest.cc View 1 2 4 chunks +8 lines, -3 lines 0 comments Download
M cc/output/gl_renderer.cc View 1 2 3 4 5 6 7 8 5 chunks +8 lines, -6 lines 0 comments Download
M cc/output/renderer_pixeltest.cc View 1 2 3 4 5 6 7 8 9 10 7 chunks +20 lines, -20 lines 0 comments Download
M cc/output/software_renderer.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -5 lines 0 comments Download
M cc/test/fake_mask_layer_impl.h View 1 2 3 4 5 6 7 1 chunk +12 lines, -5 lines 0 comments Download
M cc/test/fake_mask_layer_impl.cc View 1 2 1 chunk +14 lines, -4 lines 0 comments Download
M cc/trees/draw_property_utils.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +6 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 8 chunks +181 lines, -14 lines 0 comments Download

Messages

Total messages: 52 (20 generated)
sunxd
I think there is still one comment from the original CL that I did not ...
3 years, 10 months ago (2017-02-24 20:04:20 UTC) #3
sunxd
PTAL
3 years, 9 months ago (2017-02-28 18:51:57 UTC) #4
trchen
https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surface_impl.cc File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surface_impl.cc#newcode414 cc/layers/render_surface_impl.cc:414: if (Filters().IsEmpty() && I think the Filters().IsEmpty() condition should ...
3 years, 9 months ago (2017-03-02 01:00:30 UTC) #5
sunxd
PTAL. https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surface_impl.cc File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surface_impl.cc#newcode414 cc/layers/render_surface_impl.cc:414: if (Filters().IsEmpty() && On 2017/03/02 01:00:30, trchen wrote: ...
3 years, 9 months ago (2017-03-02 19:20:54 UTC) #6
trchen
lgtm. WDYT, enne?
3 years, 9 months ago (2017-03-04 01:13:25 UTC) #7
sunxd
https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surface_impl.cc File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surface_impl.cc#newcode434 cc/layers/render_surface_impl.cc:434: gfx::RectF()); In https://codereview.chromium.org/2716893002/, enne@ suggested keeping all tex rect ...
3 years, 9 months ago (2017-03-08 19:57:46 UTC) #10
trchen
On 2017/03/08 19:57:46, sunxd wrote: > https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surface_impl.cc > File cc/layers/render_surface_impl.cc (right): > > https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surface_impl.cc#newcode434 > ...
3 years, 9 months ago (2017-03-08 22:23:35 UTC) #11
Stephen White
https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surface_impl.cc File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surface_impl.cc#newcode434 cc/layers/render_surface_impl.cc:434: gfx::RectF()); On 2017/03/08 19:57:46, sunxd wrote: > In https://codereview.chromium.org/2716893002/, ...
3 years, 9 months ago (2017-03-09 01:05:08 UTC) #12
sunxd
I took Tien-Ren's suggestion that we assign a tex_coord_rect in RSI anyways, and later in ...
3 years, 9 months ago (2017-03-14 16:10:22 UTC) #15
sunxd
Nit change.
3 years, 9 months ago (2017-03-14 16:14:48 UTC) #16
trchen
It feels to me that DrawRenderPassDrawQuadParams::dst_rect pretty much equivalent to tex_coord_rect. Could you briefly explain ...
3 years, 9 months ago (2017-03-14 22:51:18 UTC) #17
sunxd
On 2017/03/14 22:51:18, trchen wrote: > It feels to me that DrawRenderPassDrawQuadParams::dst_rect pretty much > ...
3 years, 9 months ago (2017-03-15 14:31:18 UTC) #18
sunxd
I have updated the CL so that in RSI we try to assign a tex_coord_rect ...
3 years, 9 months ago (2017-03-27 14:01:12 UTC) #21
sunxd
On mac, we init DrawRPDQParams in another code path.
3 years, 8 months ago (2017-03-28 13:43:21 UTC) #26
enne (OOO)
Looks good in general. Just have a bunch of testing comments: https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surface_impl.cc File cc/layers/render_surface_impl.cc (right): ...
3 years, 8 months ago (2017-03-28 14:10:22 UTC) #27
sunxd
PTAL https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surface_impl.cc File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surface_impl.cc#newcode430 cc/layers/render_surface_impl.cc:430: gfx::RectF tex_coord_rect(0, 0, content_rect().width(), On 2017/03/28 14:10:22, enne ...
3 years, 8 months ago (2017-03-29 16:00:24 UTC) #30
enne (OOO)
https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surface_impl.cc File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surface_impl.cc#newcode494 cc/layers/render_surface_impl.cc:494: -content_rect().OffsetFromOrigin()); On 2017/03/29 at 16:00:23, sunxd wrote: > On ...
3 years, 8 months ago (2017-03-29 16:07:25 UTC) #31
sunxd
I haven't finish all the adding a test comments, but here are some discussions. https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surface_impl.cc ...
3 years, 8 months ago (2017-03-30 13:56:37 UTC) #32
enne (OOO)
https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surface_impl.cc File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surface_impl.cc#newcode494 cc/layers/render_surface_impl.cc:494: -content_rect().OffsetFromOrigin()); On 2017/03/30 at 13:56:37, sunxd wrote: > On ...
3 years, 8 months ago (2017-03-30 14:36:52 UTC) #33
sunxd
I wrote a unit test that tests content_rect.origin not zero case. https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surface_impl.cc File cc/layers/render_surface_impl.cc (right): ...
3 years, 8 months ago (2017-04-04 13:58:04 UTC) #34
enne (OOO)
On 2017/04/04 at 13:58:04, sunxd wrote: > As this change blocks AA patch, can I ...
3 years, 8 months ago (2017-04-04 16:48:10 UTC) #35
enne (OOO)
On 2017/04/04 at 16:48:10, enne wrote: > On 2017/04/04 at 13:58:04, sunxd wrote: > > ...
3 years, 8 months ago (2017-04-04 16:50:08 UTC) #36
sunxd
On 2017/04/04 16:50:08, enne wrote: > On 2017/04/04 at 16:48:10, enne wrote: > > On ...
3 years, 8 months ago (2017-04-04 17:27:11 UTC) #37
enne (OOO)
Recording what I said offline in chat, I think it's fine to restrict tiled mask ...
3 years, 8 months ago (2017-04-05 01:14:16 UTC) #38
sunxd
On 2017/04/05 01:14:16, enne wrote: > Recording what I said offline in chat, I think ...
3 years, 8 months ago (2017-04-05 14:59:04 UTC) #39
enne (OOO)
Ok, thanks. lgtm in general, and lgtm to follow up with tests since this is ...
3 years, 8 months ago (2017-04-05 17:42:46 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/2717533005/220001
3 years, 8 months ago (2017-04-05 18:00:51 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/270293)
3 years, 8 months ago (2017-04-05 18:56:01 UTC) #45
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/2717533005/220001
3 years, 8 months ago (2017-04-05 21:04:21 UTC) #47
commit-bot: I haz the power
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/bb0b3ae9224e1883dd491b6b6810b6bfe0da8480
3 years, 8 months ago (2017-04-05 21:14:21 UTC) #50
drbasic
On 2017/04/05 21:14:21, commit-bot: I haz the power wrote: > Committed patchset #12 (id:220001) as ...
3 years, 8 months ago (2017-04-08 15:16:38 UTC) #51
sunxd
3 years, 8 months ago (2017-04-10 13:51:41 UTC) #52
Message was sent while issue was closed.
On 2017/04/08 15:16:38, drbasic wrote:
> On 2017/04/05 21:14:21, commit-bot: I haz the power wrote:
> > Committed patchset #12 (id:220001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/bb0b3ae9224e1883dd491b6b6810...
> 
> sunxd@ Why in picture_layer.cc line 152 used LayerMaskType::MULTI_TEXTURE_MASK
?

Oops, I've uploaded another patch to fix this.

Powered by Google App Engine
This is Rietveld 408576698