Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
271533011:
cc: Move tiling management out of draw properties calculation. (Closed)
|
Created:
6 years, 7 months ago by sohanjg Modified:
5 years, 1 month ago CC:
cc-bugs_chromium.org, chromium-reviews, reveman, vmpstr Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptioncc: Move tiling management out of draw properties calculation.
This is mainly plumbing code.
As part of the changes, we are moving ManageTiling as part of
UpdateTilePriorities.
We keep picturelayers content scale as 1,and maintain the remaining
required scales in draw properties. This scales we apply later,
to shared quad state and draw transform during AppendQuads.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277000
Patch Set 1 #
Total comments: 16
Patch Set 2 : WIP - review comments #
Total comments: 1
Patch Set 3 : WIP - resolve wrong scaling. #Patch Set 4 : WIP - use properly scaled content_bounds #
Total comments: 14
Patch Set 5 : WIP - review comments #
Total comments: 8
Patch Set 6 : Review comments added, unittests pending. #
Total comments: 3
Patch Set 7 : WIP - unit tests updation #Patch Set 8 : WIP - UnitTests #Patch Set 9 : WIP - added test PictureLayerImplTest.VerifySharedQuadState #Patch Set 10 : PictureImageLayerImpl unittests updated. #
Total comments: 20
Patch Set 11 : review comments + unit/perf test fails addressed #
Total comments: 13
Patch Set 12 : unit tests + review comments #
Total comments: 9
Patch Set 13 : review comments + addl tests #Patch Set 14 : Updated PictureLayerImplTest.LayerEvictionTileIterator test. #
Total comments: 5
Patch Set 15 : review comments #
Total comments: 2
Patch Set 16 : debug build unit test failures addressed #
Total comments: 4
Patch Set 17 : review comments addressed #
Total comments: 3
Patch Set 18 : update content_to_screen_scale with max tiling scale #Patch Set 19 : rebase + review commnets #
Total comments: 8
Patch Set 20 : resubmit updating content_to_screen_scale with max tiling scale #Patch Set 21 : change updatetilepriorities params #Patch Set 22 : Fix compilation error in LTHCommon unittest #Patch Set 23 : chromeos build fix (missing scale initialization in draw prop ctor + test update) #Patch Set 24 : rebase + update test #
Total comments: 5
Patch Set 25 : handle scales for mask and replica mask layer #
Total comments: 2
Patch Set 26 : rename func + test update #Patch Set 27 : rebase to TOT #Messages
Total messages: 78 (1 generated)
this is an initial version, can you please have a look if this is what we want ? https://codereview.chromium.org/271533011/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/271533011/diff/1/cc/trees/layer_tree_impl.cc#... cc/trees/layer_tree_impl.cc:513: will this be the best place to managetiling ?
https://codereview.chromium.org/271533011/diff/1/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/271533011/diff/1/cc/layers/layer_impl.h#newco... cc/layers/layer_impl.h:185: virtual void ManageTilings(bool animating_transform_to_screen, Please don't add this to LayerImpl. Too many virtuals already. https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:149: scaled_transform.Scale(contents_scale_x(), contents_scale_x()); This contents_scale is the wrong value. You need to move the code from CalculateContentsScale that finds max_contents_scale and use that scale here. https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:191: tilings_.get(), contents_scale_x(), rect, ideal_contents_scale_); This should be max_contents_scale. https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:242: tilings_.get(), contents_scale_x(), rect, ideal_contents_scale_); This should be max_contents_scale. https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:520: *contents_scale_x = max_contents_scale; You need to remove this whole block of code and just set contents_scale_x = 1, contents_scale_y = 1, and content_bounds = layer_bounds. If you don't, then you will "double scale" when you add the scale above in AppendQuads. https://codereview.chromium.org/271533011/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/271533011/diff/1/cc/trees/layer_tree_impl.cc#... cc/trees/layer_tree_impl.cc:513: On 2014/05/13 15:56:59, sohanjg wrote: > will this be the best place to managetiling ? No. I think UpdateTilePriorities should call ManageTilings itself.
https://codereview.chromium.org/271533011/diff/1/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/271533011/diff/1/cc/layers/layer_impl.h#newco... cc/layers/layer_impl.h:185: virtual void ManageTilings(bool animating_transform_to_screen, If this is not being called mid-CalcDrawProps, it can safely use the layer's draw properties. This one is screen_space_transform_is_animating https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/draw_pro... https://codereview.chromium.org/271533011/diff/1/cc/layers/layer_impl.h#newco... cc/layers/layer_impl.h:186: float maximum_animation_contents_scale) {} This one probably needs to be added to the DrawProperties https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (left): https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:521: *contents_scale_x = max_contents_scale; we would always set these to 1 now, and move this loop to compute the max scale over to AppendQuads. https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:149: scaled_transform.Scale(contents_scale_x(), contents_scale_x()); the contents_scale_x() is going to be 1 always now, right? so instead here we should compute the scale we want to use, which would be the max scale of all the picture layer impl's tilings. https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:242: tilings_.get(), contents_scale_x(), rect, ideal_contents_scale_); we should use the computed scale from above everywhere instead of the contents_scale_x() which is 1? https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:504: ideal_contents_scale_ = std::max(ideal_contents_scale, min_contents_scale); we still need to compute these values, but during CalculateContentsScale(), but we need to do them before ManageTilings, or move them over into ManageTilings. Maybe rename this method to ComputeIdealScales() and have ManageTilings call it? We'll have to get the various inputs to this from other places such as the layer draw properties. DeviceScaleFactor should be available on the LayerTreeImpl. animating_transform_to_screen is on the DrawProperties. page scale and maximum animation scale I believe you'll need to store on the DrawProperties to make them available to this method. https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:727: float scale = contents_scale_x(); This should also be the max scale from the tilings https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:985: if (!change_target_tiling || !IsDrawnRenderSurfaceLayerListMember()) Let's do this change separately
https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:520: *contents_scale_x = max_contents_scale; On 2014/05/13 18:04:09, enne wrote: > You need to remove this whole block of code and just set contents_scale_x = 1, > contents_scale_y = 1, and content_bounds = layer_bounds. If you don't, then you > will "double scale" when you add the scale above in AppendQuads. The LayerImpl implementation of this method should do what we want, so PictureLayerImpl doesn't need to override it anymore.
can you please take a look. using max_contents_scale in ::AppendQuads as max of all tiling content_scale is causing the page to be rendered at a higher scale, is there something im missing here. thanks.
https://codereview.chromium.org/271533011/diff/40001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/40001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:149: scaled_transform.Scale(max_contents_scale, max_contents_scale); I think you want to scale by 1/max_contents_scale here?
thanks for your inputs danakj@, the scaling issue is solved. for visible_content_rect if we take enclosing rect and intersect with bounds, we are getting wrong rect, so keeping as simple scaling. can you please take a look, thanks.
On Thu, May 15, 2014 at 6:26 AM, <sohan.jyoti@samsung.com> wrote: > thanks for your inputs danakj@, the scaling issue is solved. > for visible_content_rect if we take enclosing rect and intersect with > bounds, we > are getting wrong rect, so keeping as simple scaling. > can you please take a look, thanks. > Can you explain why it's wrong? What are you getting vs what should you get? It should be intersected with the scaled content_bounds rect, you can't have visible content outside of your content space. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/05/15 14:18:06, danakj wrote: > On Thu, May 15, 2014 at 6:26 AM, <mailto:sohan.jyoti@samsung.com> wrote: > > > thanks for your inputs danakj@, the scaling issue is solved. > > for visible_content_rect if we take enclosing rect and intersect with > > bounds, we > > are getting wrong rect, so keeping as simple scaling. > > can you please take a look, thanks. > > > > Can you explain why it's wrong? What are you getting vs what should you > get? It should be intersected with the scaled content_bounds rect, you > can't have visible content outside of your content space. > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. sorry, i was intersecting with non-scaled content_bounds, hence the problem. i have rectified it, can you please take a look. thanks.
https://codereview.chromium.org/271533011/diff/100001/cc/layers/draw_properti... File cc/layers/draw_properties.h (right): https://codereview.chromium.org/271533011/diff/100001/cc/layers/draw_properti... cc/layers/draw_properties.h:132: float ideal_contents_scale; Can you add a comment for each of these describing what it is, if you're not sure what to write I can help with it. https://codereview.chromium.org/271533011/diff/100001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/100001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1067: if (layer_tree_impl()->settings().impl_side_painting) { Why did you need this? The bounds == content_bounds for picture layer now, no? And the scales given from the main thread should be 1? https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:145: gfx::Transform scaled_transform = draw_transform(); scaled_draw_transform https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:148: gfx::Size bounds( name this scaled_content_bounds https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:149: gfx::Size(std::ceil(content_bounds().width() * max_contents_scale), Use gfx::ToCeiledSize(gfx::ScaleSize()) to do this https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:152: gfx::Rect scaled_rect = scaled_visible_content_rect https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:153: gfx::Rect(visible_content_rect().x() * max_contents_scale, Use gfx::ScaleToEnclosingRect() to do this. https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:161: draw_properties().visible_content_rect = rect; The draw properties should stay unchanged here, we just don't want to use all of them anymore. https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:165: PopulateSharedQuadState(shared_quad_state); The SQS transform/visible_content_rect/content_bounds need to be updated though. https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:207: tilings_.get(), contents_scale_x(), rect, ideal_contents_scale_); because you don't want to change the draw_props, you'll use the local computed values here instead of accessors to them like contents_scale_x() https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:660: if (!IsDrawnRenderSurfaceLayerListMember()) Do this in a separate CL? This isn't related right? https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:945: if (!IsDrawnRenderSurfaceLayerListMember()) Same here, separate CL? https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1257: DoPostCommitInitializationIfNeeded(); Move this up to the UpdateTilePrios method please https://codereview.chromium.org/271533011/diff/100001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1260: ideal_page_scale_ = layer_tree_impl()->page_scale_factor(); This value is different from before, this is the value from the main thread. You can use the total_page_scale_factor() instead, but to get precisely the same behaviour as before you'd need to store the page scale per-layer the way you store the ideal_contents_scale
can you please take a look, i have written some comments for the vars in draw properties, maybe it will need some edit. thanks.
This looks great! A few small comments. How does this do on the cc_unittest suite, any failures? Can you add a PictureLayerImpl unit test that makes the layer have some ideal scale != 1 and verifies the draw transform on the SQS, and the quads it produces have their values scaled correctly? https://codereview.chromium.org/271533011/diff/120001/cc/layers/draw_properti... File cc/layers/draw_properties.h (right): https://codereview.chromium.org/271533011/diff/120001/cc/layers/draw_properti... cc/layers/draw_properties.h:132: // Content scales for the layer, used to set scales for tiling and raster. how about "The scale at which content for the layer should be rastered in order to be perfectly crisp." https://codereview.chromium.org/271533011/diff/120001/cc/layers/draw_properti... cc/layers/draw_properties.h:134: float ideal_contents_scale; empty line between each one https://codereview.chromium.org/271533011/diff/120001/cc/layers/draw_properti... cc/layers/draw_properties.h:135: // Max animation scale. "The maximum scale during the layers current animation at which content should be rastered at to be crisp." https://codereview.chromium.org/271533011/diff/120001/cc/layers/draw_properti... cc/layers/draw_properties.h:137: // Page scale factor. "The page scale factor that is applied to the layer. Since some layers may have page scale applied and others not, this may differ between layers." https://codereview.chromium.org/271533011/diff/120001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/271533011/diff/120001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:162: void UpdateSharedQuadState(SharedQuadState* state, let's name this PopulateSharedQuadStateWithOverrides https://codereview.chromium.org/271533011/diff/120001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/120001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:152: gfx::Rect rect = gfx::ToEnclosingRect(scaled_visible_content_rect); this ToEnclosingRect() is redundant since scaled_visible_content_rect is already a gfx::Rect https://codereview.chromium.org/271533011/diff/120001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:156: shared_quad_state, scaled_draw_transform, scaled_content_bounds, rect); can you pass the scaled_visible_content_rect here, that name is more meaningful. |rect| is a shortcut for the code below https://codereview.chromium.org/271533011/diff/120001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/271533011/diff/120001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:178: float GetMaxContentScale() const; let's call this MaximumTilingContentScale()
can you please take a look, i will update the unit tests soon. thanks.
Looks good thanks :) https://codereview.chromium.org/271533011/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:152: gfx::Rect rect = scaled_visible_content_rect; one nit, move this |rect| down below PopulateSQSWithOverrides, since we don't use it for that. https://codereview.chromium.org/271533011/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1285: state->SetDouble("geometry_contents_scale", contents_scale_x()); oh, can you change this to MaximumTilingContentsScale(), that's what we'll want to see in traces. Sorry I lost this comment on the last round somehow.
https://codereview.chromium.org/271533011/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:378: DCHECK(!needs_post_commit_initialization_); Just a note, this DCHECK will probably fail if you expect to do post commit initialization right below
A lot of existing unit tests in PictureLayerImplTest.*, needs modifying for these. I have taken care of most of them (36/43 passes now). Can you please have a look, if the general direction is correct for these. I will fix the remaining + add for this code change. thanks.
Fixed the PictureLayerImplTest.* tests. Some, PictureLayerImpl had to be modifed, most imp is do ManageTilings from inside CanHaveTiling, also shouldnt we now use MaximumTilingContentScale everywhere we query contents_scale_x ? Also, We need to take a decision on PictureImageLayerImpl, whether we need to implement CalculateContentsScale for it anymore ? Please take a look.
On 2014/05/19 12:18:20, sohanjg wrote: > Fixed the PictureLayerImplTest.* tests. > Some, PictureLayerImpl had to be modifed, most imp is do ManageTilings from > inside CanHaveTiling, also shouldnt we now use MaximumTilingContentScale > everywhere we query contents_scale_x ? > Also, We need to take a decision on PictureImageLayerImpl, whether we need to > implement CalculateContentsScale for it anymore ? > > Please take a look. And should we scale the *rect* in ::MarkVisibleResourcesAsRequired ?
Updated PictureImageLayerImpl and its related tests, can you please take a look, thanks.
https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:382: DCHECK(!needs_post_commit_initialization_); Is this DCHECK valid, considering that you do post commit initialization below?
https://codereview.chromium.org/271533011/diff/220001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/271533011/diff/220001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:171: void PopulateSharedQuadStateWithOverrides( Just stick this function on PictureLayerImpl since there's only one caller. LayerImpl has enough functions as it is. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl.cc (left): https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_image... cc/layers/picture_image_layer_impl.cc:41: ideal_contents_scale = 1.f; How is this behavior not lost? I think you need to override CalculateIdealScales in PictureLayerImpl. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (left): https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:184: Please leave this line. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:424: Please leave this line. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:726: should_update_tile_priorities_) { I don't think this flag or logic should go away. We CalculateDrawProperties on one tree at a time, so it's possible that this layer is syncing to another tree that hasn't had CalculateDrawProperties called on it this frame. In that case, the UpdateTilePriorities work is wrong and wasted. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:787: This line was helpful for readability. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:798: This line was helpful for readability. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1251: float max_contents_scale = 1.f; This is wrong. Replace 1.f with MinimumContentsScale? https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1259: void PictureLayerImpl::CalculateIdealScales() { Could you name this UpdateIdealScales? It's mostly moving values from draw properties into member variables.
Please take a look, thanks. https://codereview.chromium.org/271533011/diff/220001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/271533011/diff/220001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:171: void PopulateSharedQuadStateWithOverrides( On 2014/05/20 17:48:09, enne wrote: > Just stick this function on PictureLayerImpl since there's only one caller. > LayerImpl has enough functions as it is. Done. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl.cc (left): https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_image... cc/layers/picture_image_layer_impl.cc:41: ideal_contents_scale = 1.f; On 2014/05/20 17:48:09, enne wrote: > How is this behavior not lost? I think you need to override CalculateIdealScales > in PictureLayerImpl. Done. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (left): https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:184: On 2014/05/20 17:48:09, enne wrote: > Please leave this line. Done. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:424: On 2014/05/20 17:48:09, enne wrote: > Please leave this line. Done. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:726: should_update_tile_priorities_) { On 2014/05/20 17:48:09, enne wrote: > I don't think this flag or logic should go away. > > We CalculateDrawProperties on one tree at a time, so it's possible that this > layer is syncing to another tree that hasn't had CalculateDrawProperties called > on it this frame. In that case, the UpdateTilePriorities work is wrong and > wasted. Done. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:787: On 2014/05/20 17:48:09, enne wrote: > This line was helpful for readability. Done. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:798: On 2014/05/20 17:48:09, enne wrote: > This line was helpful for readability. Done. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:382: DCHECK(!needs_post_commit_initialization_); On 2014/05/20 17:10:38, vmpstr wrote: > Is this DCHECK valid, considering that you do post commit initialization below? Done. sorry, i missed it in the last round. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1251: float max_contents_scale = 1.f; On 2014/05/20 17:48:09, enne wrote: > This is wrong. Replace 1.f with MinimumContentsScale? Done. https://codereview.chromium.org/271533011/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:1259: void PictureLayerImpl::CalculateIdealScales() { On 2014/05/20 17:48:09, enne wrote: > Could you name this UpdateIdealScales? It's mostly moving values from draw > properties into member variables. Done.
Some comments on the tests. They look good overall. https://codereview.chromium.org/271533011/diff/260001/cc/layers/draw_properti... File cc/layers/draw_properties.h (right): https://codereview.chromium.org/271533011/diff/260001/cc/layers/draw_properti... cc/layers/draw_properties.h:142: float page_scale; nit: add _factor to this and below as that's what we call it everywhere else https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl_unittest.cc (left): https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_image... cc/layers/picture_image_layer_impl_unittest.cc:95: EXPECT_FLOAT_EQ(1.f, contents_scale_x); The equivalent of this test would be to call SetupDrawPropsAndUpdateTilePrios with these same inputs, and then verify what scale the tilings are at (should be 1.f). https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:343: host_impl_.pending_tree()->UpdateDrawProperties(); this needed anymore? https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:1328: pending_layer_->SetContentBounds(layer_bounds); FakePictureLayerImpl constructor sets its bounds. Can you make it set its ContentBounds too? Then you don't need to SetContentBounds for it ever like this. https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:2013: pending_layer_->SetContentsScale(max_scale, max_scale); This is wrong, the layer's contents scale should always be 1. https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:2242: TEST_F(PictureLayerImplTest, VerifySharedQuadState) { rename to SharedQuadStateContainsMaxTilingScale https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:2271: EXPECT_EQ( Add a comment to each of these EXPECT_EQ saying what it's checking. Like "The content_to_target_transform should be scaled by the MaximumTilingContentScale on the layer." https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:2272: scaled_draw_transform, use .ToString() for each of these so we get a nice output when it fails https://codereview.chromium.org/271533011/diff/260001/cc/resources/tile_manag... File cc/resources/tile_manager_perftest.cc (right): https://codereview.chromium.org/271533011/diff/260001/cc/resources/tile_manag... cc/resources/tile_manager_perftest.cc:336: active_root_layer_->SetBounds(gfx::Size(10000, 10000)); Can you do these in SetupDefaultTrees? https://codereview.chromium.org/271533011/diff/260001/cc/resources/tile_manag... File cc/resources/tile_manager_unittest.cc (right): https://codereview.chromium.org/271533011/diff/260001/cc/resources/tile_manag... cc/resources/tile_manager_unittest.cc:823: active_layer_->SetBounds(gfx::Size(1000, 1000)); Maybe do these in SetupDefaultTrees? https://codereview.chromium.org/271533011/diff/260001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/271533011/diff/260001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:1747: layer_draw_properties.ideal_contents_scale = ideal_contents_scale; Can we add a unit test to layer_tree_host_common_unittest.cc for each of these new draw properties. For each one set up a tree so that you get some edge cases for them and verify the draw properties are being set correctly.
https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_image... cc/layers/picture_image_layer_impl.cc:56: draw_properties().ideal_contents_scale = 1.f; In general cc tries not to update draw properties outside of UpdateDrawProperties. Can you do this differently? Maybe just set all the ideal_foo_scale_ variables directly here? Can you add a unit test that PictureImageLayerImpl always generates a PictureLayerTiling with a scale of 1 regardless of what the page/device/contents scales are? Such a test would have caught this regression. https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/260001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:386: should_update_tile_priorities_ = true; Hmm. I guess this at least gets us back to the previous behavior, but it seems like we're still going to needlessly update priorities when syncing to a non-updated tree which will then be updated immediately afterwards. Make this can be fixed in a follow-up.
Updated the patch, there is 1 test failure in *PictureLayerImplTest.LayerEvictionTileIterator*, not able to figure out the reason yet. Please have a look, if the rest of it is ok. Thanks.
https://codereview.chromium.org/271533011/diff/340001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl.h (right): https://codereview.chromium.org/271533011/diff/340001/cc/layers/picture_image... cc/layers/picture_image_layer_impl.h:36: virtual void UpdateIdealScales(); OVERRIDE https://codereview.chromium.org/271533011/diff/340001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/340001/cc/layers/picture_image... cc/layers/picture_image_layer_impl_unittest.cc:62: layer->SetBounds(gfx::Size(100, 200)); can you SetContentBounds() here so you don't need to down below https://codereview.chromium.org/271533011/diff/340001/cc/layers/picture_image... cc/layers/picture_image_layer_impl_unittest.cc:103: EXPECT_FLOAT_EQ(1.f, layer->contents_scale_x()); Verify the MaxTilingContentScale() as well please https://codereview.chromium.org/271533011/diff/340001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/271533011/diff/340001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:134: void PopulateSharedQuadStateWithOverrides( Can this be not public? https://codereview.chromium.org/271533011/diff/340001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:182: void UpdateIdealScales(); this should be virtual, right? https://codereview.chromium.org/271533011/diff/340001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/340001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:2056: nit: don't add whitespace here https://codereview.chromium.org/271533011/diff/340001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/271533011/diff/340001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:8315: TEST_F(LayerTreeHostCommonTest, VerifyLayerDrawProperties) { how about name this "DrawPropertyScales" The "Verify" in a test name is redundant, it's a test, it always verifies. https://codereview.chromium.org/271533011/diff/340001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:8375: EXPECT_FLOAT_EQ(1.f, root_layer->draw_properties().page_scale_factor); Can you set a page scale factor on the host_impl.active_tree(), and make the child2_layer the "page scale layer" passed to CalculateDrawProperties. Then we should see that in its page_scale_factor, and also in its ideal_content_scale and animation scale. And last also give a non-1 device scale factor to the CalculateDrawProperties call, so we see that in all of the scale factors (except not baked into the page_scale_factor). https://codereview.chromium.org/271533011/diff/340001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/340001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:6599: child_of_layer_with_mask->SetContentBounds(bounds); move below SetBounds
Can you please take a look, have made some temp fix for *PictureLayerImplTest.LayerEvictionTileIterator*, please provide your opinion for it. Thanks.
https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:2043: pending_layer_->UpdateTilePriorities(); We UpdateDrawProperties() just below. Does that not do this stuff already? How does this differ?
On 2014/05/27 07:44:56, danakj (OOO_back_june_6) wrote: > https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... > File cc/layers/picture_layer_impl_unittest.cc (right): > > https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... > cc/layers/picture_layer_impl_unittest.cc:2043: > pending_layer_->UpdateTilePriorities(); > We UpdateDrawProperties() just below. Does that not do this stuff already? How > does this differ? Yes. This soln is not proper, actually what we are missing here is in old behavior, we used to set content_bounds, and content_scale_x/y which we are not doing here. So in this test, the visble content rect is not the same (which depends on the content_bounds and transform), which affects the visible layer rect and finally the Tiling priority. If we add a function to setmaxscales in PictureLayerImpl for draw properties, during calculatecontentscale in LayerTreeHostCommon. we can avoid this problem. But that again causes other tests to fail. Is setting maxscales the correct design ?
Can you remove CalculateContentsScale from LayerImpl and just move that code into LayerTreeHostCommon? On 2014/05/27 14:41:24, sohanjg wrote: > On 2014/05/27 07:44:56, danakj (OOO_back_june_6) wrote: > > > https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... > > File cc/layers/picture_layer_impl_unittest.cc (right): > > > > > https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... > > cc/layers/picture_layer_impl_unittest.cc:2043: > > pending_layer_->UpdateTilePriorities(); > > We UpdateDrawProperties() just below. Does that not do this stuff already? How > > does this differ? > > Yes. This soln is not proper, actually what we are missing here is in old > behavior, we used to set content_bounds, and content_scale_x/y which we are not > doing here. > So in this test, the visble content rect is not the same (which depends on the > content_bounds and transform), which affects the visible layer rect and finally > the Tiling priority. UpdateDrawProperties should set content bounds via CalculateContentsScale. Why would it not here? The content rect will be different because the content scale has changed, but the visible layer rect should be the same modulo floating point differences. Why would that change? https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_image... cc/layers/picture_image_layer_impl_unittest.cc:81: layer->draw_properties().ideal_contents_scale = 1.f; Why is this 1? Shouldn't it be ideal_contents_scale? https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:155: PopulateSharedQuadStateWithOverrides(shared_quad_state, I don't think this function adds much. Can you just make the call to SetAll directly here? https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:389: if (CanHaveTilings()) style nit: add {} for multiline bodies https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:2031: pending_layer_->SetBounds(layer_bounds); This shouldn't be necessary. Why would the layer not have these bounds?
On 2014/05/27 21:41:20, enne wrote: > Can you remove CalculateContentsScale from LayerImpl and just move that code > into LayerTreeHostCommon? > > On 2014/05/27 14:41:24, sohanjg wrote: > > On 2014/05/27 07:44:56, danakj (OOO_back_june_6) wrote: > > > > > > https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... > > > File cc/layers/picture_layer_impl_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/271533011/diff/420001/cc/layers/picture_layer... > > > cc/layers/picture_layer_impl_unittest.cc:2043: > > > pending_layer_->UpdateTilePriorities(); > > > We UpdateDrawProperties() just below. Does that not do this stuff already? > How > > > does this differ? > > > > Yes. This soln is not proper, actually what we are missing here is in old > > behavior, we used to set content_bounds, and content_scale_x/y which we are > not > > doing here. > > So in this test, the visble content rect is not the same (which depends on the > > content_bounds and transform), which affects the visible layer rect and > finally > > the Tiling priority. > > UpdateDrawProperties should set content bounds via CalculateContentsScale. Why > would it not here? The content rect will be different because the content scale > has changed, but the visible layer rect should be the same modulo floating point > differences. Why would that change? > The content bounds set by CalculateContentsScale here will not be same now as we dont set it as max tiling scale anymore. The content scale available to PictureLayerTilingSet and PictureLayerTiling, affects the Tiling priority calculation. Before this code, we used to apply content scale as max tiling scale which we are not doing now, this causes the *PictureLayerImplTest.LayerEvictionTileIterator* test to fail. In order to resolve it, i have added a scale recalculation in PictureLayerTilingSet::UpdateTilePriorities, before applying to PictureLayerTiling. PTAL. enne@, will it be OK if i do the removal of CalculateContentsScale from LayerImpl as a follow-up, i need to take care of around 100 tests again for that.
I'm fine with skipping removing CalculateContentsScales and doing it in a followup. https://codereview.chromium.org/271533011/diff/430001/cc/resources/picture_la... File cc/resources/picture_layer_tiling_set.cc (right): https://codereview.chromium.org/271533011/diff/430001/cc/resources/picture_la... cc/resources/picture_layer_tiling_set.cc:327: float max_layer_contents_scale = layer_contents_scale; You should not need this. The scale used to be max but is now 1, but this math should all still work out using that scale. What is failing, specifically?
The cc_unittests were failing for debug builds, which i didnt encounter while on Android, so had to re-write some of the tests again, and expose some PictureLayerImpl functions. Please take a look. Thanks for your patience on this. https://codereview.chromium.org/271533011/diff/430001/cc/resources/picture_la... File cc/resources/picture_layer_tiling_set.cc (right): https://codereview.chromium.org/271533011/diff/430001/cc/resources/picture_la... cc/resources/picture_layer_tiling_set.cc:327: float max_layer_contents_scale = layer_contents_scale; On 2014/05/28 18:42:20, enne wrote: > You should not need this. The scale used to be max but is now 1, but this math > should all still work out using that scale. > > What is failing, specifically? In LayerEvictionTileIterator test, tile->required_for_activation() check fails for some tiles. The problem is the PictureLayerImplTest.LayerEvictionTileIterator iterates over the tiles in a wrong order, the order is again decided the way we pushed the tiles in the iterator(based on priority), which seems to be different now. The only reason i found is that previously in PictureLayerTiling::UpdateTilePriorities the scaling was different than now and based on that TilePriorities may have been assigned. Please suggest if there can be any alternate way.
On 2014/05/29 13:07:03, sohanjg wrote: > The cc_unittests were failing for debug builds, which i didnt encounter while on > Android, so had to re-write some of the tests again, and expose some > PictureLayerImpl functions. > > Please take a look. > Thanks for your patience on this. > > https://codereview.chromium.org/271533011/diff/430001/cc/resources/picture_la... > File cc/resources/picture_layer_tiling_set.cc (right): > > https://codereview.chromium.org/271533011/diff/430001/cc/resources/picture_la... > cc/resources/picture_layer_tiling_set.cc:327: float max_layer_contents_scale = > layer_contents_scale; > On 2014/05/28 18:42:20, enne wrote: > > You should not need this. The scale used to be max but is now 1, but this > math > > should all still work out using that scale. > > > > What is failing, specifically? > > In LayerEvictionTileIterator test, tile->required_for_activation() check fails > for some tiles. > The problem is the PictureLayerImplTest.LayerEvictionTileIterator iterates over > the tiles in a wrong order, the order is again decided the way we pushed the > tiles in the iterator(based on priority), which seems to be different now. > The only reason i found is that previously in > PictureLayerTiling::UpdateTilePriorities the scaling was different than now and > based on that TilePriorities may have been assigned. > > Please suggest if there can be any alternate way. Please investigate why the priorities are different. The scaled content rect in the space of the tiling and the layer rect should be the same even if the layer's scale is different. https://codereview.chromium.org/271533011/diff/450001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/450001/cc/layers/picture_image... cc/layers/picture_image_layer_impl.cc:56: DoPostCommitInitializationIfNeeded(); Why is this needed? If you need this for a test, put it in the test? https://codereview.chromium.org/271533011/diff/450001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/450001/cc/layers/picture_image... cc/layers/picture_image_layer_impl_unittest.cc:88: layer->UpdateIdealScales(); Don't make these functions public in PictureLayerImpl? Expose them as public in TestablePictureImageLayerImpl and maybe add this block there as a single function?
Can you please take a look. Think we found the difference for PictureLayerImplTest.LayerEvictionTileIterator In PictureLayerTiling::UpdateTilePriorities, the border tile priority are differing. The calculations are, current_soon_border_rect_ = visible_rect_in_content_space; float border = kSoonBorderDistanceInScreenPixels / content_to_screen_scale; current_soon_border_rect_.Inset(-border, -border, -border, -border); Since, content_to_screen_scale will be different before/after, so the current_soon_border_rect_ and the priority assignment is different for the border tiles. Will try to change border calcualtions and see how it goes. Thanks. https://codereview.chromium.org/271533011/diff/450001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/450001/cc/layers/picture_image... cc/layers/picture_image_layer_impl.cc:56: DoPostCommitInitializationIfNeeded(); On 2014/05/29 17:55:48, enne wrote: > Why is this needed? If you need this for a test, put it in the test? Done. https://codereview.chromium.org/271533011/diff/450001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/450001/cc/layers/picture_image... cc/layers/picture_image_layer_impl_unittest.cc:88: layer->UpdateIdealScales(); On 2014/05/29 17:55:48, enne wrote: > Don't make these functions public in PictureLayerImpl? Expose them as public in > TestablePictureImageLayerImpl and maybe add this block there as a single > function? Done.
On 2014/05/30 16:52:42, sohanjg wrote: > Can you please take a look. > > Think we found the difference for PictureLayerImplTest.LayerEvictionTileIterator > > In PictureLayerTiling::UpdateTilePriorities, the border tile priority are > differing. The calculations are, > > current_soon_border_rect_ = visible_rect_in_content_space; > float border = kSoonBorderDistanceInScreenPixels / content_to_screen_scale; > current_soon_border_rect_.Inset(-border, -border, -border, -border); > > Since, content_to_screen_scale will be different before/after, so the > current_soon_border_rect_ and the priority assignment is different for the > border tiles. content_to_screen_scale should not be different. This is the scale to get from the PictureLayerTiling to the screen and shouldn't have anything to do with the content scale of the layer. https://codereview.chromium.org/271533011/diff/490001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/490001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:460: contents_scale_x(), Here's the problem. This is contents_scale_x(), but it really should be ideal contents scale because it's representing the scale that this layer should be on screen. In PictureLayerTiling, it uses this scale to try to get to screen space. I think that if you adjust this, then some of the test problems should go away.
https://codereview.chromium.org/271533011/diff/490001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/490001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:460: contents_scale_x(), On 2014/05/30 19:38:43, enne wrote: > Here's the problem. This is contents_scale_x(), but it really should be ideal > contents scale because it's representing the scale that this layer should be on > screen. In PictureLayerTiling, it uses this scale to try to get to screen > space. By "ideal" here, do you mean the MaxTilingContentScale()? Using the actual ideal would be a change of behaviour. > I think that if you adjust this, then some of the test problems should go away.
Can you please take a look. Thanks again for your patience. https://codereview.chromium.org/271533011/diff/490001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/490001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:460: contents_scale_x(), On 2014/05/31 23:21:14, danakj (OOO_back_june_6) wrote: > On 2014/05/30 19:38:43, enne wrote: > > Here's the problem. This is contents_scale_x(), but it really should be ideal > > contents scale because it's representing the scale that this layer should be > on > > screen. In PictureLayerTiling, it uses this scale to try to get to screen > > space. Using > > By "ideal" here, do you mean the MaxTilingContentScale()? Using the actual ideal > would be a change of behaviour. > > > I think that if you adjust this, then some of the test problems should go > away. > I think using MaxTilingContentScale() here will change the visible_layer_rect, which wont be correct, using ideal_content_scale doesnt make any difference to test, we need to anyways adjust content_to_screen_scale in PictureLayerTiling with MaxTilingContentScale to have same behavior as before. But, conceptually using ideal_content_scale here seems more appropriate, so i have updated it.
On Mon, Jun 2, 2014 at 9:05 AM, <sohan.jyoti@samsung.com> wrote: > Can you please take a look. > Thanks again for your patience. > > > > https://codereview.chromium.org/271533011/diff/490001/cc/ > layers/picture_layer_impl.cc > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/271533011/diff/490001/cc/ > layers/picture_layer_impl.cc#newcode460 > cc/layers/picture_layer_impl.cc:460: contents_scale_x(), > On 2014/05/31 23:21:14, danakj (OOO_back_june_6) wrote: > >> On 2014/05/30 19:38:43, enne wrote: >> > Here's the problem. This is contents_scale_x(), but it really >> > should be ideal > >> > contents scale because it's representing the scale that this layer >> > should be > >> on >> > screen. In PictureLayerTiling, it uses this scale to try to get to >> > screen > >> > space. >> > > Using > > > By "ideal" here, do you mean the MaxTilingContentScale()? Using the >> > actual ideal > >> would be a change of behaviour. >> > > > I think that if you adjust this, then some of the test problems >> > should go > >> away. >> > > > I think using MaxTilingContentScale() here will change the > visible_layer_rect, which wont be correct, using ideal_content_scale > doesnt make any difference to test, we need to anyways adjust > content_to_screen_scale in PictureLayerTiling with MaxTilingContentScale > to have same behavior as before. > But, conceptually using ideal_content_scale here seems more appropriate, > so i have updated it. > The visible content rect will now be in layer space, so the scale here should be removed: https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu... and the input variable can be renamed to that instead. > > https://codereview.chromium.org/271533011/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/06/02 08:41:43, danakj (OOO_back_june_6) wrote: > On Mon, Jun 2, 2014 at 9:05 AM, <mailto:sohan.jyoti@samsung.com> wrote: > > > Can you please take a look. > > Thanks again for your patience. > > > > > > > > https://codereview.chromium.org/271533011/diff/490001/cc/ > > layers/picture_layer_impl.cc > > File cc/layers/picture_layer_impl.cc (right): > > > > https://codereview.chromium.org/271533011/diff/490001/cc/ > > layers/picture_layer_impl.cc#newcode460 > > cc/layers/picture_layer_impl.cc:460: contents_scale_x(), > > On 2014/05/31 23:21:14, danakj (OOO_back_june_6) wrote: > > > >> On 2014/05/30 19:38:43, enne wrote: > >> > Here's the problem. This is contents_scale_x(), but it really > >> > > should be ideal > > > >> > contents scale because it's representing the scale that this layer > >> > > should be > > > >> on > >> > screen. In PictureLayerTiling, it uses this scale to try to get to > >> > > screen > > > >> > space. > >> > > > > Using > > > > > > By "ideal" here, do you mean the MaxTilingContentScale()? Using the > >> > > actual ideal > > > >> would be a change of behaviour. > >> > > > > > I think that if you adjust this, then some of the test problems > >> > > should go > > > >> away. > >> > > > > > > I think using MaxTilingContentScale() here will change the > > visible_layer_rect, which wont be correct, using ideal_content_scale > > doesnt make any difference to test, we need to anyways adjust > > content_to_screen_scale in PictureLayerTiling with MaxTilingContentScale > > to have same behavior as before. > > But, conceptually using ideal_content_scale here seems more appropriate, > > so i have updated it. > > > > The visible content rect will now be in layer space, so the scale here > should be removed: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu... > and the input variable can be renamed to that instead. > > > > > > https://codereview.chromium.org/271533011/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I have updated the code accordingly and rebased. PTAL. Thanks.
Thanks this looks super great. Just one question about the tests really, which you may have already explained or gone over with enne@ (sorry if that's so). https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_image... cc/layers/picture_image_layer_impl_unittest.cc:32: void ScaleAndManageTilings(bool animating_transform_to_screen, Is there a reason the test can't call UpdateTilePrios() directly instead of duplicating a piece of that method here? https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:158: layer->ScaleAndManageTilings(animating_transform_to_screen, Similar question, wondering why this can't call UpdateTilePrios here.
Thanks. https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_image... File cc/layers/picture_image_layer_impl_unittest.cc (right): https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_image... cc/layers/picture_image_layer_impl_unittest.cc:32: void ScaleAndManageTilings(bool animating_transform_to_screen, On 2014/06/02 11:22:11, danakj (OOO_back_june_6) wrote: > Is there a reason the test can't call UpdateTilePrios() directly instead of > duplicating a piece of that method here? Using UpdateTileprios directly (as we did in initial revs) fails the dcheck in ::MarkVisibleResourcesAsRequired for debug builds, (!layer_tree_impl()->needs_update_draw_properties()) So, i had to duplicate some of the code here.
https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:186: float contents_scale = max_contents_scale; Can you remove this temporary to improve readability? https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:462: MaximumTilingContentScale(), I think this is still wrong. visible_rect_in_content_space is in content space with a scale of 1. MaximumTilingContentsScale could be any scale at all depending on the set of tilings. You can't use these two values together to get visible_rect_in_layer_space. Yes, this was wrong before as well, but it would only be wrong when the maximum != ideal, which often only happened when pinching in. Now that scale = 1, it will be wrong in a lot more cases, such as any case when ideal != 1. You can't use MaximumTilingContentScale to convert from a tiling's content space to layer space inside of PictureLayerTiling, because that's the wrong scale. I think you need to pass 1 here (aka contents_scale) as the content scale for the layer. https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:176: float MaximumTilingContentScale() const; ContentScale => ContentsScale for consistency?
On 2014/06/03 19:14:46, enne wrote: > https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... > cc/layers/picture_layer_impl.cc:186: float contents_scale = max_contents_scale; > Can you remove this temporary to improve readability? > You mean use max_contents_scale directly in quad->SetNew ? > https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... > cc/layers/picture_layer_impl.cc:462: MaximumTilingContentScale(), > I think this is still wrong. visible_rect_in_content_space is in content space > with a scale of 1. MaximumTilingContentsScale could be any scale at all > depending on the set of tilings. You can't use these two values together to get > visible_rect_in_layer_space. > > Yes, this was wrong before as well, but it would only be wrong when the maximum > != ideal, which often only happened when pinching in. Now that scale = 1, it > will be wrong in a lot more cases, such as any case when ideal != 1. > > You can't use MaximumTilingContentScale to convert from a tiling's content space > to layer space inside of PictureLayerTiling, because that's the wrong scale. I > think you need to pass 1 here (aka contents_scale) as the content scale for the > layer. If we cant use MaximumTilingContentsScale to get visible_rect_in_layer_space , how about updating content_to_screen_scale in PictureLayerTiling with MaximumTilingContentsScale (https://codereview.chromium.org/271533011/#ps530001), that would solve our unittest plight. > > https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... > File cc/layers/picture_layer_impl.h (right): > > https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... > cc/layers/picture_layer_impl.h:176: float MaximumTilingContentScale() const; > ContentScale => ContentsScale for consistency?
Please take a look. Thanks. https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:186: float contents_scale = max_contents_scale; On 2014/06/03 19:14:47, enne wrote: > Can you remove this temporary to improve readability? Done. https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/271533011/diff/550001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:176: float MaximumTilingContentScale() const; On 2014/06/03 19:14:47, enne wrote: > ContentScale => ContentsScale for consistency? Done.
Maybe the unit tests are wrong. There are two problems here. Here is what I
know is right:
(1) visible layer rect
Here is the relationship among layer content scale / content bounds / content
rects:
(layer) content bounds/rect = ceil(layer bounds/rect * (layer) contents
scale)
In
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu...,
it changes the visible content rect to visible layer rect by dividing by the
layer contents scale. This scale must be the same as layer->contents_scale().
This is what the previous code did. If you pass MaximumContentsScale() rather
than contents_scale() to UpdateTilePriorities, this rect will be wrong. To make
this case correct and more simple, I would suggest passing visible_layer_rect
from PictureLayerImpl rather than doing the conversion in PictureLayerTilingSet.
(2) content_to_screen_scale
"screen scale" is represented by ideal contents scale. Previously, this was
*usually* contents_scale. Yes, contents_scale in the past was maximum and not
ideal, and this is a bug. If you want to just pass MaximumContentsScale here
and leave a TODO, that's fine.
Summary:
(1) Convert visible_content_rect to visible_layer_rect inside of
PictureLayerImpl and pass it to UpdateTilePriorities. Remove the conversion
inside of PictureLayerTilingSet.
(2) Pass MaximumContentsScale as "layer_contents_scale" in UpdateTilePriorities
and leave a TODO in PictureLayerImpl that this is wrong and should be ideal.
Doing #1 makes it possible to do this and not screw up the visible_layer_rect.
On 2014/06/04 17:31:50, enne wrote: > Maybe the unit tests are wrong. There are two problems here. Here is what I > know is right: > > (1) visible layer rect > > Here is the relationship among layer content scale / content bounds / content > rects: > > (layer) content bounds/rect = ceil(layer bounds/rect * (layer) contents > scale) > > In > https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu..., > it changes the visible content rect to visible layer rect by dividing by the > layer contents scale. This scale must be the same as layer->contents_scale(). > This is what the previous code did. If you pass MaximumContentsScale() rather > than contents_scale() to UpdateTilePriorities, this rect will be wrong. To make > this case correct and more simple, I would suggest passing visible_layer_rect > from PictureLayerImpl rather than doing the conversion in PictureLayerTilingSet. > > (2) content_to_screen_scale > > "screen scale" is represented by ideal contents scale. Previously, this was > *usually* contents_scale. Yes, contents_scale in the past was maximum and not > ideal, and this is a bug. If you want to just pass MaximumContentsScale here > and leave a TODO, that's fine. > > > Summary: > (1) Convert visible_content_rect to visible_layer_rect inside of > PictureLayerImpl and pass it to UpdateTilePriorities. Remove the conversion > inside of PictureLayerTilingSet. > (2) Pass MaximumContentsScale as "layer_contents_scale" in UpdateTilePriorities > and leave a TODO in PictureLayerImpl that this is wrong and should be ideal. > Doing #1 makes it possible to do this and not screw up the visible_layer_rect. Thanks a lot for the right suggestion. PTAL.
lgtm. Thanks for all the changes. :)
LGTM thanks for your hard work on this
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/271533011/590001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by sohan.jyoti@samsung.com
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/271533011/610001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/271533011/610001
The CQ bit was unchecked by danakj@chromium.org
Looks like this DCHECK is being hit on some tests: [9:17:0610/132452:FATAL:picture_layer_impl.cc(970)] Check failed: ideal_page_scale_. We should understand why and write a cc_unittest that would trigger this DCHECK also, and fix it.
On 2014/06/10 21:18:40, danakj wrote: > Looks like this DCHECK is being hit on some tests: > > [9:17:0610/132452:FATAL:picture_layer_impl.cc(970)] Check failed: > ideal_page_scale_. > > We should understand why and write a cc_unittest that would trigger this DCHECK > also, and fix it. The problem was, we missed intializing the scales in DrawProperties Ctor. The chromeos test case, must have invoked updatetilepriorities w/o executecalcdrawproperties, hence scales were not set. I have updated the LTHCommon unit test, to check scales when we havent called executecalcdrawproperties. Please take a look. Thanks.
lgtm
Hm, that sounds really strange. LayerTreeHostImpl::UpdateDrawProperties() does both CalcDrawProps and UpdateTilePrios. Do you have a stacktrace for how we got to UpdateTilePrios without CalcDrawProps first? https://codereview.chromium.org/271533011/diff/670001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/271533011/diff/670001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:8352: EXPECT_FLOAT_EQ(1.f, root_layer->draw_properties().ideal_contents_scale); There's no guarantee that these are value before executing calc draw props, so I don't think we should be testing this.
Hm, I got this stacktrace: [9:17:0611/152435:FATAL:picture_layer_impl.cc(974)] Check failed: ideal_page_scale_. #0 0x7f530296536e base::debug::StackTrace::StackTrace() #1 0x7f53029f8542 logging::LogMessage::~LogMessage() #2 0x7f5302ee73ae cc::PictureLayerImpl::ManageTilings() #3 0x7f5302ee6c68 cc::PictureLayerImpl::UpdateTilePriorities() #4 0x7f530314c559 cc::LayerTreeImpl::UpdateDrawProperties() #5 0x7f5303126831 cc::LayerTreeHostImpl::CommitComplete() #6 0x7f530317410b cc::ThreadProxy::ScheduledActionCommit() #7 0x7f530317418c cc::ThreadProxy::ScheduledActionCommit() #8 0x7f53030c8793 cc::Scheduler::ProcessScheduledActions() #9 0x7f53030c8f44 cc::Scheduler::NotifyReadyToCommit() #10 0x7f53031763c2 cc::ThreadProxy::ReadyToFinalizeTextureUpdates()
On 2014/06/11 19:25:33, danakj wrote: > Hm, I got this stacktrace: > > [9:17:0611/152435:FATAL:picture_layer_impl.cc(974)] Check failed: > ideal_page_scale_. > #0 0x7f530296536e base::debug::StackTrace::StackTrace() > #1 0x7f53029f8542 logging::LogMessage::~LogMessage() > #2 0x7f5302ee73ae cc::PictureLayerImpl::ManageTilings() > #3 0x7f5302ee6c68 cc::PictureLayerImpl::UpdateTilePriorities() > #4 0x7f530314c559 cc::LayerTreeImpl::UpdateDrawProperties() > #5 0x7f5303126831 cc::LayerTreeHostImpl::CommitComplete() > #6 0x7f530317410b cc::ThreadProxy::ScheduledActionCommit() > #7 0x7f530317418c cc::ThreadProxy::ScheduledActionCommit() > #8 0x7f53030c8793 cc::Scheduler::ProcessScheduledActions() > #9 0x7f53030c8f44 cc::Scheduler::NotifyReadyToCommit() > #10 0x7f53031763c2 cc::ThreadProxy::ReadyToFinalizeTextureUpdates() I appears that the reason behind this is we're not setting these draw properties on the mask layers in CalcDrawProperties, we need to fix that.
https://codereview.chromium.org/271533011/diff/670001/cc/layers/draw_properti... File cc/layers/draw_properties.h (right): https://codereview.chromium.org/271533011/diff/670001/cc/layers/draw_properti... cc/layers/draw_properties.h:39: ideal_contents_scale(1.f), can you init these all to 0, so we don't hide this problem in the future should we do this again. https://codereview.chromium.org/271533011/diff/670001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/271533011/diff/670001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:1743: layer_draw_properties.ideal_contents_scale = ideal_contents_scale; Notice that CalculateContentsScale() in this file does its work on the layer, it's mask and it's replica's mask, if they exist. We need to set these 4 properties on those 3 layers instead of just on |layer| here. Splitting this off into a helper method that we call for each of the 3 layers would be nice.
Thanks for pointing it out danakj@, i couldnt get the back trace for the dcheck, and hence assumed what could be the possible situation for the failure. I have updated the code and test, please take a look. Thanks. https://codereview.chromium.org/271533011/diff/670001/cc/layers/draw_properti... File cc/layers/draw_properties.h (right): https://codereview.chromium.org/271533011/diff/670001/cc/layers/draw_properti... cc/layers/draw_properties.h:39: ideal_contents_scale(1.f), On 2014/06/11 19:48:35, danakj wrote: > can you init these all to 0, so we don't hide this problem in the future should > we do this again. Done. https://codereview.chromium.org/271533011/diff/670001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/271533011/diff/670001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:1743: layer_draw_properties.ideal_contents_scale = ideal_contents_scale; On 2014/06/11 19:48:35, danakj wrote: > Notice that CalculateContentsScale() in this file does its work on the layer, > it's mask and it's replica's mask, if they exist. > > We need to set these 4 properties on those 3 layers instead of just on |layer| > here. Splitting this off into a helper method that we call for each of the 3 > layers would be nice. Done.
Thanks! LGTM w/ 2 last things https://codereview.chromium.org/271533011/diff/690001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/271533011/diff/690001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:912: static inline void UpdateLayerDrawProperties( rename to UpdateLayerScaleDrawProperties, this name too general right now https://codereview.chromium.org/271533011/diff/690001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/271533011/diff/690001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:8443: EXPECT_FLOAT_EQ(15.f, child2_layer->draw_properties().ideal_contents_scale); You could also test the mask layer down here throughout the rest of the test?
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/271533011/710001
The CQ bit was unchecked by sohan.jyoti@samsung.com
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/271533011/730001
Message was sent while issue was closed.
Change committed as 277000
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/338653002/ by tonyg@chromium.org. The reason for reverting is: BUG=384730.
Message was sent while issue was closed.
Description was changed from ========== cc: Move tiling management out of draw properties calculation. This is mainly plumbing code. As part of the changes, we are moving ManageTiling as part of UpdateTilePriorities. We keep picturelayers content scale as 1,and maintain the remaining required scales in draw properties. This scales we apply later, to shared quad state and draw transform during AppendQuads. BUG=None Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277000 ========== to ========== cc: Move tiling management out of draw properties calculation. This is mainly plumbing code. As part of the changes, we are moving ManageTiling as part of UpdateTilePriorities. We keep picturelayers content scale as 1,and maintain the remaining required scales in draw properties. This scales we apply later, to shared quad state and draw transform during AppendQuads. BUG=None Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277000 ========== |
