Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12407002:
Align physical pixels of scrolled layers (Closed)
|
Created:
7 years, 9 months ago by Xianzhu Modified:
7 years, 9 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAlign physical pixels of scrolled layers
Non-integral position of scrolled layers causes blurry content.
If a layer has simple transform and the translations in the transform are
merely caused by scroll offsets, round the translations to align the pixels.
BUG=174699
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186377
Patch Set 1 #
Total comments: 11
Patch Set 2 : Align scrollable layer #
Total comments: 9
Patch Set 3 : Separate LCDText out #
Total comments: 2
Patch Set 4 : naming #Patch Set 5 : Fix win break (MSVC doesn't have round()) #Patch Set 6 : Fix win warnings #Patch Set 7 : Fix win break again #
Messages
Total messages: 22 (0 generated)
Won't snapping make animations look less smooth? Should we only do this when quiescent?
The Android fling animator gives us individual scrolls in ints (physical pixels) to begin with, so that shouldn't be an issue.
I agree with jamesr, in case in the future Android will pass float scroll offsets. Will do the change. Just found a problem that the text is still blurry after a big zoom-in then double-tap to return to the auto-sized scale. Will investigate.
https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc File cc/layer_tree_host_common.cc (right): https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc#... cc/layer_tree_host_common.cc:734: roundNearIntegerScales(&layerDrawProperties.target_space_transform); (1) It might be a problem that this value is used to compute the subsequent screen-space transform, isn't it? Does that mean we round twice for the screen space transform? (2) What about the situation where a renderSurface re-parents the transform? in that case, the layer gets a different target_space transform which may also need to be correctly rounded? I'm still uneasy about adding rounding logic of this sort directly into calcDrawProperties. Knowing that the code will evolve, it might be less error prone to make this rounding an explicit step that happens immediately after all calcDrawProperties recursion. Would that work?
https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc File cc/layer_tree_host_common.cc (right): https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc#... cc/layer_tree_host_common.cc:551: if (std::abs(scaleX - roundedScaleX) < kErrorThreshold) I'm wondering if it'd be better to only do this for both x and y scales or neither. Otherwise we might adjust one axis but the result still won't be integer scaled on the orthogonal axis. https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc#... cc/layer_tree_host_common.cc:734: roundNearIntegerScales(&layerDrawProperties.target_space_transform); > I'm still uneasy about adding rounding logic of this sort directly into > calcDrawProperties. Knowing that the code will evolve, it might be less error > prone to make this rounding an explicit step that happens immediately after all > calcDrawProperties recursion. Would that work? One problem I can think of is that rounding can move layers around, so some of the things we calculate here such as the visible layer rectangle may become invalid. https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc#... cc/layer_tree_host_common.cc:746: if (scrollLayer) { Would it be simpler to do this for all scrollable layers instead of checking for a scrollable parent? We're already walking through the entire hierarchy so we've already processed the parent at this point; had we aligned the child layer transform for that parent there's no need to do anything to any of its children, right? Maybe another way would be to round layer->scrollDelta() on line 697 to the nearest screen space pixel? That way there wouldn't be a need to adjust the transforms after they have been calculated.
https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc File cc/layer_tree_host_common.cc (right): https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc#... cc/layer_tree_host_common.cc:551: if (std::abs(scaleX - roundedScaleX) < kErrorThreshold) On 2013/03/05 11:28:34, Sami wrote: > I'm wondering if it'd be better to only do this for both x and y scales or > neither. Otherwise we might adjust one axis but the result still won't be > integer scaled on the orthogonal axis. I'm not sure. Considering the case of (scalex, scaley) = (1.00001, 3.5), the current method they will be rounded to (1, 3.5). This seems still better if there is only '-webkit-transform: scaleY(3.5)' because the scaleX will be rounded back to the expected value 1. BTW, I guess this small inaccuracy of scale is the reason that you see the pattern in your checkerboard test case. https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc#... cc/layer_tree_host_common.cc:734: roundNearIntegerScales(&layerDrawProperties.target_space_transform); On 2013/03/05 05:29:45, shawnsingh wrote: > (1) It might be a problem that this value is used to compute the subsequent > screen-space transform, isn't it? Does that mean we round twice for the screen > space transform? > roundNearIntegerScales() sets back the expected values of scales that contains small calculation errors. Round it here prevents the errors from propagating. The change of scale is too small to have visual effects but will make later Transform::IsIdentityOrXXX() calls work. roundTranslates() moves the layer, but that only happens if the translates are merely caused by scrolling and affects only the actual scroll offset. > (2) What about the situation where a renderSurface re-parents the transform? in > that case, the layer gets a different target_space transform which may also need > to be correctly rounded? > Would calculateDrawProperties() be called in the case? > I'm still uneasy about adding rounding logic of this sort directly into > calcDrawProperties. Knowing that the code will evolve, it might be less error > prone to make this rounding an explicit step that happens immediately after all > calcDrawProperties recursion. Would that work? Will change to round position() to screen space pixel to avoid the complex calculation on transforms. https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc#... cc/layer_tree_host_common.cc:734: roundNearIntegerScales(&layerDrawProperties.target_space_transform); On 2013/03/05 11:28:34, Sami wrote: > > I'm still uneasy about adding rounding logic of this sort directly into > > calcDrawProperties. Knowing that the code will evolve, it might be less error > > prone to make this rounding an explicit step that happens immediately after > all > > calcDrawProperties recursion. Would that work? > > One problem I can think of is that rounding can move layers around, so some of > the things we calculate here such as the visible layer rectangle may become > invalid. Will try to round position to screen space pixel, so that combinedTransform will based on the adjusted value and make surface's drawTransform (used to calculate visible rects) and the layers target_space_transform and screen_space_transform are consistent. https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc#... cc/layer_tree_host_common.cc:746: if (scrollLayer) { On 2013/03/05 11:28:34, Sami wrote: > Would it be simpler to do this for all scrollable layers instead of checking for > a scrollable parent? We're already walking through the entire hierarchy so we've > already processed the parent at this point; had we aligned the child layer > transform for that parent there's no need to do anything to any of its children, > right? > If the scrollable layer is the content layer itself, its scroll position is handled here. If the scrollable layer is the parent, the scrollable layer's position should be zero (correct me if wrong) and the scrolling position can only be adjusted for the content layer here. > Maybe another way would be to round layer->scrollDelta() on line 697 to the > nearest screen space pixel? That way there wouldn't be a need to adjust the > transforms after they have been calculated. It won't work, because actually the other main cause of the subpixel position is the integeral scrollOffset() in CSS coordinates (equals to position() of the content layer). However, I think this is a good direction to go. I'll try to round position to screen space pixel to avoid adjusting the transforms.
PTAL https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc File cc/layer_tree_host_common.cc (right): https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc#... cc/layer_tree_host_common.cc:734: roundNearIntegerScales(&layerDrawProperties.target_space_transform); On 2013/03/05 17:26:08, Xianzhu wrote: > On 2013/03/05 05:29:45, shawnsingh wrote: > > (2) What about the situation where a renderSurface re-parents the transform? > in > > that case, the layer gets a different target_space transform which may also > need > > to be correctly rounded? > > > > Would calculateDrawProperties() be called in the case? > Sorry, I didn't get your point when posting the reply. In the newest CL the pixel alignment also affects renderSurface's drawTransform. https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc#... cc/layer_tree_host_common.cc:746: if (scrollLayer) { On 2013/03/05 17:26:08, Xianzhu wrote: > If the scrollable layer is the content layer itself, its scroll position is > handled here. If the scrollable layer is the parent, the scrollable layer's > position should be zero (correct me if wrong) and the scrolling position can > only be adjusted for the content layer here. > I was wrong about this. Actually position() is non-zero for the scrollable layer and is zero for the content layer. Now deal with the scrollable layer only. https://codereview.chromium.org/12407002/diff/1/cc/layer_tree_host_common.cc#... cc/layer_tree_host_common.cc:746: if (scrollLayer) { On 2013/03/05 17:26:08, Xianzhu wrote: > However, I think this is a good direction to go. I'll try to round position to > screen space pixel to avoid adjusting the transforms. Done. Slightly different: now rounds the translations of combinedTransform.
I like that this latest patch only applies to scrollable layers. It might make sense to also not apply this snapping if the layer is animatingTransformToTarget. https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.cc File cc/layer_tree_host_common.cc (right): https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.... cc/layer_tree_host_common.cc:543: // Small calculation errors may result almost-integer scales like 1.00001. Where is this calculation error introduced? https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.... cc/layer_tree_host_common.cc:721: if (layer->scrollable() && combinedTransform.IsScaleOrTranslation() && layer->transform().IsIdentity()) { Why is it so important that layer->transform().IsIdentity() here? I might be missing something, but it seems like the combinedTransform.IsScaleOrTranslation() check should be sufficient. https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.... cc/layer_tree_host_common.cc:740: roundAlmostIntegerScales(&layerDrawProperties.target_space_transform); As you've already rounded combinedTransform above, why is this necessary? https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.... cc/layer_tree_host_common.cc:747: roundAlmostIntegerScales(&layerDrawProperties.screen_space_transform); Is this rounding necessary? As Shawn points out, if there's an intermediate render surface (i.e. target_space_transform != screen_space_transform), then rounding may not make any sense.
https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.cc File cc/layer_tree_host_common.cc (right): https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.... cc/layer_tree_host_common.cc:543: // Small calculation errors may result almost-integer scales like 1.00001. On 2013/03/05 23:46:28, enne wrote: > Where is this calculation error introduced? I observed the near 1 scales in content transformations, but didn't trace the exact places where the error is introduced. I think it comes from accumulated errors of multiple multiplication and division operations over mixed float (individual scales) and double (in matrix) operands. https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.... cc/layer_tree_host_common.cc:721: if (layer->scrollable() && combinedTransform.IsScaleOrTranslation() && layer->transform().IsIdentity()) { On 2013/03/05 23:46:28, enne wrote: > Why is it so important that layer->transform().IsIdentity() here? I might be > missing something, but it seems like the > combinedTransform.IsScaleOrTranslation() check should be sufficient. I think for example if the layer itself has a non-integral translation, it'd be better not to round to avoid damage to the original transform. In addition I couldn't make sure how the rounding and the layer transform interferes and couldn't create tests to cover this in time. Would a TODO look good? Don't know if we have real examples of scrollable layer having a non-identity transform. Just for analogy a non-scrollable layer example is maps for which we must keep the exact original transforms to avoid gaps between the map tiles. https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.... cc/layer_tree_host_common.cc:740: roundAlmostIntegerScales(&layerDrawProperties.target_space_transform); On 2013/03/05 23:46:28, enne wrote: > As you've already rounded combinedTransform above, why is this necessary? This is to meet the condition of layerCanUseLCDText below. It may also affect sharpness of text. If the scale is almost 1 but not 1, LCD text won't be used. Will add some comments. https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.... cc/layer_tree_host_common.cc:747: roundAlmostIntegerScales(&layerDrawProperties.screen_space_transform); On 2013/03/05 23:46:28, enne wrote: > Is this rounding necessary? As Shawn points out, if there's an intermediate > render surface (i.e. target_space_transform != screen_space_transform), then > rounding may not make any sense. Will remove.
Ah, I understand now. It seems like there's two changes of behavior here. There's "round transforms so that LCD text can be applied" and there's "align physical pixels of scrolled layers so that text is sharp". Can you separate out the general rounding code into a separate patch so that if there are regressions from this change that we'll be able to tell them apart? I am also more worried about the risks in rounding scales for all draw transforms (not just translations on scrollable layers) and I think this patch would land quicker if you didn't try to do both. https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.cc File cc/layer_tree_host_common.cc (right): https://codereview.chromium.org/12407002/diff/8001/cc/layer_tree_host_common.... cc/layer_tree_host_common.cc:721: if (layer->scrollable() && combinedTransform.IsScaleOrTranslation() && layer->transform().IsIdentity()) { On 2013/03/06 00:26:13, Xianzhu wrote: > On 2013/03/05 23:46:28, enne wrote: > > Why is it so important that layer->transform().IsIdentity() here? I might be > > missing something, but it seems like the > > combinedTransform.IsScaleOrTranslation() check should be sufficient. > > I think for example if the layer itself has a non-integral translation, it'd be > better not to round to avoid damage to the original transform. > > In addition I couldn't make sure how the rounding and the layer transform > interferes and couldn't create tests to cover this in time. Would a TODO look > good? > > Don't know if we have real examples of scrollable layer having a non-identity > transform. Just for analogy a non-scrollable layer example is maps for which we > must keep the exact original transforms to avoid gaps between the map tiles. I still don't follow what the case you're trying to prevent here with the IsIdentity check, other than just trying to be conservative against the unknown. If you want to test this, just add a transform with a non-integer translation to the scrollable layer in your unit test, and I suspect it will continue to work as expected.
PTAL. Now the CL is very simple :)
https://codereview.chromium.org/12407002/diff/16001/cc/layer_tree_host_common.cc File cc/layer_tree_host_common.cc (right): https://codereview.chromium.org/12407002/diff/16001/cc/layer_tree_host_common... cc/layer_tree_host_common.cc:543: static void roundTranslates(gfx::Transform* transform) nit: roundTranslationComponents()
https://codereview.chromium.org/12407002/diff/16001/cc/layer_tree_host_common.cc File cc/layer_tree_host_common.cc (right): https://codereview.chromium.org/12407002/diff/16001/cc/layer_tree_host_common... cc/layer_tree_host_common.cc:543: static void roundTranslates(gfx::Transform* transform) On 2013/03/06 01:49:28, danakj wrote: > nit: roundTranslationComponents() Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/12407002/9004
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/12407002/28001
Failed to trigger a try job on win HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/12407002/35002
Sorry for I got bad news for ya. Compile failed with a clobber build on win7_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/12407002/46001
Message was sent while issue was closed.
Change committed as 186377 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
