Commit f3848cbc authored by Xida Chen's avatar Xida Chen Committed by Chromium LUCI CQ

Ensure a PaintOp is rejected if the paint rect is empty

Currently a PaintOp is not necessary rejected even if its drawing
canvas has a size of (0, 0). This should not be the case, for
performance purpose, we should just skip this draw op.

In PS#1, I added a test which fails without any code change,
to indicate that we are not reject a PaintOp even if the canvas
has size of 0.

In PS#3, I added code changes to make the test pass. The
code change ensures that if the paint rect is empty, then
it should be skipped.

Bug: 1157152
Change-Id: I28110c26e838c917326df82bf1debdc67ca5b50d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2585628
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: default avatarvmpstr <vmpstr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836660}
parent 46f9dfec
...@@ -1529,13 +1529,6 @@ void DrawImageRectOp::RasterWithFlags(const DrawImageRectOp* op, ...@@ -1529,13 +1529,6 @@ void DrawImageRectOp::RasterWithFlags(const DrawImageRectOp* op,
// we should draw nothing. // we should draw nothing.
if (!params.image_provider) if (!params.image_provider)
return; return;
// TODO(crbug.com/1157152): We shouldn't need this check, QuickRejectDraw
// should have done the job.
const SkRect& clip_rect = SkRect::Make(canvas->getDeviceClipBounds());
const SkMatrix& ctm = canvas->getTotalMatrix();
gfx::Rect local_op_rect = PaintOp::ComputePaintRect(op, clip_rect, ctm);
if (local_op_rect.IsEmpty())
return;
ImageProvider::ScopedResult result = ImageProvider::ScopedResult result =
params.image_provider->GetRasterContent(DrawImage(op->image)); params.image_provider->GetRasterContent(DrawImage(op->image));
...@@ -2450,6 +2443,15 @@ bool PaintOp::QuickRejectDraw(const PaintOp* op, const SkCanvas* canvas) { ...@@ -2450,6 +2443,15 @@ bool PaintOp::QuickRejectDraw(const PaintOp* op, const SkCanvas* canvas) {
SkPaint paint = static_cast<const PaintOpWithFlags*>(op)->flags.ToSkPaint(); SkPaint paint = static_cast<const PaintOpWithFlags*>(op)->flags.ToSkPaint();
if (!paint.canComputeFastBounds()) if (!paint.canComputeFastBounds())
return false; return false;
// canvas->quickReject tried to be very fast, and sometimes give a false
// but conservative result. That's why we need the additional check for
// |local_op_rect| because it could quickReject could return false even if
// |local_op_rect| is empty.
const SkRect& clip_rect = SkRect::Make(canvas->getDeviceClipBounds());
const SkMatrix& ctm = canvas->getTotalMatrix();
gfx::Rect local_op_rect = PaintOp::ComputePaintRect(op, clip_rect, ctm);
if (local_op_rect.IsEmpty())
return true;
paint.computeFastBounds(rect, &rect); paint.computeFastBounds(rect, &rect);
} }
......
...@@ -8,8 +8,11 @@ ...@@ -8,8 +8,11 @@
#include <stdint.h> #include <stdint.h>
#include <limits> #include <limits>
#include <memory>
#include <string> #include <string>
#include <type_traits> #include <type_traits>
#include <utility>
#include <vector>
#include "base/callback.h" #include "base/callback.h"
#include "base/check_op.h" #include "base/check_op.h"
...@@ -1312,7 +1315,7 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt { ...@@ -1312,7 +1315,7 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
base::Optional<Iterator> iter_; base::Optional<Iterator> iter_;
}; };
class PlaybackFoldingIterator { class CC_PAINT_EXPORT PlaybackFoldingIterator {
public: public:
PlaybackFoldingIterator(const PaintOpBuffer* buffer, PlaybackFoldingIterator(const PaintOpBuffer* buffer,
const std::vector<size_t>* offsets); const std::vector<size_t>* offsets);
......
...@@ -727,6 +727,31 @@ class PaintOpBufferOffsetsTest : public ::testing::Test { ...@@ -727,6 +727,31 @@ class PaintOpBufferOffsetsTest : public ::testing::Test {
PaintOpBuffer buffer_; PaintOpBuffer buffer_;
}; };
TEST_F(PaintOpBufferOffsetsTest, EmptyClipRectShouldRejectAnOp) {
SkCanvas device(0, 0);
SkCanvas* canvas = &device;
canvas->translate(-254, 0);
SkIRect bounds = canvas->getDeviceClipBounds();
EXPECT_TRUE(bounds.isEmpty());
SkMatrix ctm = canvas->getTotalMatrix();
EXPECT_EQ(ctm[2], -254);
scoped_refptr<TestPaintWorkletInput> input =
base::MakeRefCounted<TestPaintWorkletInput>(gfx::SizeF(32.0f, 32.0f));
PaintImage image = CreatePaintWorkletPaintImage(input);
SkRect src = SkRect::MakeLTRB(0, 0, 100, 100);
SkRect dst = SkRect::MakeLTRB(168, -23, 268, 77);
push_op<DrawImageRectOp>(image, src, dst, nullptr,
SkCanvas::kStrict_SrcRectConstraint);
std::vector<size_t> offsets = Select({0});
for (PaintOpBuffer::PlaybackFoldingIterator iter(&buffer_, &offsets); iter;
++iter) {
const PaintOp* op = *iter;
EXPECT_EQ(op->GetType(), PaintOpType::DrawImageRect);
EXPECT_TRUE(PaintOp::QuickRejectDraw(op, canvas));
}
}
TEST_F(PaintOpBufferOffsetsTest, ContiguousIndices) { TEST_F(PaintOpBufferOffsetsTest, ContiguousIndices) {
testing::StrictMock<MockCanvas> canvas; testing::StrictMock<MockCanvas> canvas;
......
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