0

Clamp in gfx::ScaleTo*Rect() and fix bug

We had gfx::ScaleToEnclosingRectSafe() which clamps, and
gfx::ScaleTo*Rect() function which DCHECK'ed if the values would
overflow and expected the caller to ensure no overflow. However, none
of the callers of the latter functions can ensure that, so the DCHECKs
could fail. We had the two versions because performance of the safe
version had not been verified.

This CL makes all gfx::ScaleTo*Rect() functions safe, and removes
gfx::SaveToEnclosingRectSafe(). Will monitor performance after this
lands, and I feel this won't cause obvious performance regression
because the clamping has been used in hotter functions (e.g.
constructor of gfx::Size and gfx::Rect). Pinpoint jobs of rendering
benchmarks didn't show obvious difference with this CL.

This CL also fixes a bug of ScaleToEnclosingRectSafe() in cases like
  ScaleToEnclosingRectSafe(Rect(2, 3, 9, 8), 0.3).
Previously the function ceiled width/height instead of right/bottom,
which was incorrect for the above case for width (
expected: 0,1 4x3, actual: 0,1 3x3, i.e. the subpixels between
x=3 and x=3.3 in column 3 were not included).

This CL changes behavior of ScaleToEnclosingRectSafe() in some
edge cases like
ScaleToEnclosingRectSafe(Rect(-1000000, -1000000, 10, 20), 100000).
Previously the origin was clamped, and scaled size was kept. Now as
the whole rect is scaled out of minimum integer range, the result
is an empty rect at minimum integer location.

Bug: 738465
Change-Id: I6bfdc555b4f0220ae9ca947260cabb2b369f68d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3291301
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Owners-Override: Lei Zhang <thestig@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#944764}
This commit is contained in:
Xianzhu Wang
2021-11-24 00:49:24 +00:00
committed by Chromium LUCI CQ
parent 5f173bb811
commit 51c9108e49
8 changed files with 117 additions and 207 deletions

@ -4863,7 +4863,7 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTestWindowControlsOverlay,
double zoom_factor = blink::PageZoomLevelToZoomFactor(
content::HostZoomMap::GetZoomLevel(web_contents));
gfx::Rect scaled_rect =
gfx::ScaleToEnclosingRectSafe(bounding_client_rect, 1.0f / zoom_factor);
gfx::ScaleToEnclosingRect(bounding_client_rect, 1.0f / zoom_factor);
EXPECT_EQ(true, EvalJs(web_contents, "visible"));
EXPECT_EQ(scaled_rect.x(), EvalJs(web_contents, "rect.x"));

