Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
574343003:
cc: Change LTHCommon tests to use impl painting. (Closed)
Descriptioncc: Change LTHCommon tests to use impl painting.
Make LTHCommon tests to use PictureLayers instead of ContentLayer,
and update the scaling tests to use ideal scales instead of
contents scale accordingly.
BUG=401492
Committed: https://crrev.com/e3bd619e1875337517ec496f9e8cb818b304fcd5
Cr-Commit-Position: refs/heads/master@{#299269}
Patch Set 1 #
Total comments: 1
Patch Set 2 : use ideal content scale. #
Total comments: 3
Patch Set 3 : updated ideal scale calc in SurfaceLayerTransformsInHighDPI test. #
Total comments: 4
Patch Set 4 : review comments addressed. #
Total comments: 44
Patch Set 5 : review comments addressed. #Patch Set 6 : rebase to ToT #
Total comments: 4
Patch Set 7 : review comments addressed + rebased to ToT #Messages
Total messages: 29 (6 generated)
sohan.jyoti@samsung.com changed reviewers: + danakj@chromium.org
There are lot of repetitive tests for content scale which is now 1, i removed some code. Can you please take a look, if the direction is correct, thanks!
https://codereview.chromium.org/574343003/diff/1/cc/trees/layer_tree_host_com... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/1/cc/trees/layer_tree_host_com... cc/trees/layer_tree_host_common_unittest.cc:4386: EXPECT_CONTENTS_SCALE_EQ(1, parent); we should check the layer's ideal_contents_scale instead of the contents_scale_x() now. we're still computing something in LTHCommon it's just being used differently on picture layer impl now.
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Please give your comments, thanks. https://codereview.chromium.org/574343003/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common_unittest.cc:4289: EXPECT_IDEAL_SCALE_EQ(device_scale_factor * page_scale_factor, scale_surface); In this test somehow ideal scale is (device_scale_factor * page_scale_factor / 10), not sure how , is scale_small_matrix affecting ?can u suggest?
https://codereview.chromium.org/574343003/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common_unittest.cc:4289: EXPECT_IDEAL_SCALE_EQ(device_scale_factor * page_scale_factor, scale_surface); On 2014/09/23 14:52:03, sohanjg wrote: > In this test somehow ideal scale is (device_scale_factor * page_scale_factor / > 10), not sure how , is scale_small_matrix affecting ?can u suggest? What is it instead? Are the other 2 right?
can you please take a look, thanks. https://codereview.chromium.org/574343003/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common_unittest.cc:4289: EXPECT_IDEAL_SCALE_EQ(device_scale_factor * page_scale_factor, scale_surface); On 2014/09/23 15:00:17, danakj wrote: > On 2014/09/23 14:52:03, sohanjg wrote: > > In this test somehow ideal scale is (device_scale_factor * page_scale_factor / > > 10), not sure how , is scale_small_matrix affecting ?can u suggest? > > What is it instead? Are the other 2 right? We had to consider max combined transforms here, updated it.
sohan.jyoti@samsung.com changed reviewers: + enne@chromium.org
https://codereview.chromium.org/574343003/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common_unittest.cc:4291: gfx::Transform transform = scale_small_matrix; Did you figure out why this is a factor in the scale we choose in implside but not in the old path?
https://codereview.chromium.org/574343003/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common_unittest.cc:4291: gfx::Transform transform = scale_small_matrix; On 2014/09/24 14:38:16, danakj wrote: > Did you figure out why this is a factor in the scale we choose in implside but > not in the old path? When i checked in non impl-path, the ideal scale value was coming same as in impl-path (i.e device_scale_factor * page_scale_factor / 10)
https://codereview.chromium.org/574343003/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common_unittest.cc:4291: gfx::Transform transform = scale_small_matrix; On 2014/09/24 15:09:31, sohanjg wrote: > On 2014/09/24 14:38:16, danakj wrote: > > Did you figure out why this is a factor in the scale we choose in implside but > > not in the old path? > > When i checked in non impl-path, the ideal scale value was coming same as in > impl-path (i.e device_scale_factor * page_scale_factor / 10) But the non-impl path doesn't use that scale does it? It's contents scale was not using this transform.
https://codereview.chromium.org/574343003/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common_unittest.cc:4291: gfx::Transform transform = scale_small_matrix; On 2014/09/24 15:11:26, danakj wrote: > On 2014/09/24 15:09:31, sohanjg wrote: > > On 2014/09/24 14:38:16, danakj wrote: > > > Did you figure out why this is a factor in the scale we choose in implside > but > > > not in the old path? > > > > When i checked in non impl-path, the ideal scale value was coming same as in > > impl-path (i.e device_scale_factor * page_scale_factor / 10) > > But the non-impl path doesn't use that scale does it? It's contents scale was > not using this transform. No, for non-impl path, contents scale was not using this transform. But if we queried the ideal contents scale we found it to be using the transform. i think in here we only do CalculateDrawProperties, and we dont go to layer impls to set contents scale to ideal contents scale, hence the difference between ideal and contents scale for layer with scaled transform?
On Wed, Sep 24, 2014 at 11:21 AM, <sohan.jyoti@samsung.com> wrote: > > https://codereview.chromium.org/574343003/diff/80001/cc/ > trees/layer_tree_host_common_unittest.cc > File cc/trees/layer_tree_host_common_unittest.cc (right): > > https://codereview.chromium.org/574343003/diff/80001/cc/ > trees/layer_tree_host_common_unittest.cc#newcode4291 > cc/trees/layer_tree_host_common_unittest.cc:4291: gfx::Transform > transform = scale_small_matrix; > On 2014/09/24 15:11:26, danakj wrote: > >> On 2014/09/24 15:09:31, sohanjg wrote: >> > On 2014/09/24 14:38:16, danakj wrote: >> > > Did you figure out why this is a factor in the scale we choose in >> > implside > >> but >> > > not in the old path? >> > >> > When i checked in non impl-path, the ideal scale value was coming >> > same as in > >> > impl-path (i.e device_scale_factor * page_scale_factor / 10) >> > > But the non-impl path doesn't use that scale does it? It's contents >> > scale was > >> not using this transform. >> > > No, for non-impl path, contents scale was not using this transform. > But if we queried the ideal contents scale we found it to be using the > transform. > > i think in here we only do CalculateDrawProperties, and we dont go to > layer impls to set contents scale to ideal contents scale, hence the > difference between ideal and contents scale for layer with scaled > transform? > What I mean is, in non-impl painting, the contents scale was used to raster contents. In impl-painting, the raster scale is set to the ideal scale. So this means we're producing different pixel output in this case with impl-side. That's how I understand it, and that seems like a bug. I don't think we should change the expectations of this test. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/09/24 15:23:58, danakj wrote: > On Wed, Sep 24, 2014 at 11:21 AM, <mailto:sohan.jyoti@samsung.com> wrote: > > > > > https://codereview.chromium.org/574343003/diff/80001/cc/ > > trees/layer_tree_host_common_unittest.cc > > File cc/trees/layer_tree_host_common_unittest.cc (right): > > > > https://codereview.chromium.org/574343003/diff/80001/cc/ > > trees/layer_tree_host_common_unittest.cc#newcode4291 > > cc/trees/layer_tree_host_common_unittest.cc:4291: gfx::Transform > > transform = scale_small_matrix; > > On 2014/09/24 15:11:26, danakj wrote: > > > >> On 2014/09/24 15:09:31, sohanjg wrote: > >> > On 2014/09/24 14:38:16, danakj wrote: > >> > > Did you figure out why this is a factor in the scale we choose in > >> > > implside > > > >> but > >> > > not in the old path? > >> > > >> > When i checked in non impl-path, the ideal scale value was coming > >> > > same as in > > > >> > impl-path (i.e device_scale_factor * page_scale_factor / 10) > >> > > > > But the non-impl path doesn't use that scale does it? It's contents > >> > > scale was > > > >> not using this transform. > >> > > > > No, for non-impl path, contents scale was not using this transform. > > But if we queried the ideal contents scale we found it to be using the > > transform. > > > > i think in here we only do CalculateDrawProperties, and we dont go to > > layer impls to set contents scale to ideal contents scale, hence the > > difference between ideal and contents scale for layer with scaled > > transform? > > > > What I mean is, in non-impl painting, the contents scale was used to raster > contents. In impl-painting, the raster scale is set to the ideal scale. So > this means we're producing different pixel output in this case with > impl-side. That's how I understand it, and that seems like a bug. I don't > think we should change the expectations of this test. > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Yes, we're producing different pixel output. Looks like ContentsScalingLayer making the contents scale as |raster_scale * device_scale_factor * page_scale_factor| https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... is the main difference between the 2 paths, for impl painting we will set contents scale as |1| https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer.cc... Whereas, the ideal contents scale remains the same across impl/non-impl world. Should we raise a bug and re-calculate scales in PictureLayer ?
On Thu, Sep 25, 2014 at 7:01 AM, <sohan.jyoti@samsung.com> wrote: > On 2014/09/24 15:23:58, danakj wrote: > >> On Wed, Sep 24, 2014 at 11:21 AM, <mailto:sohan.jyoti@samsung.com> wrote: >> > > > >> > https://codereview.chromium.org/574343003/diff/80001/cc/ >> > trees/layer_tree_host_common_unittest.cc >> > File cc/trees/layer_tree_host_common_unittest.cc (right): >> > >> > https://codereview.chromium.org/574343003/diff/80001/cc/ >> >> > trees/layer_tree_host_common_unittest.cc#newcode4291 >> > cc/trees/layer_tree_host_common_unittest.cc:4291: gfx::Transform >> > transform = scale_small_matrix; >> > On 2014/09/24 15:11:26, danakj wrote: >> > >> >> On 2014/09/24 15:09:31, sohanjg wrote: >> >> > On 2014/09/24 14:38:16, danakj wrote: >> >> > > Did you figure out why this is a factor in the scale we choose in >> >> >> > implside >> > >> >> but >> >> > > not in the old path? >> >> > >> >> > When i checked in non impl-path, the ideal scale value was coming >> >> >> > same as in >> > >> >> > impl-path (i.e device_scale_factor * page_scale_factor / 10) >> >> >> > >> > But the non-impl path doesn't use that scale does it? It's contents >> >> >> > scale was >> > >> >> not using this transform. >> >> >> > >> > No, for non-impl path, contents scale was not using this transform. >> > But if we queried the ideal contents scale we found it to be using the >> > transform. >> > >> > i think in here we only do CalculateDrawProperties, and we dont go to >> > layer impls to set contents scale to ideal contents scale, hence the >> > difference between ideal and contents scale for layer with scaled >> > transform? >> > >> > > What I mean is, in non-impl painting, the contents scale was used to >> raster >> contents. In impl-painting, the raster scale is set to the ideal scale. So >> this means we're producing different pixel output in this case with >> impl-side. That's how I understand it, and that seems like a bug. I don't >> think we should change the expectations of this test. >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > Yes, we're producing different pixel output. > Looks like ContentsScalingLayer making the contents scale as |raster_scale > * > device_scale_factor * page_scale_factor| > > https://code.google.com/p/chromium/codesearch#chromium/ > src/cc/trees/layer_tree_host_common.cc&sq=package:chromium& > q=UpdateLayerContentsScale&l=1014 > > LayerTreeHostCommon is supposed to also compute the ideal scale in the same way, it should be source * dsf * psf (source scale was called raster scale in the old code). > is the main difference between the 2 paths, for impl painting we will set > contents scale as |1| > > https://code.google.com/p/chromium/codesearch#chromium/ > src/cc/layers/layer.cc&cl=GROK&l=411&gsn=CalculateContentsScale&rcl= > 1411534708 Sure, that's fine, we don't use that we use the ideal scale instead with implside painting. I'm wondering why the ideal scale being computed in LTHCommon is different than the "raster*dsf*psf" that was also being computed there for non-implside. Whereas, the ideal contents scale remains the same across impl/non-impl > world. > > Should we raise a bug and re-calculate scales in PictureLayer ? > > https://codereview.chromium.org/574343003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/09/25 14:18:27, danakj wrote: > On Thu, Sep 25, 2014 at 7:01 AM, <mailto:sohan.jyoti@samsung.com> wrote: > > > On 2014/09/24 15:23:58, danakj wrote: > > > >> On Wed, Sep 24, 2014 at 11:21 AM, <mailto:sohan.jyoti@samsung.com> wrote: > >> > > > > > > >> > https://codereview.chromium.org/574343003/diff/80001/cc/ > >> > trees/layer_tree_host_common_unittest.cc > >> > File cc/trees/layer_tree_host_common_unittest.cc (right): > >> > > >> > https://codereview.chromium.org/574343003/diff/80001/cc/ > >> > >> > trees/layer_tree_host_common_unittest.cc#newcode4291 > >> > cc/trees/layer_tree_host_common_unittest.cc:4291: gfx::Transform > >> > transform = scale_small_matrix; > >> > On 2014/09/24 15:11:26, danakj wrote: > >> > > >> >> On 2014/09/24 15:09:31, sohanjg wrote: > >> >> > On 2014/09/24 14:38:16, danakj wrote: > >> >> > > Did you figure out why this is a factor in the scale we choose in > >> >> > >> > implside > >> > > >> >> but > >> >> > > not in the old path? > >> >> > > >> >> > When i checked in non impl-path, the ideal scale value was coming > >> >> > >> > same as in > >> > > >> >> > impl-path (i.e device_scale_factor * page_scale_factor / 10) > >> >> > >> > > >> > But the non-impl path doesn't use that scale does it? It's contents > >> >> > >> > scale was > >> > > >> >> not using this transform. > >> >> > >> > > >> > No, for non-impl path, contents scale was not using this transform. > >> > But if we queried the ideal contents scale we found it to be using the > >> > transform. > >> > > >> > i think in here we only do CalculateDrawProperties, and we dont go to > >> > layer impls to set contents scale to ideal contents scale, hence the > >> > difference between ideal and contents scale for layer with scaled > >> > transform? > >> > > >> > > > > What I mean is, in non-impl painting, the contents scale was used to > >> raster > >> contents. In impl-painting, the raster scale is set to the ideal scale. So > >> this means we're producing different pixel output in this case with > >> impl-side. That's how I understand it, and that seems like a bug. I don't > >> think we should change the expectations of this test. > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > Yes, we're producing different pixel output. > > Looks like ContentsScalingLayer making the contents scale as |raster_scale > > * > > device_scale_factor * page_scale_factor| > > > > https://code.google.com/p/chromium/codesearch#chromium/ > > src/cc/trees/layer_tree_host_common.cc&sq=package:chromium& > > q=UpdateLayerContentsScale&l=1014 > > > > > LayerTreeHostCommon is supposed to also compute the ideal scale in the same > way, it should be source * dsf * psf (source scale was called raster scale > in the old code). > > > > is the main difference between the 2 paths, for impl painting we will set > > contents scale as |1| > > > > https://code.google.com/p/chromium/codesearch#chromium/ > > src/cc/layers/layer.cc&cl=GROK&l=411&gsn=CalculateContentsScale&rcl= > > 1411534708 > > > Sure, that's fine, we don't use that we use the ideal scale instead with > implside painting. I'm wondering why the ideal scale being computed in > LTHCommon is different than the "raster*dsf*psf" that was also being > computed there for non-implside. > Looks like, the ideal_raster_scale which is calculated in UpdateLayerContentsScale, is not updated as raster_scale because it is |< 1.f| and kept as |1| Hence in non-impl case, contents scale = |raster* dsf*psf| but, ideal scale remains as |raster*dsf*psf| in both impl and non-impl case. > Whereas, the ideal contents scale remains the same across impl/non-impl > > world. > > > > Should we raise a bug and re-calculate scales in PictureLayer ? > > > > https://codereview.chromium.org/574343003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Fri, Sep 26, 2014 at 2:05 AM, <sohan.jyoti@samsung.com> wrote: > On 2014/09/25 14:18:27, danakj wrote: > > On Thu, Sep 25, 2014 at 7:01 AM, <mailto:sohan.jyoti@samsung.com> wrote: >> > > > On 2014/09/24 15:23:58, danakj wrote: >> > >> >> On Wed, Sep 24, 2014 at 11:21 AM, <mailto:sohan.jyoti@samsung.com> >> wrote: >> >> >> > >> > > >> >> > https://codereview.chromium.org/574343003/diff/80001/cc/ >> >> > trees/layer_tree_host_common_unittest.cc >> >> > File cc/trees/layer_tree_host_common_unittest.cc (right): >> >> > >> >> > https://codereview.chromium.org/574343003/diff/80001/cc/ >> >> >> >> > trees/layer_tree_host_common_unittest.cc#newcode4291 >> >> > cc/trees/layer_tree_host_common_unittest.cc:4291: gfx::Transform >> >> > transform = scale_small_matrix; >> >> > On 2014/09/24 15:11:26, danakj wrote: >> >> > >> >> >> On 2014/09/24 15:09:31, sohanjg wrote: >> >> >> > On 2014/09/24 14:38:16, danakj wrote: >> >> >> > > Did you figure out why this is a factor in the scale we choose >> in >> >> >> >> >> > implside >> >> > >> >> >> but >> >> >> > > not in the old path? >> >> >> > >> >> >> > When i checked in non impl-path, the ideal scale value was coming >> >> >> >> >> > same as in >> >> > >> >> >> > impl-path (i.e device_scale_factor * page_scale_factor / 10) >> >> >> >> >> > >> >> > But the non-impl path doesn't use that scale does it? It's contents >> >> >> >> >> > scale was >> >> > >> >> >> not using this transform. >> >> >> >> >> > >> >> > No, for non-impl path, contents scale was not using this transform. >> >> > But if we queried the ideal contents scale we found it to be using >> the >> >> > transform. >> >> > >> >> > i think in here we only do CalculateDrawProperties, and we dont go to >> >> > layer impls to set contents scale to ideal contents scale, hence the >> >> > difference between ideal and contents scale for layer with scaled >> >> > transform? >> >> > >> >> >> > >> > What I mean is, in non-impl painting, the contents scale was used to >> >> raster >> >> contents. In impl-painting, the raster scale is set to the ideal >> scale. So >> >> this means we're producing different pixel output in this case with >> >> impl-side. That's how I understand it, and that seems like a bug. I >> don't >> >> think we should change the expectations of this test. >> >> >> > >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> >> >> > email >> > >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> > >> > Yes, we're producing different pixel output. >> > Looks like ContentsScalingLayer making the contents scale as >> |raster_scale >> > * >> > device_scale_factor * page_scale_factor| >> > >> > https://code.google.com/p/chromium/codesearch#chromium/ >> > src/cc/trees/layer_tree_host_common.cc&sq=package:chromium& >> > q=UpdateLayerContentsScale&l=1014 >> > >> > >> LayerTreeHostCommon is supposed to also compute the ideal scale in the >> same >> way, it should be source * dsf * psf (source scale was called raster >> scale >> in the old code). >> > > > > is the main difference between the 2 paths, for impl painting we will >> set >> > contents scale as |1| >> > >> > https://code.google.com/p/chromium/codesearch#chromium/ >> > src/cc/layers/layer.cc&cl=GROK&l=411&gsn=CalculateContentsScale&rcl= >> > 1411534708 >> > > > Sure, that's fine, we don't use that we use the ideal scale instead with >> implside painting. I'm wondering why the ideal scale being computed in >> LTHCommon is different than the "raster*dsf*psf" that was also being >> computed there for non-implside. >> > > > Looks like, the ideal_raster_scale which is calculated in > UpdateLayerContentsScale, is not updated as raster_scale because it is |< > 1.f| > and kept as |1| > Hence in non-impl case, contents scale = |raster* dsf*psf| > but, ideal scale remains as |raster*dsf*psf| in both impl and non-impl > case. Ahh okay, so in non-impl painting we just don't allow a source scale < 1, and that's the difference. Thanks for figuring that out. The surface in this case should have a similar scale. Can you also verify that the the max 2d scale component of the layer's target_space_transform is the same value in the test also? You can comment that this shows that the ideal scale will draw 1:1 with its render target space along the larger-scale axis. > > > Whereas, the ideal contents scale remains the same across impl/non-impl >> > world. >> > >> > Should we raise a bug and re-calculate scales in PictureLayer ? >> > >> > https://codereview.chromium.org/574343003/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > https://codereview.chromium.org/574343003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/09/26 14:47:43, danakj wrote: > On Fri, Sep 26, 2014 at 2:05 AM, <mailto:sohan.jyoti@samsung.com> wrote: > > > On 2014/09/25 14:18:27, danakj wrote: > > > > On Thu, Sep 25, 2014 at 7:01 AM, <mailto:sohan.jyoti@samsung.com> wrote: > >> > > > > > On 2014/09/24 15:23:58, danakj wrote: > >> > > >> >> On Wed, Sep 24, 2014 at 11:21 AM, <mailto:sohan.jyoti@samsung.com> > >> wrote: > >> >> > >> > > >> > > > >> >> > https://codereview.chromium.org/574343003/diff/80001/cc/ > >> >> > trees/layer_tree_host_common_unittest.cc > >> >> > File cc/trees/layer_tree_host_common_unittest.cc (right): > >> >> > > >> >> > https://codereview.chromium.org/574343003/diff/80001/cc/ > >> >> > >> >> > trees/layer_tree_host_common_unittest.cc#newcode4291 > >> >> > cc/trees/layer_tree_host_common_unittest.cc:4291: gfx::Transform > >> >> > transform = scale_small_matrix; > >> >> > On 2014/09/24 15:11:26, danakj wrote: > >> >> > > >> >> >> On 2014/09/24 15:09:31, sohanjg wrote: > >> >> >> > On 2014/09/24 14:38:16, danakj wrote: > >> >> >> > > Did you figure out why this is a factor in the scale we choose > >> in > >> >> >> > >> >> > implside > >> >> > > >> >> >> but > >> >> >> > > not in the old path? > >> >> >> > > >> >> >> > When i checked in non impl-path, the ideal scale value was coming > >> >> >> > >> >> > same as in > >> >> > > >> >> >> > impl-path (i.e device_scale_factor * page_scale_factor / 10) > >> >> >> > >> >> > > >> >> > But the non-impl path doesn't use that scale does it? It's contents > >> >> >> > >> >> > scale was > >> >> > > >> >> >> not using this transform. > >> >> >> > >> >> > > >> >> > No, for non-impl path, contents scale was not using this transform. > >> >> > But if we queried the ideal contents scale we found it to be using > >> the > >> >> > transform. > >> >> > > >> >> > i think in here we only do CalculateDrawProperties, and we dont go to > >> >> > layer impls to set contents scale to ideal contents scale, hence the > >> >> > difference between ideal and contents scale for layer with scaled > >> >> > transform? > >> >> > > >> >> > >> > > >> > What I mean is, in non-impl painting, the contents scale was used to > >> >> raster > >> >> contents. In impl-painting, the raster scale is set to the ideal > >> scale. So > >> >> this means we're producing different pixel output in this case with > >> >> impl-side. That's how I understand it, and that seems like a bug. I > >> don't > >> >> think we should change the expectations of this test. > >> >> > >> > > >> > To unsubscribe from this group and stop receiving emails from it, send > >> an > >> >> > >> > email > >> > > >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> >> > >> > > >> > Yes, we're producing different pixel output. > >> > Looks like ContentsScalingLayer making the contents scale as > >> |raster_scale > >> > * > >> > device_scale_factor * page_scale_factor| > >> > > >> > https://code.google.com/p/chromium/codesearch#chromium/ > >> > src/cc/trees/layer_tree_host_common.cc&sq=package:chromium& > >> > q=UpdateLayerContentsScale&l=1014 > >> > > >> > > >> LayerTreeHostCommon is supposed to also compute the ideal scale in the > >> same > >> way, it should be source * dsf * psf (source scale was called raster > >> scale > >> in the old code). > >> > > > > > > > is the main difference between the 2 paths, for impl painting we will > >> set > >> > contents scale as |1| > >> > > >> > https://code.google.com/p/chromium/codesearch#chromium/ > >> > src/cc/layers/layer.cc&cl=GROK&l=411&gsn=CalculateContentsScale&rcl= > >> > 1411534708 > >> > > > > > > Sure, that's fine, we don't use that we use the ideal scale instead with > >> implside painting. I'm wondering why the ideal scale being computed in > >> LTHCommon is different than the "raster*dsf*psf" that was also being > >> computed there for non-implside. > >> > > > > > > Looks like, the ideal_raster_scale which is calculated in > > UpdateLayerContentsScale, is not updated as raster_scale because it is |< > > 1.f| > > and kept as |1| > > Hence in non-impl case, contents scale = |raster* dsf*psf| > > but, ideal scale remains as |raster*dsf*psf| in both impl and non-impl > > case. > > > Ahh okay, so in non-impl painting we just don't allow a source scale < 1, > and that's the difference. Thanks for figuring that out. > > The surface in this case should have a similar scale. Can you also verify > that the the max 2d scale component of the layer's target_space_transform > is the same value in the test also? You can comment that this shows that > the ideal scale will draw 1:1 with its render target space along the > larger-scale axis. > Updated. Can you please take a look. thanks. > > > > > > > Whereas, the ideal contents scale remains the same across impl/non-impl > >> > world. > >> > > >> > Should we raise a bug and re-calculate scales in PictureLayer ? > >> > > >> > https://codereview.chromium.org/574343003/ > >> > > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > > > https://codereview.chromium.org/574343003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Hi, so it seems like a lot of tests were testing the ContentsScalingLayer interaction with LTHCommon, which doesn't exist with PictureLayer at all. So some of these tests can just be removed (but we should probably just TODO() to remove them for now so we keep coverage for browser compositor for now). https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:3365: // layer, so they are also not scaled by the device_scale_factor. s/also// https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4020: class NoScalePictureLayer : public PictureLayer { PictureLayer does not inherit from ContentLayer, so this class is no different than just using FakePictureLayer, so shall we just use that instead? https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4116: scoped_refptr<NoScalePictureLayer> child_no_scale = this is no different from |child| anymore, remove it. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4179: expected_child_transform.Scale(device_scale_factor, device_scale_factor); do the Scale() before the Translate() and you don't have to multiply the translate components by DSF explicitly https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4289: // ideal scale is the max of combined transform (transform hierarchy // Ideal scale is the max 2d scale component of the combined transform up to the nearest render target. Here this includes the layer transform as well as the device and page scale factors. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4304: device_scale_factor * page_scale_factor); use 0.f as the 2nd argument? https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4307: max_2d_scale); put the expected value first in the EXPECT https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4326: gfx::Transform expected_scale_surface_layer_draw_transform = just remove this and the check below on line 4331, it's not relevant anymore https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4346: gfx::Transform expected_perspective_surface_layer_draw_transform; also remove this and the check for it on 4353 https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4359: LayerTransformsInHighDPIAccurateScaleZeroChildPosition) { I don't think this test is checking anything anymore is it? Can you just leave it unchanged and put a TODO() to remove it when we remove ContentLayer? https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4478: TEST_F(LayerTreeHostCommonTest, IdealScale) { Is this testing anything the tests above it do not already cover? It seems not to me? Can we leave it unchanged and put a TODO() to remove it when we remove ContentLayer? https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4600: IdealScale_LayerTransformsDontAffectIdealScale) { this is testing the opposite of the test name now, and we already test the ideal scale is affected by transforms in other tests. how about leave this test unchanged and a TODO() to remove it with ContentLayer also? https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4767: EXPECT_IDEAL_SCALE_EQ(device_scale_factor * page_scale_factor * Let's show that the scale is going below 1 now. // The ideal scale is able to go below 1. float expected_ideal_scale = foo * bar * baz....; EXPECT_LT(expected_ideal_scale, 1.f); EXPECT_IDEAL_SCALE_EQ(expected_ideal_scale, child_scale); https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4772: // When chilld's total scale becomes >= 1, we should save and use that scale This part of the test doesn't really mean much anymore since we're always using the fully ideal scale, so I'd just remove from here down in the test. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4842: scoped_refptr<NoScalePictureLayer> surface_scale_child_no_scale = this is the same as surface_scale_child_scale, remove it https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4852: scoped_refptr<NoScaleContentLayer> surface_no_scale = why are these content layers? also if you changed it to a picture layer, this is the same as surface_scale, we could remove it. but actually, this test seems to be entirely testing things that just don't matter for PictureLayer. leave it unchanged and TODO() to remove it? https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5022: IdealScaleForSurfaces_LayerTransformsDontAffectIdealScale) { same here, leave it and TODO() to remove it https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5229: TEST_F(LayerTreeHostCommonTest, ContentsScaleForAnimatingLayer) { IdealScale https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5282: EXPECT_IDEAL_SCALE_EQ(initial_parent_scale, parent); // Animating layers compute ideal scale in the same way as when they are static. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5304: ChangeInContentBoundsOrScaleTriggersPushProperties) { This is no longer testing anything on picture layer, leave it along and TODO() to remove it? https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5429: expected_screen_space_transform.Scale(device_scale_factor, Scale() before Translate() and you don't have to multiply by DSF explicitly https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5554: gfx::Transform transform; can you just remove these checks for the layer transforms from this test? it's really for testing the surfaces stuff below now (ie remove line 5554-5567). also the duplicate_child_non_owner has no purpose in this test and can be removed
Thanks for going through it, i have updated it as per your comments, please take a look. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:3365: // layer, so they are also not scaled by the device_scale_factor. On 2014/10/06 18:53:02, danakj wrote: > s/also// you mean remove the *also* from comment ? https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4020: class NoScalePictureLayer : public PictureLayer { On 2014/10/06 18:53:02, danakj wrote: > PictureLayer does not inherit from ContentLayer, so this class is no different > than just using FakePictureLayer, so shall we just use that instead? Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4116: scoped_refptr<NoScalePictureLayer> child_no_scale = On 2014/10/06 18:53:02, danakj wrote: > this is no different from |child| anymore, remove it. Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4179: expected_child_transform.Scale(device_scale_factor, device_scale_factor); On 2014/10/06 18:53:02, danakj wrote: > do the Scale() before the Translate() and you don't have to multiply the > translate components by DSF explicitly Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4289: // ideal scale is the max of combined transform (transform hierarchy On 2014/10/06 18:53:02, danakj wrote: > // Ideal scale is the max 2d scale component of the combined transform up to the > nearest render target. Here this includes the layer transform as well as the > device and page scale factors. Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4304: device_scale_factor * page_scale_factor); On 2014/10/06 18:53:01, danakj wrote: > use 0.f as the 2nd argument? Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4307: max_2d_scale); On 2014/10/06 18:53:02, danakj wrote: > put the expected value first in the EXPECT Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4326: gfx::Transform expected_scale_surface_layer_draw_transform = On 2014/10/06 18:53:02, danakj wrote: > just remove this and the check below on line 4331, it's not relevant anymore Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4346: gfx::Transform expected_perspective_surface_layer_draw_transform; On 2014/10/06 18:53:02, danakj wrote: > also remove this and the check for it on 4353 Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4359: LayerTransformsInHighDPIAccurateScaleZeroChildPosition) { On 2014/10/06 18:53:01, danakj wrote: > I don't think this test is checking anything anymore is it? Can you just leave > it unchanged and put a TODO() to remove it when we remove ContentLayer? Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4478: TEST_F(LayerTreeHostCommonTest, IdealScale) { On 2014/10/06 18:53:02, danakj wrote: > Is this testing anything the tests above it do not already cover? It seems not > to me? Can we leave it unchanged and put a TODO() to remove it when we remove > ContentLayer? Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4600: IdealScale_LayerTransformsDontAffectIdealScale) { On 2014/10/06 18:53:01, danakj wrote: > this is testing the opposite of the test name now, and we already test the ideal > scale is affected by transforms in other tests. > > how about leave this test unchanged and a TODO() to remove it with ContentLayer > also? Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4767: EXPECT_IDEAL_SCALE_EQ(device_scale_factor * page_scale_factor * On 2014/10/06 18:53:02, danakj wrote: > Let's show that the scale is going below 1 now. > > // The ideal scale is able to go below 1. > float expected_ideal_scale = foo * bar * baz....; > EXPECT_LT(expected_ideal_scale, 1.f); > EXPECT_IDEAL_SCALE_EQ(expected_ideal_scale, child_scale); Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4772: // When chilld's total scale becomes >= 1, we should save and use that scale On 2014/10/06 18:53:02, danakj wrote: > This part of the test doesn't really mean much anymore since we're always using > the fully ideal scale, so I'd just remove from here down in the test. Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4842: scoped_refptr<NoScalePictureLayer> surface_scale_child_no_scale = On 2014/10/06 18:53:02, danakj wrote: > this is the same as surface_scale_child_scale, remove it Acknowledged. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4852: scoped_refptr<NoScaleContentLayer> surface_no_scale = On 2014/10/06 18:53:02, danakj wrote: > why are these content layers? also if you changed it to a picture layer, this is > the same as surface_scale, we could remove it. > > but actually, this test seems to be entirely testing things that just don't > matter for PictureLayer. leave it unchanged and TODO() to remove it? Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5022: IdealScaleForSurfaces_LayerTransformsDontAffectIdealScale) { On 2014/10/06 18:53:02, danakj wrote: > same here, leave it and TODO() to remove it Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5229: TEST_F(LayerTreeHostCommonTest, ContentsScaleForAnimatingLayer) { On 2014/10/06 18:53:01, danakj wrote: > IdealScale Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5282: EXPECT_IDEAL_SCALE_EQ(initial_parent_scale, parent); On 2014/10/06 18:53:02, danakj wrote: > // Animating layers compute ideal scale in the same way as when they are static. Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5304: ChangeInContentBoundsOrScaleTriggersPushProperties) { On 2014/10/06 18:53:01, danakj wrote: > This is no longer testing anything on picture layer, leave it along and TODO() > to remove it? Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5429: expected_screen_space_transform.Scale(device_scale_factor, On 2014/10/06 18:53:02, danakj wrote: > Scale() before Translate() and you don't have to multiply by DSF explicitly Done. https://codereview.chromium.org/574343003/diff/100001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5554: gfx::Transform transform; On 2014/10/06 18:53:02, danakj wrote: > can you just remove these checks for the layer transforms from this test? it's > really for testing the surfaces stuff below now (ie remove line 5554-5567). also > the duplicate_child_non_owner has no purpose in this test and can be removed Done.
https://codereview.chromium.org/574343003/diff/140001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/140001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4270: transform, device_scale_factor * page_scale_factor); pass 0.f as the 2nd parameter. we should always get the scale from the transform here, we don't want a valid-looking fallback value https://codereview.chromium.org/574343003/diff/140001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5271: // Remove the animation, now it can save a raster scale. remove from here to the end of this test, it's not testing anything new.
LGTM with those changes. thanks for working through all these tests, there are sure a lot of them
Thanks for going through this. https://codereview.chromium.org/574343003/diff/140001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/574343003/diff/140001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:4270: transform, device_scale_factor * page_scale_factor); On 2014/10/08 21:13:52, danakj wrote: > pass 0.f as the 2nd parameter. we should always get the scale from the transform > here, we don't want a valid-looking fallback value Done. https://codereview.chromium.org/574343003/diff/140001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5271: // Remove the animation, now it can save a raster scale. On 2014/10/08 21:13:52, danakj wrote: > remove from here to the end of this test, it's not testing anything new. Done.
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/574343003/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e3bd619e1875337517ec496f9e8cb818b304fcd5 Cr-Commit-Position: refs/heads/master@{#299269}
Message was sent while issue was closed.
Description was changed from ========== cc: Change LTHCommon tests to use impl painting. Make LTHCommon tests to use PictureLayers instead of ContentLayer, and update the scaling tests to use ideal scales instead of contents scale accordingly. BUG=401492 Committed: https://crrev.com/e3bd619e1875337517ec496f9e8cb818b304fcd5 Cr-Commit-Position: refs/heads/master@{#299269} ========== to ========== cc: Change LTHCommon tests to use impl painting. Make LTHCommon tests to use PictureLayers instead of ContentLayer, and update the scaling tests to use ideal scales instead of contents scale accordingly. BUG=401492 Committed: https://crrev.com/e3bd619e1875337517ec496f9e8cb818b304fcd5 Cr-Commit-Position: refs/heads/master@{#299269} ========== |
