Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2673813002:
Changes the bounds being sent for occlusion from physical pixels to DIP (Closed)
|
Created:
3 years, 10 months ago by malaykeshav Modified:
3 years, 10 months ago CC:
cc-bugs_chromium.org, chromium-reviews, oshima Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChanges the bounds being sent for occlusion from physical pixels to DIP
Occlusion::GetUnoccludedContentRect() applies a draw transform to the
bounds being sent. This draw transform already has the scaling applied
to it which causes the physical pixel bounds to be scaled again
leading to an incorrect visible rect result.
Also adds unittest to check for this scenario in the future.
BUG=660744
COMPONENT=CC, Layer, Surface Layer Impl
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2673813002
Cr-Commit-Position: refs/heads/master@{#449755}
Committed: https://chromium.googlesource.com/chromium/src/+/9c7c49cb95b000cdaef78de90eb6a3cd46c663f9
Patch Set 1 #Patch Set 2 : Test breaks no more! #Patch Set 3 : Changes the bounds being sent for occlusion from physical pixels to DIP #Patch Set 4 : Intersect visible_quad_rect with surface size in pixels #Patch Set 5 : Adds unittests to check for this case #
Total comments: 4
Patch Set 6 : Adds another occlusion test for occlusion from outside #Patch Set 7 : Use gfx::ScaleToEnclosingRect instead of gfx::ScaleToEnclosedRect #
Total comments: 10
Patch Set 8 : Resolving comments #
Total comments: 3
Messages
Total messages: 62 (49 generated)
Description was changed from ========== Changes the bounds being sent for occlusion from physical pixels to DIP Occlusion::GetUnoccludedContentRect() applies a draw transform to the bounds being sent. This draw transform already has the scaling applied to it which causes the physical pixel bounds to be scaled again leading to an incorrect visible rect result. BUG=660744 COMPONENT=CC, Layer, Surface Layer Impl ========== to ========== Changes the bounds being sent for occlusion from physical pixels to DIP Occlusion::GetUnoccludedContentRect() applies a draw transform to the bounds being sent. This draw transform already has the scaling applied to it which causes the physical pixel bounds to be scaled again leading to an incorrect visible rect result. BUG=660744 COMPONENT=CC, Layer, Surface Layer Impl CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by malaykeshav@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_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 malaykeshav@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...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by malaykeshav@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_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 malaykeshav@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...
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_...)
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by malaykeshav@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Changes the bounds being sent for occlusion from physical pixels to DIP Occlusion::GetUnoccludedContentRect() applies a draw transform to the bounds being sent. This draw transform already has the scaling applied to it which causes the physical pixel bounds to be scaled again leading to an incorrect visible rect result. BUG=660744 COMPONENT=CC, Layer, Surface Layer Impl CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Changes the bounds being sent for occlusion from physical pixels to DIP Occlusion::GetUnoccludedContentRect() applies a draw transform to the bounds being sent. This draw transform already has the scaling applied to it which causes the physical pixel bounds to be scaled again leading to an incorrect visible rect result. Also adds unittest to check for this scenario in the future. BUG=660744 COMPONENT=CC, Layer, Surface Layer Impl CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by malaykeshav@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...
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by malaykeshav@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...
malaykeshav@chromium.org changed reviewers: + weiliangc@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2673813002/diff/140001/cc/layers/surface_laye... File cc/layers/surface_layer_impl_unittest.cc (right): https://codereview.chromium.org/2673813002/diff/140001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:23: gfx::Size scaled_layer_size( nit: Rename to scaled_surface_size, since scaled_layer_size sounds like it might be in same space as layer_size. https://codereview.chromium.org/2673813002/diff/140001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:67: gfx::Rect occluded(gfx::ScaleToEnclosedRect(gfx::Rect(200, 0, 800, 1000), I don't think this test really reflects the bug. The bug is where there is occlusion that is inside viewport, but outside layer, it covers up layer when dsf is not 1. Possibly add another case here similar to the partial occlusion test case, bug make sure the occluded rect is outside layer_rect, but still inside viewport. And even w/ dsf, the end result is similar to no occlusion case. To make sure the unittest actually catches the bug, you can comment out your fix in real code, and check to see if the unittest not fails.
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by malaykeshav@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...
Updated unit test and fixed a small bug in layer_test_common code. PTAL https://codereview.chromium.org/2673813002/diff/140001/cc/layers/surface_laye... File cc/layers/surface_layer_impl_unittest.cc (right): https://codereview.chromium.org/2673813002/diff/140001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:23: gfx::Size scaled_layer_size( On 2017/02/09 at 18:25:32, weiliangc wrote: > nit: Rename to scaled_surface_size, since scaled_layer_size sounds like it might be in same space as layer_size. done https://codereview.chromium.org/2673813002/diff/140001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:67: gfx::Rect occluded(gfx::ScaleToEnclosedRect(gfx::Rect(200, 0, 800, 1000), On 2017/02/09 at 18:25:32, weiliangc wrote: > I don't think this test really reflects the bug. > > The bug is where there is occlusion that is inside viewport, but outside layer, it covers up layer when dsf is not 1. > > Possibly add another case here similar to the partial occlusion test case, bug make sure the occluded rect is outside layer_rect, but still inside viewport. And even w/ dsf, the end result is similar to no occlusion case. > > To make sure the unittest actually catches the bug, you can comment out your fix in real code, and check to see if the unittest not fails. Yes. But without the patch these 3 tests failed as well. The occlusion from outside is a result/combination of these 3. Keeping these tests and adding a 4th that occludes from outside.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by malaykeshav@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...
malaykeshav@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org
malaykeshav@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org
PTAL
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:62: float scale_x, scale_y; Can you give this a name that describes what the scale is? Scale from what space to what space? https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:73: PopulateScaledSharedQuadState(shared_quad_state, We don't need 2 calls to Populate anymore, if we have these scale_x/y variables. We can just call Populate below the if/else block with them. https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer_impl_unittest.cc (right): https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:19: TEST(SurfaceLayerImplTest, OcclusionWithDSF) { try to avoid acronyms https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:66: SCOPED_TRACE("Partial occlusion"); Which cases fail in this test without your changes in SurfaceLayerImpl? https://codereview.chromium.org/2673813002/diff/200001/cc/test/layer_test_com... File cc/test/layer_test_common.cc (right): https://codereview.chromium.org/2673813002/diff/200001/cc/test/layer_test_com... cc/test/layer_test_common.cc:53: gfx::RectF(iter->visible_rect)); Should this verify quad rect /and/ visible rect?
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Resolved comments. PTAL https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:62: float scale_x, scale_y; On 2017/02/10 at 16:32:22, danakj wrote: > Can you give this a name that describes what the scale is? Scale from what space to what space? Done https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:73: PopulateScaledSharedQuadState(shared_quad_state, On 2017/02/10 at 16:32:22, danakj wrote: > We don't need 2 calls to Populate anymore, if we have these scale_x/y variables. We can just call Populate below the if/else block with them. Done https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer_impl_unittest.cc (right): https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:19: TEST(SurfaceLayerImplTest, OcclusionWithDSF) { On 2017/02/10 at 16:32:22, danakj wrote: > try to avoid acronyms Done https://codereview.chromium.org/2673813002/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:66: SCOPED_TRACE("Partial occlusion"); On 2017/02/10 at 16:32:22, danakj wrote: > Which cases fail in this test without your changes in SurfaceLayerImpl? All except "No occlusion". https://codereview.chromium.org/2673813002/diff/200001/cc/test/layer_test_com... File cc/test/layer_test_common.cc (right): https://codereview.chromium.org/2673813002/diff/200001/cc/test/layer_test_com... cc/test/layer_test_common.cc:53: gfx::RectF(iter->visible_rect)); On 2017/02/10 at 16:32:22, danakj wrote: > Should this verify quad rect /and/ visible rect? Done
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2673813002/diff/220001/cc/test/layer_test_com... File cc/test/layer_test_common.cc (right): https://codereview.chromium.org/2673813002/diff/220001/cc/test/layer_test_com... cc/test/layer_test_common.cc:51: EXPECT_TRUE(iter->rect.Contains(iter->visible_rect)); In order to exactly cover, would the rects not need to be equal? Or am I thinking about this wrong.
https://codereview.chromium.org/2673813002/diff/220001/cc/test/layer_test_com... File cc/test/layer_test_common.cc (right): https://codereview.chromium.org/2673813002/diff/220001/cc/test/layer_test_com... cc/test/layer_test_common.cc:51: EXPECT_TRUE(iter->rect.Contains(iter->visible_rect)); On 2017/02/10 at 20:35:44, danakj wrote: > In order to exactly cover, would the rects not need to be equal? Or am I thinking about this wrong. I think its: all the |quads| (visible) combined should exactly cover |remaining| region.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2673813002/diff/220001/cc/test/layer_test_com... File cc/test/layer_test_common.cc (right): https://codereview.chromium.org/2673813002/diff/220001/cc/test/layer_test_com... cc/test/layer_test_common.cc:51: EXPECT_TRUE(iter->rect.Contains(iter->visible_rect)); On 2017/02/10 21:02:50, malaykeshav wrote: > On 2017/02/10 at 20:35:44, danakj wrote: > > In order to exactly cover, would the rects not need to be equal? Or am I > thinking about this wrong. > > I think its: all the |quads| (visible) combined should exactly cover |remaining| > region. OK I think you're right, and checking that the visible rect isn't exceeding the quads is good.
The CQ bit was checked by malaykeshav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from weiliangc@chromium.org Link to the patchset: https://codereview.chromium.org/2673813002/#ps220001 (title: "Resolving comments")
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": 1486764439554260,
"parent_rev": "75ab22d721451fbe895843e588ef66558ba03ddb", "commit_rev":
"9c7c49cb95b000cdaef78de90eb6a3cd46c663f9"}
Message was sent while issue was closed.
Description was changed from ========== Changes the bounds being sent for occlusion from physical pixels to DIP Occlusion::GetUnoccludedContentRect() applies a draw transform to the bounds being sent. This draw transform already has the scaling applied to it which causes the physical pixel bounds to be scaled again leading to an incorrect visible rect result. Also adds unittest to check for this scenario in the future. BUG=660744 COMPONENT=CC, Layer, Surface Layer Impl CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Changes the bounds being sent for occlusion from physical pixels to DIP Occlusion::GetUnoccludedContentRect() applies a draw transform to the bounds being sent. This draw transform already has the scaling applied to it which causes the physical pixel bounds to be scaled again leading to an incorrect visible rect result. Also adds unittest to check for this scenario in the future. BUG=660744 COMPONENT=CC, Layer, Surface Layer Impl CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2673813002 Cr-Commit-Position: refs/heads/master@{#449755} Committed: https://chromium.googlesource.com/chromium/src/+/9c7c49cb95b000cdaef78de90eb6... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001) as https://chromium.googlesource.com/chromium/src/+/9c7c49cb95b000cdaef78de90eb6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
