Commit 49402155 authored by yiyix's avatar yiyix Committed by Commit Bot

Fix Regression: Integer-overflow in viz::Display::RemoveOverdrawQuads

In draw occlusion algorithm, the total area of drawing is calculated to
measure the effectiveness of draw occlusion. However, the integer
multiplication in the area calculation has caused the integer overflow
crash with huge draw quads. In this patch, I used GetCheckedArea() to
check for the resulting area before assigning.

Bug: 826727

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I667b458ca6f7d477ba13036868536dd842e59068
Reviewed-on: https://chromium-review.googlesource.com/991121
Commit-Queue: Yi Xu <yiyix@chromium.org>
Reviewed-by: default avatardanakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547856}
parent 56c8ecc0
...@@ -595,7 +595,7 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) { ...@@ -595,7 +595,7 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) {
if (!pass->filters.IsEmpty() || !pass->background_filters.IsEmpty()) { if (!pass->filters.IsEmpty() || !pass->background_filters.IsEmpty()) {
for (auto* const quad : pass->quad_list) { for (auto* const quad : pass->quad_list) {
total_quad_area_shown_wo_occlusion_px += total_quad_area_shown_wo_occlusion_px +=
quad->visible_rect.height() * quad->visible_rect.width(); quad->visible_rect.size().GetCheckedArea();
} }
continue; continue;
} }
...@@ -605,7 +605,7 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) { ...@@ -605,7 +605,7 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) {
if (pass != frame->render_pass_list.back()) { if (pass != frame->render_pass_list.back()) {
for (auto* const quad : pass->quad_list) { for (auto* const quad : pass->quad_list) {
total_quad_area_shown_wo_occlusion_px += total_quad_area_shown_wo_occlusion_px +=
quad->visible_rect.height() * quad->visible_rect.width(); quad->visible_rect.size().GetCheckedArea();
} }
continue; continue;
} }
...@@ -614,7 +614,7 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) { ...@@ -614,7 +614,7 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) {
gfx::Rect occlusion_in_quad_content_space; gfx::Rect occlusion_in_quad_content_space;
for (auto quad = pass->quad_list.begin(); quad != quad_list_end;) { for (auto quad = pass->quad_list.begin(); quad != quad_list_end;) {
total_quad_area_shown_wo_occlusion_px += total_quad_area_shown_wo_occlusion_px +=
quad->visible_rect.height() * quad->visible_rect.width(); quad->visible_rect.size().GetCheckedArea();
// Skip quad if it is a RenderPassDrawQuad because RenderPassDrawQuad is a // Skip quad if it is a RenderPassDrawQuad because RenderPassDrawQuad is a
// special type of DrawQuad where the visible_rect of shared quad state is // special type of DrawQuad where the visible_rect of shared quad state is
...@@ -693,8 +693,7 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) { ...@@ -693,8 +693,7 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) {
// Case 1: for simple transforms (scale or translation), define the // Case 1: for simple transforms (scale or translation), define the
// occlusion region in the quad content space. If the |quad| is not // occlusion region in the quad content space. If the |quad| is not
// shown on the screen, then remove |quad| from the compositor frame. // shown on the screen, then remove |quad| from the compositor frame.
total_area_saved_in_px += total_area_saved_in_px += quad->visible_rect.size().GetCheckedArea();
quad->visible_rect.height() * quad->visible_rect.width();
quad = pass->quad_list.EraseAndInvalidateAllPointers(quad); quad = pass->quad_list.EraseAndInvalidateAllPointers(quad);
} else if (occlusion_in_quad_content_space.Intersects( } else if (occlusion_in_quad_content_space.Intersects(
...@@ -706,7 +705,7 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) { ...@@ -706,7 +705,7 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) {
quad->visible_rect.Subtract(occlusion_in_quad_content_space); quad->visible_rect.Subtract(occlusion_in_quad_content_space);
if (origin_rect != quad->visible_rect) { if (origin_rect != quad->visible_rect) {
origin_rect.Subtract(quad->visible_rect); origin_rect.Subtract(quad->visible_rect);
total_area_saved_in_px += origin_rect.height() * origin_rect.width(); total_area_saved_in_px += origin_rect.size().GetCheckedArea();
} }
++quad; ++quad;
} else if (occlusion_in_quad_content_space.IsEmpty() && } else if (occlusion_in_quad_content_space.IsEmpty() &&
...@@ -716,14 +715,14 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) { ...@@ -716,14 +715,14 @@ void Display::RemoveOverdrawQuads(CompositorFrame* frame) {
// Case 3: for non simple transforms, define the occlusion region in // Case 3: for non simple transforms, define the occlusion region in
// target space. If the |quad| is not shown on the screen, then remove // target space. If the |quad| is not shown on the screen, then remove
// |quad| from the compositor frame. // |quad| from the compositor frame.
total_area_saved_in_px += total_area_saved_in_px += quad->visible_rect.size().GetCheckedArea();
quad->visible_rect.height() * quad->visible_rect.width();
quad = pass->quad_list.EraseAndInvalidateAllPointers(quad); quad = pass->quad_list.EraseAndInvalidateAllPointers(quad);
} else { } else {
++quad; ++quad;
} }
} }
} }
UMA_HISTOGRAM_PERCENTAGE( UMA_HISTOGRAM_PERCENTAGE(
"Compositing.Display.Draw.Occlusion.Percentage.Saved", "Compositing.Display.Draw.Occlusion.Percentage.Saved",
total_quad_area_shown_wo_occlusion_px.ValueOrDefault(0) == 0 total_quad_area_shown_wo_occlusion_px.ValueOrDefault(0) == 0
......
...@@ -3101,9 +3101,9 @@ TEST_F(DisplayTest, DrawOcclusionWithLargeDrawQuad) { ...@@ -3101,9 +3101,9 @@ TEST_F(DisplayTest, DrawOcclusionWithLargeDrawQuad) {
display_->Initialize(&client, manager_.surface_manager()); display_->Initialize(&client, manager_.surface_manager());
CompositorFrame frame = MakeDefaultCompositorFrame(); CompositorFrame frame = MakeDefaultCompositorFrame();
// The size of this DrawQuad will be 19770x97790 > 2^32 (uint32_t.max()) // The size of this DrawQuad will be 237790x237790 > 2^32 (uint32_t.max())
// which caused the integer overflow in the bug. // which caused the integer overflow in the bug.
gfx::Rect rect1(197790, 97790); gfx::Rect rect1(237790, 237790);
bool is_clipped = false; bool is_clipped = false;
bool are_contents_opaque = true; bool are_contents_opaque = true;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment