|
|
Descriptioncc: 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. #
Messages
Total messages: 52 (20 generated)
Description was changed from ========== 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 ========== to ========== 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 ==========
sunxd@chromium.org changed reviewers: + enne@chromium.org, trchen@chromium.org
I think there is still one comment from the original CL that I did not resolve, but I would like to send this to code review just for clearance. Our test case did not cover the AA problem, all layout tests are passed if I turn on mask tiling.
PTAL
https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surfac... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surfac... cc/layers/render_surface_impl.cc:414: if (Filters().IsEmpty() && I think the Filters().IsEmpty() condition should be removed. It doesn't matter whether there is a filter or not. If the mask layer is tiled, we should use the tiled code path; if not, we should use the non-tiled code path. If the other part of the system gave us a tiled mask with filtered content despite we don't support it, we should catch it and fail, as we already did on #440. We should never garbage-in garbage-out. https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surfac... cc/layers/render_surface_impl.cc:423: mask_uv_rect = gfx::ScaleRect(gfx::RectF(content_rect()), This looks correct, but a comment like this should help: // mask_uv_rect = RectToRect(RectF(PointF(), unclipped_mask_target_size), RectF(0, 0, 1, 1)) * content_rect(); https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surfac... cc/layers/render_surface_impl.cc:525: SkAlpha solid = 255; Use SK_AlphaOPAQUE instead.
PTAL. https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surfac... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surfac... cc/layers/render_surface_impl.cc:414: if (Filters().IsEmpty() && On 2017/03/02 01:00:30, trchen wrote: > I think the Filters().IsEmpty() condition should be removed. > It doesn't matter whether there is a filter or not. If the mask layer is tiled, > we should use the tiled code path; if not, we should use the non-tiled code > path. If the other part of the system gave us a tiled mask with filtered > content despite we don't support it, we should catch it and fail, as we already > did on #440. We should never garbage-in garbage-out. Done. https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surfac... cc/layers/render_surface_impl.cc:423: mask_uv_rect = gfx::ScaleRect(gfx::RectF(content_rect()), On 2017/03/02 01:00:30, trchen wrote: > This looks correct, but a comment like this should help: > // mask_uv_rect = RectToRect(RectF(PointF(), unclipped_mask_target_size), > RectF(0, 0, 1, 1)) * content_rect(); Done. https://codereview.chromium.org/2717533005/diff/60001/cc/layers/render_surfac... cc/layers/render_surface_impl.cc:525: SkAlpha solid = 255; On 2017/03/02 01:00:30, trchen wrote: > Use SK_AlphaOPAQUE instead. Done.
lgtm. WDYT, enne?
Description was changed from ========== 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 ========== to ========== 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 ==========
sunxd@chromium.org changed reviewers: + senorblanco@chromium.org
https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surfac... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surfac... cc/layers/render_surface_impl.cc:434: gfx::RectF()); In https://codereview.chromium.org/2716893002/, enne@ suggested keeping all tex rect logic either in the renderer or render surface impl. As we need to deal with multiple spaces when computing tex coord rect for a tiled mask, I think at least in this case the logic should reside in render surface impl. However, in other cases (no mask or mask not tilable), the tex rect is set as a result of ApplyImageFilter in the renderers, I'm not sure if we have enough information in render surface impl. @senorblanco, do you have suggestions here? Thanks!
On 2017/03/08 19:57:46, sunxd wrote: > https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surfac... > File cc/layers/render_surface_impl.cc (right): > > https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surfac... > cc/layers/render_surface_impl.cc:434: gfx::RectF()); > In https://codereview.chromium.org/2716893002/, enne@ suggested keeping all tex > rect logic either in the renderer or render surface impl. > > As we need to deal with multiple spaces when computing tex coord rect for a > tiled mask, I think at least in this case the logic should reside in render > surface impl. > > However, in other cases (no mask or mask not tilable), the tex rect is set as a > result of ApplyImageFilter in the renderers, I'm not sure if we have enough > information in render surface impl. > > @senorblanco, do you have suggestions here? Thanks! The problem is closely related to how renderers implement filters. When a filter is present, the renderers basically create a new texture for the filtered result, then replace the quad with a new quad on the fly. Whether this is the best architecture, I'm not sure, as filters can change geometry of the quads, and I think that shouldn't be done implicitly without other part of the code base being aware of it. That's also the exact reason why making filter + tiled mask to work is difficult in our current architecture. Anyway, let's not debate that, and assume we are going to keep the current architecture. Then IMO the nature way to solve this problem is to set tex_coord_rect for the single tile mask code path too. Also in GLRenderer::UpdateRPDQWithSkiaFilters and its software renderer counterpart, replace tex_coord_rect accordingly.
https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surfac... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/80001/cc/layers/render_surfac... cc/layers/render_surface_impl.cc:434: gfx::RectF()); On 2017/03/08 19:57:46, sunxd wrote: > In https://codereview.chromium.org/2716893002/, enne@ suggested keeping all tex > rect logic either in the renderer or render surface impl. > > As we need to deal with multiple spaces when computing tex coord rect for a > tiled mask, I think at least in this case the logic should reside in render > surface impl. > > However, in other cases (no mask or mask not tilable), the tex rect is set as a > result of ApplyImageFilter in the renderers, I'm not sure if we have enough > information in render surface impl. > > @senorblanco, do you have suggestions here? Thanks! That's to accommodate the filter fast-path, which can return a "scratch" texture from Skia as its result, often containing extra (garbage) pixels beyond the result size. The tex rect is used by cc to crop out the garbage. Whatever changes you make, you'll need to ensure that the rect returned by Skia can be passed back to cc for cropping. I'm not sure how that interacts with masks, or how much testing there is of the filter + mask combination.
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I took Tien-Ren's suggestion that we assign a tex_coord_rect in RSI anyways, and later in renderers we overwrite with correct value if a filter changes the texture bounds.
Nit change.
It feels to me that DrawRenderPassDrawQuadParams::dst_rect pretty much equivalent to tex_coord_rect. Could you briefly explain their semantic differences?
On 2017/03/14 22:51:18, trchen wrote: > It feels to me that DrawRenderPassDrawQuadParams::dst_rect pretty much > equivalent to tex_coord_rect. Could you briefly explain their semantic > differences? I haven't investigated that deeply, but it seems if we tile masks, dst_rect has the whole mask layer texture instead of a quad.
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I have updated the CL so that in RSI we try to assign a tex_coord_rect and we overwrite the value in gl_renderer if it is required by the filter.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On mac, we init DrawRPDQParams in another code path.
Looks good in general. Just have a bunch of testing comments: https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... cc/layers/render_surface_impl.cc:430: gfx::RectF tex_coord_rect(0, 0, content_rect().width(), nit: could just be tex_coord_rect(content_rect().size()) https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... cc/layers/render_surface_impl.cc:469: shared_quad_state->visible_quad_layer_rect = gfx::ToEnclosedRect( gfx::ScaleToEnclosingRect, here and elsewhere https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... cc/layers/render_surface_impl.cc:494: -content_rect().OffsetFromOrigin()); Is this logic with a content rect not at the origin tested? https://codereview.chromium.org/2717533005/diff/160001/cc/output/software_ren... File cc/output/software_renderer.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/output/software_ren... cc/output/software_renderer.cc:468: SkRect content_rect = RectFToSkRect(quad->tex_coord_rect); Thanks for that other change to always pass the tex coord rect. I think it makes this much cleaner here, even if gl renderer wants to be fancy. https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6635: if (host_impl->settings().enable_mask_tiling) { Hmm. What's the end state of mask tiling? I assume this setting will go away and always be true. Are we going to keep the mask_type_ in PictureLayerImpl, such that some mask layers will be single tile and others will be tiled based on some criteria? Mostly I'm asking because I'm wondering if this should really be two tests (because we'll have tiled and untiled in the future) or if this is just code that will get cleaned up when the setting flips. https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6637: gfx::RectF(20.f / 128.f, 10.f / 128.f, 10.f / 128.f, 20.f / 128.f) Where does 128 come from? I'm guessing that's rounding up to multiples of 64 for tile size? Can you make this based on the tile size itself, so that when we change tile size, this test won't break? https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6737: gfx::RectF(0.f, 0.f, 100.f / 128.f, 100.f / 128.f).ToString(), Are there any unit tests that have multiple mask tiles? And cc pixel tests are all using this tiled mask path already, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... 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 wrote: > nit: could just be tex_coord_rect(content_rect().size()) Done. https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... cc/layers/render_surface_impl.cc:469: shared_quad_state->visible_quad_layer_rect = gfx::ToEnclosedRect( On 2017/03/28 14:10:22, enne wrote: > gfx::ScaleToEnclosingRect, here and elsewhere Done. https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... cc/layers/render_surface_impl.cc:494: -content_rect().OffsetFromOrigin()); On 2017/03/28 14:10:22, enne wrote: > Is this logic with a content rect not at the origin tested? It's tested in LayoutTests/compositing/overflow/mask-with-small-content-rect.html https://codereview.chromium.org/2717533005/diff/160001/cc/output/software_ren... File cc/output/software_renderer.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/output/software_ren... cc/output/software_renderer.cc:468: SkRect content_rect = RectFToSkRect(quad->tex_coord_rect); On 2017/03/28 14:10:22, enne wrote: > Thanks for that other change to always pass the tex coord rect. I think it > makes this much cleaner here, even if gl renderer wants to be fancy. Done. https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6635: if (host_impl->settings().enable_mask_tiling) { On 2017/03/28 14:10:22, enne wrote: > Hmm. What's the end state of mask tiling? I assume this setting will go away > and always be true. Are we going to keep the mask_type_ in PictureLayerImpl, > such that some mask layers will be single tile and others will be tiled based on > some criteria? > > Mostly I'm asking because I'm wondering if this should really be two tests > (because we'll have tiled and untiled in the future) or if this is just code > that will get cleaned up when the setting flips. The flag is going away once we fix the AA and filter-demoting bugs. But we will have two kinds of masks: tiled one and single texture one (because of filters). https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6637: gfx::RectF(20.f / 128.f, 10.f / 128.f, 10.f / 128.f, 20.f / 128.f) On 2017/03/28 14:10:22, enne wrote: > Where does 128 come from? I'm guessing that's rounding up to multiples of 64 for > tile size? Can you make this based on the tile size itself, so that when we > change tile size, this test won't break? It's an unchecked round up with 64. While 64 is defined as kTileRoundUp in picture_layer_impl.cc, I think we are not able to access this number in cc unit tests, maybe we need to write it as UncheckedRoundUp(mask_size, 64)? https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6737: gfx::RectF(0.f, 0.f, 100.f / 128.f, 100.f / 128.f).ToString(), On 2017/03/28 14:10:22, enne wrote: > Are there any unit tests that have multiple mask tiles? > > And cc pixel tests are all using this tiled mask path already, right? I think this test is testing multiple tiles if the flag is enabled. I'll enable this flag for cc tests.
https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... cc/layers/render_surface_impl.cc:494: -content_rect().OffsetFromOrigin()); On 2017/03/29 at 16:00:23, sunxd wrote: > On 2017/03/28 14:10:22, enne wrote: > > Is this logic with a content rect not at the origin tested? > > It's tested in LayoutTests/compositing/overflow/mask-with-small-content-rect.html Can you add a cc unit test? I don't think layou tests are really the right place to test that sort of detail. https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6635: if (host_impl->settings().enable_mask_tiling) { On 2017/03/29 at 16:00:24, sunxd wrote: > On 2017/03/28 14:10:22, enne wrote: > > Hmm. What's the end state of mask tiling? I assume this setting will go away > > and always be true. Are we going to keep the mask_type_ in PictureLayerImpl, > > such that some mask layers will be single tile and others will be tiled based on > > some criteria? > > > > Mostly I'm asking because I'm wondering if this should really be two tests > > (because we'll have tiled and untiled in the future) or if this is just code > > that will get cleaned up when the setting flips. > > The flag is going away once we fix the AA and filter-demoting bugs. > But we will have two kinds of masks: tiled one and single texture one (because of filters). Ok, then it sounds like we needs two kinds of tests. Can you make these tests test both with and without tiled masks? Additionally, can you make all of the mask pixel tests test with and without tiled masks? https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6637: gfx::RectF(20.f / 128.f, 10.f / 128.f, 10.f / 128.f, 20.f / 128.f) On 2017/03/29 at 16:00:24, sunxd wrote: > On 2017/03/28 14:10:22, enne wrote: > > Where does 128 come from? I'm guessing that's rounding up to multiples of 64 for > > tile size? Can you make this based on the tile size itself, so that when we > > change tile size, this test won't break? > > It's an unchecked round up with 64. While 64 is defined as kTileRoundUp in picture_layer_impl.cc, I think we are not able to access this number in cc unit tests, maybe we need to write it as UncheckedRoundUp(mask_size, 64)? Can you make this based on the resource size, or something? I just want a test that doesn't fail when PictureLayerImpl changes. https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6737: gfx::RectF(0.f, 0.f, 100.f / 128.f, 100.f / 128.f).ToString(), On 2017/03/29 at 16:00:24, sunxd wrote: > On 2017/03/28 14:10:22, enne wrote: > > Are there any unit tests that have multiple mask tiles? > > > > And cc pixel tests are all using this tiled mask path already, right? > > I think this test is testing multiple tiles if the flag is enabled. > > I'll enable this flag for cc tests. As I said above, can you make it work with both kinds of mask types, regardless of whether the flag is on?
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_surfa... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... cc/layers/render_surface_impl.cc:494: -content_rect().OffsetFromOrigin()); On 2017/03/29 16:07:25, enne wrote: > On 2017/03/29 at 16:00:23, sunxd wrote: > > On 2017/03/28 14:10:22, enne wrote: > > > Is this logic with a content rect not at the origin tested? > > > > It's tested in > LayoutTests/compositing/overflow/mask-with-small-content-rect.html > > Can you add a cc unit test? I don't think layou tests are really the right place > to test that sort of detail. I'll work on this on a separate patch just because I need some time to learn how to write a pixel unit test. https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6635: if (host_impl->settings().enable_mask_tiling) { On 2017/03/29 16:07:25, enne wrote: > On 2017/03/29 at 16:00:24, sunxd wrote: > > On 2017/03/28 14:10:22, enne wrote: > > > Hmm. What's the end state of mask tiling? I assume this setting will go > away > > > and always be true. Are we going to keep the mask_type_ in > PictureLayerImpl, > > > such that some mask layers will be single tile and others will be tiled > based on > > > some criteria? > > > > > > Mostly I'm asking because I'm wondering if this should really be two tests > > > (because we'll have tiled and untiled in the future) or if this is just code > > > that will get cleaned up when the setting flips. > > > > The flag is going away once we fix the AA and filter-demoting bugs. > > But we will have two kinds of masks: tiled one and single texture one (because > of filters). > > Ok, then it sounds like we needs two kinds of tests. Can you make these tests > test both with and without tiled masks? Additionally, can you make all of the > mask pixel tests test with and without tiled masks? I can, but the flag will go away shortly. Single texture masks only exist if we have a filter in that world. I think probably I should add tests that has a filter applied to the mask? https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6637: gfx::RectF(20.f / 128.f, 10.f / 128.f, 10.f / 128.f, 20.f / 128.f) On 2017/03/29 16:07:25, enne wrote: > On 2017/03/29 at 16:00:24, sunxd wrote: > > On 2017/03/28 14:10:22, enne wrote: > > > Where does 128 come from? I'm guessing that's rounding up to multiples of 64 > for > > > tile size? Can you make this based on the tile size itself, so that when we > > > change tile size, this test won't break? > > > > It's an unchecked round up with 64. While 64 is defined as kTileRoundUp in > picture_layer_impl.cc, I think we are not able to access this number in cc unit > tests, maybe we need to write it as UncheckedRoundUp(mask_size, 64)? > > Can you make this based on the resource size, or something? I just want a test > that doesn't fail when PictureLayerImpl changes. Done. https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6737: gfx::RectF(0.f, 0.f, 100.f / 128.f, 100.f / 128.f).ToString(), On 2017/03/29 16:07:25, enne wrote: > On 2017/03/29 at 16:00:24, sunxd wrote: > > On 2017/03/28 14:10:22, enne wrote: > > > Are there any unit tests that have multiple mask tiles? > > > > > > And cc pixel tests are all using this tiled mask path already, right? > > > > I think this test is testing multiple tiles if the flag is enabled. > > > > I'll enable this flag for cc tests. > > As I said above, can you make it work with both kinds of mask types, regardless > of whether the flag is on? Acknowledged.
https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... cc/layers/render_surface_impl.cc:494: -content_rect().OffsetFromOrigin()); On 2017/03/30 at 13:56:37, sunxd wrote: > On 2017/03/29 16:07:25, enne wrote: > > On 2017/03/29 at 16:00:23, sunxd wrote: > > > On 2017/03/28 14:10:22, enne wrote: > > > > Is this logic with a content rect not at the origin tested? > > > > > > It's tested in > > LayoutTests/compositing/overflow/mask-with-small-content-rect.html > > > > Can you add a cc unit test? I don't think layou tests are really the right place > > to test that sort of detail. > > I'll work on this on a separate patch just because I need some time to learn how to write a pixel unit test. I think this detail here about the content rect not at the origin can just be a unit test and not a pixel test. Also, I think running the existing mask pixel tests (in layer_tree_host_pixeltest_masks.cc) through both paths of single texture and tiled would be good. And one test like MaskOfLayer with a mask that's large enough to have more than one tile show up in the pixel output. If you have any questions about cc pixel tests, I'm happy to help. :)
I wrote a unit test that tests content_rect.origin not zero case. https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/layers/render_surfa... cc/layers/render_surface_impl.cc:494: -content_rect().OffsetFromOrigin()); On 2017/03/30 14:36:52, enne wrote: > On 2017/03/30 at 13:56:37, sunxd wrote: > > On 2017/03/29 16:07:25, enne wrote: > > > On 2017/03/29 at 16:00:23, sunxd wrote: > > > > On 2017/03/28 14:10:22, enne wrote: > > > > > Is this logic with a content rect not at the origin tested? > > > > > > > > It's tested in > > > LayoutTests/compositing/overflow/mask-with-small-content-rect.html > > > > > > Can you add a cc unit test? I don't think layou tests are really the right > place > > > to test that sort of detail. > > > > I'll work on this on a separate patch just because I need some time to learn > how to write a pixel unit test. > > I think this detail here about the content rect not at the origin can just be a > unit test and not a pixel test. > > Also, I think running the existing mask pixel tests (in > layer_tree_host_pixeltest_masks.cc) through both paths of single texture and > tiled would be good. And one test like MaskOfLayer with a mask that's large > enough to have more than one tile show up in the pixel output. If you have any > questions about cc pixel tests, I'm happy to help. :) Done. https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/2717533005/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest.cc:6635: if (host_impl->settings().enable_mask_tiling) { On 2017/03/30 13:56:37, sunxd wrote: > On 2017/03/29 16:07:25, enne wrote: > > On 2017/03/29 at 16:00:24, sunxd wrote: > > > On 2017/03/28 14:10:22, enne wrote: > > > > Hmm. What's the end state of mask tiling? I assume this setting will go > > away > > > > and always be true. Are we going to keep the mask_type_ in > > PictureLayerImpl, > > > > such that some mask layers will be single tile and others will be tiled > > based on > > > > some criteria? > > > > > > > > Mostly I'm asking because I'm wondering if this should really be two tests > > > > (because we'll have tiled and untiled in the future) or if this is just > code > > > > that will get cleaned up when the setting flips. > > > > > > The flag is going away once we fix the AA and filter-demoting bugs. > > > But we will have two kinds of masks: tiled one and single texture one > (because > > of filters). > > > > Ok, then it sounds like we needs two kinds of tests. Can you make these tests > > test both with and without tiled masks? Additionally, can you make all of the > > mask pixel tests test with and without tiled masks? > > I can, but the flag will go away shortly. Single texture masks only exist if we > have a filter in that world. I think probably I should add tests that has a > filter applied to the mask? As this change blocks AA patch, can I write the tests in another patch?
On 2017/04/04 at 13:58:04, sunxd wrote: > As this change blocks AA patch, can I write the tests in another patch? How does this block it?
On 2017/04/04 at 16:48:10, enne wrote: > On 2017/04/04 at 13:58:04, sunxd wrote: > > > As this change blocks AA patch, can I write the tests in another patch? > > How does this block it? Sorry, maybe I'm misunderstanding. Which change are you talking about?
On 2017/04/04 16:50:08, enne wrote: > On 2017/04/04 at 16:48:10, enne wrote: > > On 2017/04/04 at 13:58:04, sunxd wrote: > > > > > As this change blocks AA patch, can I write the tests in another patch? > > > > How does this block it? > > Sorry, maybe I'm misunderstanding. Which change are you talking about? Oh I mean this patch blocks the AA bug patch. There is also an intention to enable compositor scrolling on border-radius from threaded-rendering OKR.
Recording what I said offline in chat, I think it's fine to restrict tiled mask layers to layers that don't need AA and punt on that until another patch. If that's the case though, I'd like to add some hysteresis here and make sure that layers that go tiled -> single tile don't ever go back the other way, just to prevent raster churn if a page is animating the transform of a masked layer. I also would like to see (1) cc pixel tests with tiled mask layers and (2) as I said in comment #31, break up the tests that are conditionally testing based on a setting to create layers of both single tile and masks and test both ways. Right now the unit tests are only testing one of the code paths conditionally based on a setting, and if both paths are going to live on, I think they both need to be tested in two separate tests.
On 2017/04/05 01:14:16, enne wrote: > Recording what I said offline in chat, I think it's fine to restrict tiled mask > layers to layers that don't need AA and punt on that until another patch. > > If that's the case though, I'd like to add some hysteresis here and make sure > that layers that go tiled -> single tile don't ever go back the other way, just > to prevent raster churn if a page is animating the transform of a masked layer. > > I also would like to see (1) cc pixel tests with tiled mask layers and (2) as I > said in comment #31, break up the tests that are conditionally testing based on > a setting to create layers of both single tile and masks and test both ways. > Right now the unit tests are only testing one of the code paths conditionally > based on a setting, and if both paths are going to live on, I think they both > need to be tested in two separate tests. Sure I have added some code in SetLayerMaskType to prevent the conversion from a single-texture-mask to a multi-texture-mask, so we now can only go one way. I have also filed a tracking issue crbug.com/708582 to track the test changes.
Ok, thanks. lgtm in general, and lgtm to follow up with tests since this is behind a setting.
The CQ bit was checked by sunxd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org Link to the patchset: https://codereview.chromium.org/2717533005/#ps220001 (title: "Prevent single_texture_mask to multi_texture_mask conversion.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sunxd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 220001, "attempt_start_ts": 1491426230731450, "parent_rev": "0c37b7b4d864dfc4d958d1775ba8d9eb2df86282", "commit_rev": "bb0b3ae9224e1883dd491b6b6810b6bfe0da8480"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/bb0b3ae9224e1883dd491b6b6810... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/bb0b3ae9224e1883dd491b6b6810...
Message was sent while issue was closed.
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 ?
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. |