Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14244021:
cc: Remove incorrect dcheck for masks. (Closed)
|
Created:
7 years, 8 months ago by danakj Modified:
7 years, 8 months ago CC:
chromium-reviews, cc-bugs_chromium.org, backer, piman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc: Remove incorrect dcheck for masks.
If a layer has a replica, which has a mask, then the layer itself does not
need to clip to its bounding box. This dcheck verified that it would.
Add some new unit tests and pixel tests that verify behaviour of the mask
texture with masked replicas. The current behaviour is correct. The
replica is masked at the origin of the replicated layer.
Tests:
LayerTreeHostImplTest.ReflectionMaskLayerWithDifferentBounds
LayerTreeHostImplTest.ReflectionMaskLayerForSurfaceWithUnclippedChild
LayerTreeHostImplTest.MaskLayerForSurfaceWithClippedLayer
LayerTreeHostMasksPixelTest.MaskOfLayer
LayerTreeHostMasksPixelTest.ImageMaskOfLayer
LayerTreeHostMasksPixelTest.MaskOfClippedLayer
LayerTreeHostMasksPixelTest.MaskWithReplica
LayerTreeHostMasksPixelTest.MaskWithReplicaOfClippedLayer
LayerTreeHostMasksPixelTest.MaskOfReplica
LayerTreeHostMasksPixelTest.MaskOfReplicaOfClippedLayer
R=enne
BUG=171734
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194639
Patch Set 1 #
Total comments: 7
Patch Set 2 : Better patch #Patch Set 3 : #Patch Set 4 : #
Total comments: 14
Patch Set 5 : With pixel tests #
Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/14244021/diff/1/cc/trees/layer_tree_host_impl... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/14244021/diff/1/cc/trees/layer_tree_host_impl... cc/trees/layer_tree_host_impl_unittest.cc:4508: }, { These are for cpplint.
+shawnsingh
https://codereview.chromium.org/14244021/diff/1/cc/trees/layer_tree_host_impl... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/14244021/diff/1/cc/trees/layer_tree_host_impl... cc/trees/layer_tree_host_impl_unittest.cc:5164: // Applying a different contents scale to the mask layer will still result I'm not sure I follow this case. Why is this the same as the above? https://codereview.chromium.org/14244021/diff/1/cc/trees/layer_tree_host_impl... cc/trees/layer_tree_host_impl_unittest.cc:5273: EXPECT_EQ(gfx::RectF(0.f, 0.f, 2.f, 1.f).ToString(), This 2.f is unexpected in a uv rect. Why would the replica not be masked to bounds here? https://codereview.chromium.org/14244021/diff/1/cc/trees/layer_tree_host_impl... cc/trees/layer_tree_host_impl_unittest.cc:5280: // Move the child to (-50, 0) instead. Now the mask should be moved to still It should?
I've updated the CL a bunch. It should now handle properly: 1) Mask layer with different content scale than owning layer 2) Replica with a mask, with a child layer that grows the surface texture 3) Replica with a mask, with a child layer that grows the surface texture and offsets the replicated layer in the texture. https://codereview.chromium.org/14244021/diff/1/cc/trees/layer_tree_host_impl... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/14244021/diff/1/cc/trees/layer_tree_host_impl... cc/trees/layer_tree_host_impl_unittest.cc:5164: // Applying a different contents scale to the mask layer will still result I think this case is wrong. It was a copy paste from the test above this, but I disagree with what I wrote there too.. This isn't even setting a scale differently, just a content bounds. Instead what this should say and do, I think, is: Applying a different contents scale to the mask layer means it will have to scale itself to cover the same area its bounds should cover. Since the contents scale is halved, the mask will have to be stretched double to cover the right area. I.e. If the masking layer's bounds are not changed, but its contents scale/texture size is changed, then the uv rect should be adjusted accordingly. https://codereview.chromium.org/14244021/diff/1/cc/trees/layer_tree_host_impl... cc/trees/layer_tree_host_impl_unittest.cc:5273: EXPECT_EQ(gfx::RectF(0.f, 0.f, 2.f, 1.f).ToString(), On 2013/04/15 22:20:40, enne wrote: > This 2.f is unexpected in a uv rect. Why would the replica not be masked to > bounds here? Right, it should be 1.f https://codereview.chromium.org/14244021/diff/1/cc/trees/layer_tree_host_impl... cc/trees/layer_tree_host_impl_unittest.cc:5280: // Move the child to (-50, 0) instead. Now the mask should be moved to still On 2013/04/15 22:20:40, enne wrote: > It should? Ya, 0,0 in the surface texture is not inside the bounds of the owning layer which is being replicated/masked. Now the owning layer is at 50,0 in the surface, so the mask texture coords should be shifted by -50,0, such that they will be 0 while outside the reflected layer's bounds.
I also didn't see a test for the case where the surface is clipped, so I added LayerTreeHostImplTest.MaskLayerForSurfaceWithClippedLayer (note that if it has a replica we explicitly don't allow the surface texture to be clipped in LTHCommon)
WTB pixel tests so that correctness is more obvious https://codereview.chromium.org/14244021/diff/13001/cc/layers/render_surface_... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/14244021/diff/13001/cc/layers/render_surface_... cc/layers/render_surface_impl.cc:257: gfx::SizeF clipped_mask_target_size = gfx::IntersectRects( I'm a little confused about this intersection. Don't masks get stretched? https://codereview.chromium.org/14244021/diff/13001/cc/layers/render_surface_... cc/layers/render_surface_impl.cc:266: gfx::Vector2dF owning_layer_to_mask_layer_bounds_ratio( This whole function needs a lot more comments about what's going on here. https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl_unittest.cc:5183: EXPECT_EQ(gfx::RectF(0.f, 0.f, 0.5f, 0.5f).ToString(), Can you explain this? These uv coords look like you're just going to use the upper left quadrant of the mask just because the owning layer has a contents scale, which is not what the comment above says. https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl_unittest.cc:5266: // The mask covers the owning layer only. This does not jibe with the numbers below. 0011 means "stretch the mask to cover this quad" and 0, 0, 100, 50 is "owner + child". Therefore, the mask covers both, right? https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl_unittest.cc:5308: EXPECT_EQ(gfx::RectF(-0.5f, 0.f, 1.f, 1.f).ToString(), Previously, I mentioned that 2.f as a texcoord was unexpected. Likewise, -0.5f as a texcoord is also unexpected. Shouldn't the quad be clipped such that uv rect coords are all 0..1?
https://codereview.chromium.org/14244021/diff/13001/cc/layers/render_surface_... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/14244021/diff/13001/cc/layers/render_surface_... cc/layers/render_surface_impl.cc:257: gfx::SizeF clipped_mask_target_size = gfx::IntersectRects( On 2013/04/16 21:06:44, enne wrote: > I'm a little confused about this intersection. Don't masks get stretched? This is the old behaviour minus assumptions that the content_rect_ == owning layer's bounds. Masks do not get stretched. Previously we have the scale as content_rect.width / unclipped_surface_size.width. What that was trying to do was say "if the owning layer is 100x100 but the surface texture gets clipped to 20x20, then map correctly from from the surface texture, where 0...1 represents 0-20, to the mask texture where 0...1 represents 0-100. So we need to scale the coords down to get the same pixels in the mask texture as if the surface texture was unclipped. Previously this code just used the content_rect_. But if you have a replica with a mask, then the content_rect_ != the owning layer's clipped space in the target surface. So instead of using the full content_rect_, just use the part for the owning layer. If another layer makes the surface larger on the left, but the surface texture is clipping the right half of the owning layer, then this will map the tex coords for the mask to match the texels of the owning layer, not the owning layer + its children. https://codereview.chromium.org/14244021/diff/13001/cc/layers/render_surface_... cc/layers/render_surface_impl.cc:266: gfx::Vector2dF owning_layer_to_mask_layer_bounds_ratio( On 2013/04/16 21:06:44, enne wrote: > This whole function needs a lot more comments about what's going on here. ok, will add. https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl_unittest.cc:5183: EXPECT_EQ(gfx::RectF(0.f, 0.f, 0.5f, 0.5f).ToString(), On 2013/04/16 21:06:44, enne wrote: > Can you explain this? These uv coords look like you're just going to use the > upper left quadrant of the mask just because the owning layer has a contents > scale, which is not what the comment above says. The shader takes a tex coord in the surface texture, and multiplies it by 0.5 to find a tex coord in the mask. So, both 1 in the surface gets 0.5 in the mask. 2 in the surface gets 1 in the mask. So the mask texture covers twice as many surface texels. https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl_unittest.cc:5266: // The mask covers the owning layer only. On 2013/04/16 21:06:44, enne wrote: > This does not jibe with the numbers below. 0011 means "stretch the mask to > cover this quad" and 0, 0, 100, 50 is "owner + child". Therefore, the mask > covers both, right? If the mask is 50px wide, and the surface texture is 100px wide, this means the left 50px of the surface texture map 1:1 with texels in the mask. The right 50px will be outside the bounds of the mask texture (so they get whatever GL_CLAMP behaviour we have going on). The GL_CLAMP stuff is a bit unfortunate, but not sure how to solve that with our current shader implementation of masks. https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl_unittest.cc:5308: EXPECT_EQ(gfx::RectF(-0.5f, 0.f, 1.f, 1.f).ToString(), On 2013/04/16 21:06:44, enne wrote: > Previously, I mentioned that 2.f as a texcoord was unexpected. Likewise, -0.5f > as a texcoord is also unexpected. Shouldn't the quad be clipped such that uv > rect coords are all 0..1? The offset is not applied the same way as the scale. Instead it is used directly to add to the mask tex coords. So you get the same result as above, but it's all shifted by 0.5. I.e. the left half of the surface texture maps to tex coords outside the bounds of the mask texture. The right half maps to tex coords 1:1 inside the mask texture, which is where the owning layer is. To be clear, this is our current behaviour. The only behaviour change in this CL is dealing with contents scale not matching between the mask and the owning layer (the "Applying a different contents scale to the mask layer" case in the previous tests).
On 2013/04/16 21:21:00, danakj wrote: > The only behaviour change in this CL > is dealing with contents scale not matching between the mask and the owning > layer (the "Applying a different contents scale to the mask layer" case in the > previous tests). Oh, I think that's a lie, it should also be fixing the case where the owning layer is clipped, and the replica has a mask. I'll add a test for that
On 2013/04/16 21:06:44, enne wrote: > WTB pixel tests so that correctness is more obvious I think layout tests would be most appropriate here, would you agree?
On 2013/04/16 21:23:31, danakj wrote: > On 2013/04/16 21:06:44, enne wrote: > > WTB pixel tests so that correctness is more obvious > > I think layout tests would be most appropriate here, would you agree? No, I meant pixel tests intentionally. In my opinion, layout tests that interact with cc are for making sure that html/css make it into the compositor correctly. Pixel tests are making sure that inputs to or subsystems of the compositor turn into correct pixels.
https://codereview.chromium.org/14244021/diff/13001/cc/layers/render_surface_... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/14244021/diff/13001/cc/layers/render_surface_... cc/layers/render_surface_impl.cc:257: gfx::SizeF clipped_mask_target_size = gfx::IntersectRects( > If another layer makes the surface larger on the left, but the surface texture > is clipping the right half of the owning layer, then this will map the tex > coords for the mask to match the texels of the owning layer, not the owning > layer + its children. I don't understand this - if the surface has a mask, it's masking to bounds, and its origin will be its top-left. no other layer can make the surface "larger on the left" then. What am I missing?
https://codereview.chromium.org/14244021/diff/13001/cc/layers/render_surface_... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/14244021/diff/13001/cc/layers/render_surface_... cc/layers/render_surface_impl.cc:257: gfx::SizeF clipped_mask_target_size = gfx::IntersectRects( On 2013/04/16 23:08:26, shawnsingh wrote: > > If another layer makes the surface larger on the left, but the surface texture > > is clipping the right half of the owning layer, then this will map the tex > > coords for the mask to match the texels of the owning layer, not the owning > > layer + its children. > > > I don't understand this - if the surface has a mask, it's masking to bounds, and > its origin will be its top-left. no other layer can make the surface "larger on > the left" then. What am I missing? if the replica has a mask, the layer does not mask to bounds and the surface can grow. that said beautiful pixel tests have shown a bug in here for that scenario.
Now with pixeltests! Here's the pixel results: LayerTreeHostMasksPixelTest.MaskOfLayer http://i.imgur.com/jUZlczes.jpg LayerTreeHostMasksPixelTest.ImageMaskOfLayer http://imgur.com/kbSiqkI LayerTreeHostMasksPixelTest.MaskOfClippedLayer http://imgur.com/aXpU4n3 LayerTreeHostMasksPixelTest.MaskWithReplica http://imgur.com/RROCNSi LayerTreeHostMasksPixelTest.MaskWithReplicaOfClippedLayer http://imgur.com/dOPvUoU LayerTreeHostMasksPixelTest.MaskOfReplica http://imgur.com/ncceFyJ LayerTreeHostMasksPixelTest.MaskOfReplicaOfClippedLayer http://imgur.com/0CoR9aT
On 2013/04/17 00:04:49, danakj wrote: > Now with pixeltests! Here's the pixel results: > > LayerTreeHostMasksPixelTest.MaskOfLayer http://i.imgur.com/jUZlczes.jpg That should be http://i.imgur.com/jUZlczes without the .jpg
https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl_unittest.cc:5308: EXPECT_EQ(gfx::RectF(-0.5f, 0.f, 1.f, 1.f).ToString(), On 2013/04/16 21:21:01, danakj wrote: > On 2013/04/16 21:06:44, enne wrote: > > Previously, I mentioned that 2.f as a texcoord was unexpected. Likewise, > -0.5f > > as a texcoord is also unexpected. Shouldn't the quad be clipped such that uv > > rect coords are all 0..1? > > The offset is not applied the same way as the scale. Instead it is used directly > to add to the mask tex coords. > > So you get the same result as above, but it's all shifted by 0.5. I.e. the left > half of the surface texture maps to tex coords outside the bounds of the mask > texture. The right half maps to tex coords 1:1 inside the mask texture, which is > where the owning layer is. > > To be clear, this is our current behaviour. The only behaviour change in this CL > is dealing with contents scale not matching between the mask and the owning > layer (the "Applying a different contents scale to the mask layer" case in the > previous tests). I still think this is wrong, and this is why I suggested having a mask that goes all the way to the border on your pixel test patch. Because of the clamping behavior, anything outside of the mask bounds will either get clipped or not clipped depending on the border texels of the mask. It means that between -50 and 0 in the x coordinate of the quad, it'll use texcoords -1..0, and between 0 and 50, it'll use 0..1. What happens in -1..0 depends on the texels in column x=0. I think we should really be generating a quad that's 0, 0, 50, 50 and texcoords that are 0, 0, 1, 1. Maybe this is a separate bug, but it still looks like a bug.
https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/14244021/diff/13001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl_unittest.cc:5308: EXPECT_EQ(gfx::RectF(-0.5f, 0.f, 1.f, 1.f).ToString(), On 2013/04/17 16:24:40, enne wrote: > On 2013/04/16 21:21:01, danakj wrote: > > On 2013/04/16 21:06:44, enne wrote: > > > Previously, I mentioned that 2.f as a texcoord was unexpected. Likewise, > > -0.5f > > > as a texcoord is also unexpected. Shouldn't the quad be clipped such that > uv > > > rect coords are all 0..1? > > > > The offset is not applied the same way as the scale. Instead it is used > directly > > to add to the mask tex coords. > > > > So you get the same result as above, but it's all shifted by 0.5. I.e. the > left > > half of the surface texture maps to tex coords outside the bounds of the mask > > texture. The right half maps to tex coords 1:1 inside the mask texture, which > is > > where the owning layer is. > > > > To be clear, this is our current behaviour. The only behaviour change in this > CL > > is dealing with contents scale not matching between the mask and the owning > > layer (the "Applying a different contents scale to the mask layer" case in the > > previous tests). > > I still think this is wrong, and this is why I suggested having a mask that goes > all the way to the border on your pixel test patch. Because of the clamping > behavior, anything outside of the mask bounds will either get clipped or not > clipped depending on the border texels of the mask. It means that between -50 > and 0 in the x coordinate of the quad, it'll use texcoords -1..0, and between 0 > and 50, it'll use 0..1. What happens in -1..0 depends on the texels in column > x=0. I think we should really be generating a quad that's 0, 0, 50, 50 and > texcoords that are 0, 0, 1, 1. > > Maybe this is a separate bug, but it still looks like a bug. Oh I replied to the pictures one about this before reading this one. I see, I had no idea if we could solve this without doing something completely different. If we can solve it just by adjusting the quad rect, then hooray. I can look into that. I had avoided this bug in the pixel tests because I was considering it "unsolvable" in the immediate moment.
lgtm to land this and fix this one bug. You filed a follow-up, so we can address the clamping issue there. Thanks again for taking the time to write those pixel tests. Those are super helpful.
Message was sent while issue was closed.
Committed patchset #5 manually as r194639 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
