Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
375693006:
Snap layers in views to physical pixel (Closed)
|
Created:
6 years, 5 months ago by oshima Modified:
6 years, 5 months ago CC:
chromium-reviews, tdanderson+views_chromium.org, Ian Vollick, tfarina, sievers+watch_chromium.org, jbauman+watch_chromium.org, ben+views_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionSnap layers in views to physical pixel
minor changes
- print offset/mask info when printing layer hierarchy.
- copy the offset when cloning layer.
BUG=391822
TEST=Added new tests in layer_unittests and views_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283672
Patch Set 1 : #
Total comments: 17
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : cmath #Patch Set 6 : use cc::MathUtil::Round instead #
Total comments: 1
Messages
Total messages: 32 (0 generated)
danakj -> ui/compositor ben -> ui/views
danakj -> ui/compositor ben -> ui/views
Snapping for arbitrary content (as opposed to only leaf nodes) has to be aware of the embedding context as well or we'll accumulate large error and gaps. If we want to support snapping across the tree, we probably want to compute the position on the physical pixel grid for everything in the tree and adjust things holistically (i.e. we might want to move a parent layer towards (0, 0) then move a child layer towards (+inf, +inf) to be closer to its 'ideal' location). https://codereview.chromium.org/375693006/diff/20001/ui/compositor/dip_util.cc File ui/compositor/dip_util.cc (right): https://codereview.chromium.org/375693006/diff/20001/ui/compositor/dip_util.c... ui/compositor/dip_util.cc:87: std::round(view_offset.y())); this moves the layer towards (0, 0). If this is used by nested layers, won't each one move up a bit causing large accumulated error? This 'works' for RWHVAura since there can't be multiple layers of embedding there, but there can be on an arbitrary layer.
The way blink deals with issues like this is to lay everything out on an idealized grid (it happens to be subpixel but that's not really important) and then delay snapping to physical pixels to as late as possible in the rendering process so that everything can end up as close as it can be to the ideal grid but still be snapped. This way the embedding level doesn't impact the rounding.
On 2014/07/08 01:00:59, jamesr wrote: > Snapping for arbitrary content (as opposed to only leaf nodes) has to be aware > of the embedding context as well or we'll accumulate large error and gaps. If > we want to support snapping across the tree, we probably want to compute the > position on the physical pixel grid for everything in the tree and adjust things > holistically (i.e. we might want to move a parent layer towards (0, 0) then move > a child layer towards (+inf, +inf) to be closer to its 'ideal' location). This is working fine for 1.25 and 1.5 DSF on chromeos which has more layers. > > https://codereview.chromium.org/375693006/diff/20001/ui/compositor/dip_util.cc > File ui/compositor/dip_util.cc (right): > > https://codereview.chromium.org/375693006/diff/20001/ui/compositor/dip_util.c... > ui/compositor/dip_util.cc:87: std::round(view_offset.y())); > this moves the layer towards (0, 0). If this is used by nested layers, won't > each one move up a bit causing large accumulated error? > > This 'works' for RWHVAura since there can't be multiple layers of embedding > there, but there can be on an arbitrary layer. I think error accumulation is less of an issue because what's important is its position to the parent. (as long as they're aligned)
On 2014/07/08 01:15:11, jamesr wrote: > The way blink deals with issues like this is to lay everything out on an > idealized grid (it happens to be subpixel but that's not really important) and > then delay snapping to physical pixels to as late as possible in the rendering > process so that everything can end up as close as it can be to the ideal grid > but still be snapped. This way the embedding level doesn't impact the rounding. I understand that'd be necessary for Blink whose layers can be deeper and can have arbitrary zooming, where layer's scale and positions are unknown (as application can do whatever they want). Ash, on the other hand, is controlled environment. Only limited views have layers, they're often positioned at 0,0, so this sounds overkill for current ash especially UI looks fine with scale factors that we support (1.25, 1.5). We can revisit this when this becomes necessary of course.
https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc#newcode577 ui/views/view.cc:577: void View::SnapLayerToPixelBoundary() { should this be implemented in Layer?
https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc#newcode577 ui/views/view.cc:577: void View::SnapLayerToPixelBoundary() { On 2014/07/08 16:07:34, Ben Goodger (Google) wrote: > should this be implemented in Layer? Layer currently does not know which layers are aligned (this CL assumes that widgets are aligned). I can add a flag to layer (to enable or disable depending on the default), that's one option. Or I can define Layer::SnapToPixelBoundary(ui::Layer* aligned_layer) and let caller decide which layers are aligned. ben@, danakj@, what do you think? Let me know if you have a better idea, and I'm happy to change.
On 2014/07/08 17:39:19, oshima wrote: > https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc > File ui/views/view.cc (right): > > https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc#newcode577 > ui/views/view.cc:577: void View::SnapLayerToPixelBoundary() { > On 2014/07/08 16:07:34, Ben Goodger (Google) wrote: > > should this be implemented in Layer? > > Layer currently does not know which layers are aligned (this CL assumes that > widgets are aligned). I can add a flag to layer (to enable or disable depending > on the default), that's one option. Or I can define > Layer::SnapToPixelBoundary(ui::Layer* aligned_layer) and let caller decide which > layers are aligned. > > ben@, danakj@, what do you think? Let me know if you have a better idea, and I'm > happy to change. Oshima filled me in on the details here. I think it's OK to add this to View for now. If we end up having to replicate this logic in other places we can consider moving it to a more central location then.
On 2014/07/11 18:22:30, Ben Goodger (Google) wrote: > On 2014/07/08 17:39:19, oshima wrote: > > https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc > > File ui/views/view.cc (right): > > > > > https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc#newcode577 > > ui/views/view.cc:577: void View::SnapLayerToPixelBoundary() { > > On 2014/07/08 16:07:34, Ben Goodger (Google) wrote: > > > should this be implemented in Layer? > > > > Layer currently does not know which layers are aligned (this CL assumes that > > widgets are aligned). I can add a flag to layer (to enable or disable > depending > > on the default), that's one option. Or I can define > > Layer::SnapToPixelBoundary(ui::Layer* aligned_layer) and let caller decide > which > > layers are aligned. > > > > ben@, danakj@, what do you think? Let me know if you have a better idea, and > I'm > > happy to change. > > Oshima filled me in on the details here. I think it's OK to add this to View for > now. If we end up having to replicate this logic in other places we can consider > moving it to a more central location then. (lgtm)
danakj: ping?
https://codereview.chromium.org/375693006/diff/20001/ui/compositor/dip_util.cc File ui/compositor/dip_util.cc (right): https://codereview.chromium.org/375693006/diff/20001/ui/compositor/dip_util.c... ui/compositor/dip_util.cc:76: if (!snapped_layer || !snapped_layer->Contains(layer_to_snap)) Could these be DCHECKs instead? Also can we DCHECK that the snapped layer is in fact snapped? https://codereview.chromium.org/375693006/diff/20001/ui/compositor/layer_unit... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/375693006/diff/20001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:1560: std::string ToString(const gfx::Vector2dF& vector) { Vector2dFToHundredthPrecisionString or something? https://codereview.chromium.org/375693006/diff/20001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:1585: // c11 msut already be alignedt at 1.5 scale. "must" "aligned" https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc#newcode582 ui/views/view.cc:582: layer()->parent() && layer()->GetCompositor()) { why the check for GetCompositor? https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc#newcode... ui/views/view.cc:1560: snap_layer_to_pixel_boundary_ = Didn't you want this to be false for some layers like shadows? https://codereview.chromium.org/375693006/diff/20001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/375693006/diff/20001/ui/views/view_unittest.c... ui/views/view_unittest.cc:3938: // DSF change shoud get propagated and update offsets. should https://codereview.chromium.org/375693006/diff/20001/ui/views/view_unittest.c... ui/views/view_unittest.cc:3943: // Deleting parent' layer should update the child view's layer's offset. s/'//
https://codereview.chromium.org/375693006/diff/20001/ui/compositor/dip_util.cc File ui/compositor/dip_util.cc (right): https://codereview.chromium.org/375693006/diff/20001/ui/compositor/dip_util.c... ui/compositor/dip_util.cc:76: if (!snapped_layer || !snapped_layer->Contains(layer_to_snap)) On 2014/07/15 21:11:02, danakj wrote: > Could these be DCHECKs instead? > Also can we DCHECK that the snapped layer is in fact snapped? Done. https://codereview.chromium.org/375693006/diff/20001/ui/compositor/layer_unit... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/375693006/diff/20001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:1560: std::string ToString(const gfx::Vector2dF& vector) { On 2014/07/15 21:11:02, danakj wrote: > Vector2dFToHundredthPrecisionString or something? Done. https://codereview.chromium.org/375693006/diff/20001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:1585: // c11 msut already be alignedt at 1.5 scale. On 2014/07/15 21:11:02, danakj wrote: > "must" "aligned" Done. https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc#newcode582 ui/views/view.cc:582: layer()->parent() && layer()->GetCompositor()) { On 2014/07/15 21:11:02, danakj wrote: > why the check for GetCompositor? Because view can exist without parent nor widget. It gets recomputed when (re)attached in OnDeviceScaleFactorChanged. https://codereview.chromium.org/375693006/diff/20001/ui/views/view.cc#newcode... ui/views/view.cc:1560: snap_layer_to_pixel_boundary_ = On 2014/07/15 21:11:02, danakj wrote: > Didn't you want this to be false for some layers like shadows? Yes, it'll be handled in a separate CL for ash. (shadow is in ash, not views) https://codereview.chromium.org/375693006/diff/20001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/375693006/diff/20001/ui/views/view_unittest.c... ui/views/view_unittest.cc:3938: // DSF change shoud get propagated and update offsets. On 2014/07/15 21:11:02, danakj wrote: > should Done. https://codereview.chromium.org/375693006/diff/20001/ui/views/view_unittest.c... ui/views/view_unittest.cc:3943: // Deleting parent' layer should update the child view's layer's offset. On 2014/07/15 21:11:02, danakj wrote: > s/'// Done.
ok LGTM until we need something better
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/375693006/80001
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...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29134)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/375693006/100001
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/375693006/120001
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...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
On 2014/07/16 22:21:54, I haz the power (commit-bot) wrote: > 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...) > android_dbg on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) > linux_chromium_gn_rel on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) > mac_chromium_compile_dbg on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) > mac_chromium_rel on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) mac/android doesn't seem to like std::round. We actually have cc::MathUtil::Round, so I switched to it.
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/375693006/140001
Message was sent while issue was closed.
Change committed as 283672
Message was sent while issue was closed.
https://codereview.chromium.org/375693006/diff/140001/ui/compositor/dip_util.cc File ui/compositor/dip_util.cc (right): https://codereview.chromium.org/375693006/diff/140001/ui/compositor/dip_util.... ui/compositor/dip_util.cc:100: gfx::PointF view_offset_snapped(cc::MathUtil::Round(view_offset.x()), MathUtil is not meant to be used outside of cc. I think you were looking for https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/sa...
Message was sent while issue was closed.
On 2014/07/17 15:12:14, danakj wrote: > https://codereview.chromium.org/375693006/diff/140001/ui/compositor/dip_util.cc > File ui/compositor/dip_util.cc (right): > > https://codereview.chromium.org/375693006/diff/140001/ui/compositor/dip_util.... > ui/compositor/dip_util.cc:100: gfx::PointF > view_offset_snapped(cc::MathUtil::Round(view_offset.x()), > MathUtil is not meant to be used outside of cc. I think you were looking for > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/sa... Thanks. I uploaded a fix: https://codereview.chromium.org/391343009/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