@ -510,7 +510,7 @@ bool OutOfProcessInstance::HandleInputEvent(const pp::InputEvent& event) {
}
void OutOfProcessInstance::DidChangeView(const pp::View& view) {
const gfx::Rect new_plugin_rect = gfx::ScaleToEnclosingRectSafe(
const gfx::Rect new_plugin_rect = gfx::ScaleToEnclosingRect(
RectFromPPRect(view.GetRect()), view.GetDeviceScale());
UpdateGeometryOnPluginRectChanged(new_plugin_rect, view.GetDeviceScale());
}

@ -890,7 +890,7 @@ void PdfViewPluginBase::UpdateGeometryOnPluginRectChanged(
// We should try to avoid the downscaling during this calculation process and
// maybe migrate off `plugin_dip_size_`.
plugin_dip_size_ =
gfx::ScaleToEnclosingRectSafe(new_plugin_rect, 1.0f / new_device_scale)
gfx::ScaleToEnclosingRect(new_plugin_rect, 1.0f / new_device_scale)
.size();
paint_manager_.SetSize(plugin_rect_.size(), device_scale_);

@ -673,7 +673,7 @@ void PdfViewWebPlugin::NotifySelectedFindResultChanged(int current_find_index) {
}
void PdfViewWebPlugin::CaretChanged(const gfx::Rect& caret_rect) {
caret_rect_ = gfx::ScaleToEnclosingRectSafe(
caret_rect_ = gfx::ScaleToEnclosingRect(
caret_rect + available_area().OffsetFromOrigin(), device_to_css_scale_);
}
@ -972,7 +972,7 @@ void PdfViewWebPlugin::OnViewportChanged(
// `plugin_rect_in_css_pixel` needs to be converted to device pixels before
// getting passed into PdfViewPluginBase::UpdateGeometryOnPluginRectChanged().
UpdateGeometryOnPluginRectChanged(
gfx::ScaleToEnclosingRectSafe(
gfx::ScaleToEnclosingRect(
plugin_rect_in_css_pixel,
client_->IsUseZoomForDSFEnabled() ? 1.0f : new_device_scale),
new_device_scale);

@ -2751,7 +2751,7 @@ void LocalFrame::UpdateWindowControlsOverlay(
const float zoom_factor = local_frame_root.PageZoomFactor();
const float scale_factor = zoom_factor / window_to_viewport_factor;
gfx::Rect window_controls_overlay_rect =
gfx::ScaleToEnclosingRectSafe(bounding_rect_in_dips, 1.0f / scale_factor);
gfx::ScaleToEnclosingRect(bounding_rect_in_dips, 1.0f / scale_factor);
bool fire_event =
(window_controls_overlay_rect != window_controls_overlay_rect_);

@ -331,7 +331,7 @@ _CONFIG = [
'gfx::RectToSkRect',
'gfx::ScalePoint',
'gfx::ScaleToCeiledSize',
'gfx::ScaleToEnclosingRectSafe',
'gfx::ScaleToEnclosingRect',
'gfx::ScaleToFlooredSize',
'gfx::ScaleSize',
'gfx::ScalePoint',

@ -323,57 +323,24 @@ GEOMETRY_EXPORT Rect SubtractRects(const Rect& a, const Rect& b);
// contained within the rect, because they will appear on one of these edges.
GEOMETRY_EXPORT Rect BoundingRect(const Point& p1, const Point& p2);
// Scales the rect and returns the enclosing rect. Use this only the inputs are
// known to not overflow. Use ScaleToEnclosingRectSafe if the inputs are
// unknown and need to use saturated math.
// Scales the rect and returns the enclosing rect. The components are clamped
// if they would overflow.
inline Rect ScaleToEnclosingRect(const Rect& rect,
float x_scale,
float y_scale) {
if (x_scale == 1.f && y_scale == 1.f)
return rect;
// These next functions cast instead of using e.g. base::ClampFloor() because
// we haven't checked to ensure that the clamping behavior of the helper
// functions doesn't degrade performance, and callers shouldn't be passing
// values that cause overflow anyway.
DCHECK(base::IsValueInRangeForNumericType<int>(
std::floor(rect.x() * x_scale)));
DCHECK(base::IsValueInRangeForNumericType<int>(
std::floor(rect.y() * y_scale)));
DCHECK(base::IsValueInRangeForNumericType<int>(
std::ceil(rect.right() * x_scale)));
DCHECK(base::IsValueInRangeForNumericType<int>(
std::ceil(rect.bottom() * y_scale)));
int x = static_cast<int>(std::floor(rect.x() * x_scale));
int y = static_cast<int>(std::floor(rect.y() * y_scale));
int r = rect.width() == 0 ?
x : static_cast<int>(std::ceil(rect.right() * x_scale));
int b = rect.height() == 0 ?
y : static_cast<int>(std::ceil(rect.bottom() * y_scale));
return Rect(x, y, r - x, b - y);
}
inline Rect ScaleToEnclosingRect(const Rect& rect, float scale) {
return ScaleToEnclosingRect(rect, scale, scale);
}
// ScaleToEnclosingRect but clamping instead of asserting if the resulting rect
// would overflow.
// TODO(pkasting): Attempt to switch ScaleTo...Rect() to this construction and
// check performance.
inline Rect ScaleToEnclosingRectSafe(const Rect& rect,
float x_scale,
float y_scale) {
if (x_scale == 1.f && y_scale == 1.f)
return rect;
int x = base::ClampFloor(rect.x() * x_scale);
int y = base::ClampFloor(rect.y() * y_scale);
int w = base::ClampCeil(rect.width() * x_scale);
int h = base::ClampCeil(rect.height() * y_scale);
return Rect(x, y, w, h);
int r = rect.width() == 0 ? x : base::ClampCeil(rect.right() * x_scale);
int b = rect.height() == 0 ? y : base::ClampCeil(rect.bottom() * y_scale);
Rect result;
result.SetByBounds(x, y, r, b);
return result;
}
inline Rect ScaleToEnclosingRectSafe(const Rect& rect, float scale) {
return ScaleToEnclosingRectSafe(rect, scale, scale);
inline Rect ScaleToEnclosingRect(const Rect& rect, float scale) {
return ScaleToEnclosingRect(rect, scale, scale);
}
inline Rect ScaleToEnclosedRect(const Rect& rect,
@ -381,21 +348,13 @@ inline Rect ScaleToEnclosedRect(const Rect& rect,
float y_scale) {
if (x_scale == 1.f && y_scale == 1.f)
return rect;
DCHECK(base::IsValueInRangeForNumericType<int>(
std::ceil(rect.x() * x_scale)));
DCHECK(base::IsValueInRangeForNumericType<int>(
std::ceil(rect.y() * y_scale)));
DCHECK(base::IsValueInRangeForNumericType<int>(
std::floor(rect.right() * x_scale)));
DCHECK(base::IsValueInRangeForNumericType<int>(
std::floor(rect.bottom() * y_scale)));
int x = static_cast<int>(std::ceil(rect.x() * x_scale));
int y = static_cast<int>(std::ceil(rect.y() * y_scale));
int r = rect.width() == 0 ?
x : static_cast<int>(std::floor(rect.right() * x_scale));
int b = rect.height() == 0 ?
y : static_cast<int>(std::floor(rect.bottom() * y_scale));
return Rect(x, y, r - x, b - y);
int x = base::ClampCeil(rect.x() * x_scale);
int y = base::ClampCeil(rect.y() * y_scale);
int r = rect.width() == 0 ? x : base::ClampFloor(rect.right() * x_scale);
int b = rect.height() == 0 ? y : base::ClampFloor(rect.bottom() * y_scale);
Rect result;
result.SetByBounds(x, y, r, b);
return result;
}
inline Rect ScaleToEnclosedRect(const Rect& rect, float scale) {
@ -404,33 +363,20 @@ inline Rect ScaleToEnclosedRect(const Rect& rect, float scale) {
// Scales |rect| by scaling its four corner points. If the corner points lie on
// non-integral coordinate after scaling, their values are rounded to the
// nearest integer.
// nearest integer. The components are clamped if they would overflow.
// This is helpful during layout when relative positions of multiple gfx::Rect
// in a given coordinate space needs to be same after scaling as it was before
// scaling. ie. this gives a lossless relative positioning of rects.
inline Rect ScaleToRoundedRect(const Rect& rect, float x_scale, float y_scale) {
if (x_scale == 1.f && y_scale == 1.f)
return rect;
DCHECK(
base::IsValueInRangeForNumericType<int>(std::round(rect.x() * x_scale)));
DCHECK(
base::IsValueInRangeForNumericType<int>(std::round(rect.y() * y_scale)));
DCHECK(base::IsValueInRangeForNumericType<int>(
std::round(rect.right() * x_scale)));
DCHECK(base::IsValueInRangeForNumericType<int>(
std::round(rect.bottom() * y_scale)));
int x = static_cast<int>(std::round(rect.x() * x_scale));
int y = static_cast<int>(std::round(rect.y() * y_scale));
int r = rect.width() == 0
? x
: static_cast<int>(std::round(rect.right() * x_scale));
int b = rect.height() == 0
? y
: static_cast<int>(std::round(rect.bottom() * y_scale));
return Rect(x, y, r - x, b - y);
int x = base::ClampRound(rect.x() * x_scale);
int y = base::ClampRound(rect.y() * y_scale);
int r = rect.width() == 0 ? x : base::ClampRound(rect.right() * x_scale);
int b = rect.height() == 0 ? y : base::ClampRound(rect.bottom() * y_scale);
Rect result;
result.SetByBounds(x, y, r, b);
return result;
}
inline Rect ScaleToRoundedRect(const Rect& rect, float scale) {

@ -382,95 +382,97 @@ TEST(RectTest, SharesEdgeWith) {
EXPECT_FALSE(r.SharesEdgeWith(just_right_no_edge));
}
TEST(RectTest, ScaleToEnclosedRect) {
static const struct Test {
Rect input_rect;
float input_scale;
Rect expected_rect;
} tests[] = {
{
Rect(),
5.f,
Rect(),
}, {
Rect(1, 1, 1, 1),
5.f,
Rect(5, 5, 5, 5),
}, {
Rect(-1, -1, 0, 0),
5.f,
Rect(-5, -5, 0, 0),
}, {
Rect(1, -1, 0, 1),
5.f,
Rect(5, -5, 0, 5),
}, {
Rect(-1, 1, 1, 0),
5.f,
Rect(-5, 5, 5, 0),
}, {
Rect(1, 2, 3, 4),
1.5f,
Rect(2, 3, 4, 6),
}, {
Rect(-1, -2, 0, 0),
1.5f,
Rect(-1, -3, 0, 0),
}
};
static void TestScaleRectOverflowClamp(Rect (*function)(const Rect&,
float,
float)) {
// The whole rect is scaled out of kMinInt.
Rect xy_underflow1(-100000, -123456, 10, 20);
EXPECT_EQ(Rect(kMinInt, kMinInt, 0, 0),
function(xy_underflow1, 100000, 100000));
for (size_t i = 0; i < base::size(tests); ++i) {
Rect result = ScaleToEnclosedRect(tests[i].input_rect,
tests[i].input_scale);
EXPECT_EQ(tests[i].expected_rect, result);
}
// This rect's right/bottom is 0. The origin overflows, and is clamped to
// -kMaxInt (instead of kMinInt) to keep width/height not overflowing.
Rect xy_underflow2(-100000, -123456, 100000, 123456);
EXPECT_EQ(Rect(-kMaxInt, -kMaxInt, kMaxInt, kMaxInt),
function(xy_underflow2, 100000, 100000));
// A location overflow means that width/right and bottom/top also
// overflow so need to be clamped.
Rect xy_overflow(100000, 123456, 10, 20);
EXPECT_EQ(Rect(kMaxInt, kMaxInt, 0, 0),
function(xy_overflow, 100000, 100000));
// In practice all rects are clamped to 0 width / 0 height so
// negative sizes don't matter, but try this for the sake of testing.
Rect size_underflow(-1, -2, 100000, 100000);
EXPECT_EQ(Rect(100000, 200000, 0, 0),
function(size_underflow, -100000, -100000));
Rect size_overflow(-1, -2, 123456, 234567);
EXPECT_EQ(Rect(-100000, -200000, kMaxInt, kMaxInt),
function(size_overflow, 100000, 100000));
// Verify width/right gets clamped properly too if x/y positive.
Rect size_overflow2(1, 2, 123456, 234567);
EXPECT_EQ(Rect(100000, 200000, kMaxInt - 100000, kMaxInt - 200000),
function(size_overflow2, 100000, 100000));
constexpr float kMaxIntAsFloat = static_cast<float>(kMaxInt);
Rect max_origin_rect(kMaxInt, kMaxInt, kMaxInt, kMaxInt);
// width/height of max_origin_rect has already been clamped to 0.
EXPECT_EQ(Rect(kMaxInt, kMaxInt, 0, 0), max_origin_rect);
EXPECT_EQ(Rect(kMaxInt, kMaxInt, 0, 0),
function(max_origin_rect, kMaxIntAsFloat, kMaxIntAsFloat));
Rect max_size_rect1(0, 0, kMaxInt, kMaxInt);
// Max sized rect can't be scaled up any further in any dimension.
EXPECT_EQ(max_size_rect1, function(max_size_rect1, 2, 3.5));
EXPECT_EQ(max_size_rect1,
function(max_size_rect1, kMaxIntAsFloat, kMaxIntAsFloat));
// Max sized ret scaled by negative scale is an empty rect.
EXPECT_EQ(Rect(), function(max_size_rect1, kMinInt, kMinInt));
Rect max_size_rect2(-kMaxInt, -kMaxInt, kMaxInt, kMaxInt);
EXPECT_EQ(max_size_rect2, function(max_size_rect2, 2, 3.5));
EXPECT_EQ(max_size_rect2,
function(max_size_rect2, kMaxIntAsFloat, kMaxIntAsFloat));
EXPECT_EQ(Rect(kMaxInt, kMaxInt, 0, 0),
function(max_size_rect2, kMinInt, kMinInt));
}
TEST(RectTest, ScaleToEnclosedRect) {
EXPECT_EQ(Rect(), ScaleToEnclosedRect(Rect(), 5.f));
EXPECT_EQ(Rect(5, 5, 5, 5), ScaleToEnclosedRect(Rect(1, 1, 1, 1), 5.f));
EXPECT_EQ(Rect(-5, -5, 0, 0), ScaleToEnclosedRect(Rect(-1, -1, 0, 0), 5.f));
EXPECT_EQ(Rect(5, -5, 0, 5), ScaleToEnclosedRect(Rect(1, -1, 0, 1), 5.f));
EXPECT_EQ(Rect(-5, 5, 5, 0), ScaleToEnclosedRect(Rect(-1, 1, 1, 0), 5.f));
EXPECT_EQ(Rect(2, 3, 4, 6), ScaleToEnclosedRect(Rect(1, 2, 3, 4), 1.5f));
EXPECT_EQ(Rect(-1, -3, 0, 0), ScaleToEnclosedRect(Rect(-1, -2, 0, 0), 1.5f));
EXPECT_EQ(Rect(1, 2, 2, 1), ScaleToEnclosedRect(Rect(2, 4, 9, 8), 0.3f));
TestScaleRectOverflowClamp(ScaleToEnclosedRect);
}
TEST(RectTest, ScaleToEnclosingRect) {
static const struct Test {
Rect input_rect;
float input_scale;
Rect expected_rect;
} tests[] = {
{
Rect(),
5.f,
Rect(),
}, {
Rect(1, 1, 1, 1),
5.f,
Rect(5, 5, 5, 5),
}, {
Rect(-1, -1, 0, 0),
5.f,
Rect(-5, -5, 0, 0),
}, {
Rect(1, -1, 0, 1),
5.f,
Rect(5, -5, 0, 5),
}, {
Rect(-1, 1, 1, 0),
5.f,
Rect(-5, 5, 5, 0),
}, {
Rect(1, 2, 3, 4),
1.5f,
Rect(1, 3, 5, 6),
}, {
Rect(-1, -2, 0, 0),
1.5f,
Rect(-2, -3, 0, 0),
}
};
EXPECT_EQ(Rect(), ScaleToEnclosingRect(Rect(), 5.f));
EXPECT_EQ(Rect(5, 5, 5, 5), ScaleToEnclosingRect(Rect(1, 1, 1, 1), 5.f));
EXPECT_EQ(Rect(-5, -5, 0, 0), ScaleToEnclosingRect(Rect(-1, -1, 0, 0), 5.f));
EXPECT_EQ(Rect(5, -5, 0, 5), ScaleToEnclosingRect(Rect(1, -1, 0, 1), 5.f));
EXPECT_EQ(Rect(-5, 5, 5, 0), ScaleToEnclosingRect(Rect(-1, 1, 1, 0), 5.f));
EXPECT_EQ(Rect(1, 3, 5, 6), ScaleToEnclosingRect(Rect(1, 2, 3, 4), 1.5f));
EXPECT_EQ(Rect(-2, -3, 0, 0), ScaleToEnclosingRect(Rect(-1, -2, 0, 0), 1.5f));
EXPECT_EQ(Rect(0, 1, 4, 3), ScaleToEnclosingRect(Rect(2, 4, 9, 8), 0.3f));
TestScaleRectOverflowClamp(ScaleToEnclosingRect);
}
for (size_t i = 0; i < base::size(tests); ++i) {
Rect result =
ScaleToEnclosingRect(tests[i].input_rect, tests[i].input_scale);
EXPECT_EQ(tests[i].expected_rect, result);
Rect result_safe =
ScaleToEnclosingRectSafe(tests[i].input_rect, tests[i].input_scale);
EXPECT_EQ(tests[i].expected_rect, result_safe);
}
TEST(RectTest, ScaleToRoundedRect) {
EXPECT_EQ(Rect(), ScaleToRoundedRect(Rect(), 5.f));
EXPECT_EQ(Rect(5, 5, 5, 5), ScaleToRoundedRect(Rect(1, 1, 1, 1), 5.f));
EXPECT_EQ(Rect(-5, -5, 0, 0), ScaleToRoundedRect(Rect(-1, -1, 0, 0), 5.f));
EXPECT_EQ(Rect(5, -5, 0, 5), ScaleToRoundedRect(Rect(1, -1, 0, 1), 5.f));
EXPECT_EQ(Rect(-5, 5, 5, 0), ScaleToRoundedRect(Rect(-1, 1, 1, 0), 5.f));
EXPECT_EQ(Rect(2, 3, 4, 6), ScaleToRoundedRect(Rect(1, 2, 3, 4), 1.5f));
EXPECT_EQ(Rect(-2, -3, 0, 0), ScaleToRoundedRect(Rect(-1, -2, 0, 0), 1.5f));
EXPECT_EQ(Rect(1, 1, 2, 3), ScaleToRoundedRect(Rect(2, 4, 9, 8), 0.3f));
TestScaleRectOverflowClamp(ScaleToRoundedRect);
}
#if defined(OS_WIN)
@ -740,44 +742,6 @@ TEST(RectTest, IntegerOverflow) {
}
}
TEST(RectTest, ScaleToEnclosingRectSafe) {
Rect xy_underflow(-100000, -123456, 10, 20);
EXPECT_EQ(ScaleToEnclosingRectSafe(xy_underflow, 100000),
Rect(kMinInt, kMinInt, 1000000, 2000000));
// A location overflow means that width/right and bottom/top also
// overflow so need to be clamped.
Rect xy_overflow(100000, 123456, 10, 20);
EXPECT_EQ(ScaleToEnclosingRectSafe(xy_overflow, 100000),
Rect(kMaxInt, kMaxInt, 0, 0));
// In practice all rects are clamped to 0 width / 0 height so
// negative sizes don't matter, but try this for the sake of testing.
Rect size_underflow(-1, -2, 100000, 100000);
EXPECT_EQ(ScaleToEnclosingRectSafe(size_underflow, -100000),
Rect(100000, 200000, 0, 0));
Rect size_overflow(-1, -2, 123456, 234567);
EXPECT_EQ(ScaleToEnclosingRectSafe(size_overflow, 100000),
Rect(-100000, -200000, kMaxInt, kMaxInt));
// Verify width/right gets clamped properly too if x/y positive.
Rect size_overflow2(1, 2, 123456, 234567);
EXPECT_EQ(ScaleToEnclosingRectSafe(size_overflow2, 100000),
Rect(100000, 200000, kMaxInt - 100000, kMaxInt - 200000));
Rect max_rect(kMaxInt, kMaxInt, kMaxInt, kMaxInt);
EXPECT_EQ(ScaleToEnclosingRectSafe(max_rect, static_cast<float>(kMaxInt)),
Rect(kMaxInt, kMaxInt, 0, 0));
Rect min_rect(kMinInt, kMinInt, kMaxInt, kMaxInt);
// Min rect can't be scaled up any further in any dimension.
EXPECT_EQ(ScaleToEnclosingRectSafe(min_rect, 2, 3.5), min_rect);
EXPECT_EQ(ScaleToEnclosingRectSafe(min_rect, static_cast<float>(kMaxInt)),
min_rect);
// Min rect scaled by min is an empty rect at (max, max)
EXPECT_EQ(ScaleToEnclosingRectSafe(min_rect, kMinInt), max_rect);
}
TEST(RectTest, Inset) {
Rect r(10, 20, 30, 40);
r.Inset(0);