Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(6311)

Unified Diff: cc/tiles/picture_layer_tiling.cc

Issue 2816943004: cc: Fix bugs found by fuzzer due to floating point imprecision. (Closed)
Patch Set: Created 3 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | cc/tiles/picture_layer_tiling_unittest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: cc/tiles/picture_layer_tiling.cc
diff --git a/cc/tiles/picture_layer_tiling.cc b/cc/tiles/picture_layer_tiling.cc
index e52a8ab6af1bf1c860756e9681c0513883d06488..fe7e4c9a7a743289c208b99e846a45203ab03d20 100644
--- a/cc/tiles/picture_layer_tiling.cc
+++ b/cc/tiles/picture_layer_tiling.cc
@@ -30,6 +30,12 @@
namespace cc {
+namespace {
+float SnapToOne(float n) {
+ return (std::abs(1.f - n) < std::numeric_limits<float>::epsilon()) ? 1.f : n;
+}
trchen 2017/04/14 22:07:33 I doubt this function works as intended... By def
vmpstr 2017/04/14 23:36:26 The definition of epsilon is the difference betwee
trchen 2017/04/14 23:53:43 You're right. I forgot (1-epsilon) contains only 2
vmpstr 2017/04/15 00:31:24 It just happens that when coverage scale is 7352.3
trchen 2017/04/18 21:13:46 Does this helps with the bug you're fixing? I seem
+}
+
PictureLayerTiling::PictureLayerTiling(
WhichTree tree,
const gfx::AxisTransform2d& raster_transform,
@@ -380,9 +386,15 @@ PictureLayerTiling::CoverageIterator::CoverageIterator(
const gfx::Rect& coverage_rect)
: tiling_(tiling),
coverage_rect_(coverage_rect),
+ // Note that since this iterator will map possibly large numbers, it's
+ // very sensitive to small differences in scale (even less than epsilon).
+ // It is also very likely that this coverage iterator would be used with a
+ // scale that matches the contents scale (since majority of the time,
+ // we're rasterizing high res content. As a result, snap the scale to one
+ // if it's within the epsilon.
coverage_to_content_(
- gfx::PreScaleAxisTransform2d(tiling->raster_transform(),
- 1.f / coverage_scale)) {
+ SnapToOne(tiling->raster_transform().scale() / coverage_scale),
+ tiling->raster_transform().translation()) {
DCHECK(tiling_);
// In order to avoid artifacts in geometry_rect scaling and clamping to ints,
// the |coverage_scale| should always be at least as big as the tiling's
@@ -420,11 +432,35 @@ PictureLayerTiling::CoverageIterator::CoverageIterator(
const TilingData& data = tiling_->tiling_data_;
left_ = data.LastBorderTileXIndexFromSrcCoord(wanted_texels.x());
top_ = data.LastBorderTileYIndexFromSrcCoord(wanted_texels.y());
+
right_ = std::max(
left_, data.FirstBorderTileXIndexFromSrcCoord(wanted_texels.right()));
bottom_ = std::max(
top_, data.FirstBorderTileYIndexFromSrcCoord(wanted_texels.bottom()));
+ // Unfortunately due to floating point math in ToEnclosingRect, wanted_texels
+ // could be lying about its dimensions. For example, consider the following:
+ //
+ // float a = 192.f;
+ // float b = 2104670720.f;
+ // output("%.0f\n", static_cast<double>(a + b) - b);
+ //
+ // This code prints out 256. So if that happens to be our coverage rect
+ // position and dimension, then the texel extent would try to grab something
+ // outside of it, resulting in a DCHECK later (or empty geometry rect with
+ // DCHECks disabled), which is undesirable. The solution is to simply
+ // backtrack the value until the content rect actually intersects the texel
+ // extent for the values we generated. Note this only happens with
+ // right/bottom values.
+ while (!content_rect.Intersects(data.TexelExtent(right_, top_)) &&
enne (OOO) 2017/04/14 05:35:56 Hmmm, do you think that FirstBorderTileXIndexFromS
vmpstr 2017/04/14 18:17:42 The problem is that FirstBorderTileXIndexFromSrcCo
trchen 2017/04/14 22:07:33 Just double checked FirstBorderTileXIndexFromSrcCo
vmpstr 2017/04/14 23:36:26 Yes, I meant the access to right/bottom in enclosi
trchen 2017/04/18 21:13:46 Just sync'd up with enne@ offline. We agreed that
+ right_ > left_) {
+ --right_;
+ }
+ while (!content_rect.Intersects(data.TexelExtent(left_, bottom_)) &&
+ bottom_ > top_) {
+ --bottom_;
+ }
+
tile_i_ = left_ - 1;
tile_j_ = top_;
++(*this);
@@ -451,6 +487,8 @@ PictureLayerTiling::CoverageIterator::operator++() {
}
}
+ DCHECK_LT(tile_i_, tiling_->tiling_data_.num_tiles_x());
+ DCHECK_LT(tile_j_, tiling_->tiling_data_.num_tiles_y());
current_tile_ = tiling_->TileAt(tile_i_, tile_j_);
// Calculate the current geometry rect. As we reserved overlap between tiles
@@ -521,11 +559,13 @@ PictureLayerTiling::CoverageIterator::operator++() {
int inset_top = std::max(0, min_top - current_geometry_rect_.y());
current_geometry_rect_.Inset(inset_left, inset_top, 0, 0);
+#if DCHECK_IS_ON()
enne (OOO) 2017/04/14 05:35:56 Do you need these extra #ifs?
vmpstr 2017/04/14 18:17:42 Not really, it's just everything is DCHECK in ther
if (!new_row) {
DCHECK_EQ(last_geometry_rect.right(), current_geometry_rect_.x());
DCHECK_EQ(last_geometry_rect.bottom(), current_geometry_rect_.bottom());
DCHECK_EQ(last_geometry_rect.y(), current_geometry_rect_.y());
}
+#endif
return *this;
}
@@ -584,7 +624,7 @@ void PictureLayerTiling::ComputeTilePriorityRects(
}
const float content_to_screen_scale =
- ideal_contents_scale / raster_transform_.scale();
+ SnapToOne(ideal_contents_scale / raster_transform_.scale());
const gfx::Rect* input_rects[] = {
&visible_rect_in_layer_space, &skewport_in_layer_space,
« no previous file with comments | « no previous file | cc/tiles/picture_layer_tiling_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698