Fix matrix mapping of negative rects

bug:27381362

Also rejects ops with empty clip at record time, and short circuits clip
intersection, when one is empty.

Change-Id: I842612da14ad8fd9f1ba5e9e4fd027ba4e08d365

7 files changed, 63 insertions, 2 deletions

diff --git a/libs/hwui/Android.mk b/libs/hwui/Android.mk
index da7b7fb55330..70995acbb4ca 100644
--- a/libs/hwui/Android.mk
+++ b/libs/hwui/Android.mk
@@ -242,6 +242,7 @@ LOCAL_SRC_FILES += \
     tests/unit/GpuMemoryTrackerTests.cpp \
     tests/unit/LayerUpdateQueueTests.cpp \
     tests/unit/LinearAllocatorTests.cpp \
+    tests/unit/MatrixTests.cpp \
     tests/unit/OffscreenBufferPoolTests.cpp \
     tests/unit/SkiaBehaviorTests.cpp \
     tests/unit/StringUtilsTests.cpp \
diff --git a/libs/hwui/ClipArea.cpp b/libs/hwui/ClipArea.cpp
index e368537f0b4f..501cbe50ee05 100644
--- a/libs/hwui/ClipArea.cpp
+++ b/libs/hwui/ClipArea.cpp
@@ -404,11 +404,17 @@ static bool cannotFitInRectangleList(const ClipArea& clipArea, const ClipBase* s
     return currentRectCount + recordedRectCount > RectangleList::kMaxTransformedRectangles;
 }
 
+static const ClipRect sEmptyClipRect(Rect(0, 0));
+
 const ClipBase* ClipArea::serializeIntersectedClip(LinearAllocator& allocator,
         const ClipBase* recordedClip, const Matrix4& recordedClipTransform) {
+
     // if no recordedClip passed, just serialize current state
     if (!recordedClip) return serializeClip(allocator);
 
+    // if either is empty, clip is empty
+    if (CC_UNLIKELY(recordedClip->rect.isEmpty())|| mClipRect.isEmpty()) return &sEmptyClipRect;
+
     if (!mLastResolutionResult
             || recordedClip != mLastResolutionClip
             || recordedClipTransform != mLastResolutionTransform) {
diff --git a/libs/hwui/Matrix.cpp b/libs/hwui/Matrix.cpp
index 73ebd1304750..deab95690d0e 100644
--- a/libs/hwui/Matrix.cpp
+++ b/libs/hwui/Matrix.cpp
@@ -438,7 +438,7 @@ void Matrix4::mapPoint(float& x, float& y) const {
 }
 
 void Matrix4::mapRect(Rect& r) const {
-    if (isIdentity()) return;
+    if (isIdentity() || r.isEmpty()) return;
 
     if (isSimple()) {
         MUL_ADD_STORE(r.left, data[kScaleX], data[kTranslateX]);
diff --git a/libs/hwui/RecordingCanvas.cpp b/libs/hwui/RecordingCanvas.cpp
index 31de305d5246..9ae2212d0732 100644
--- a/libs/hwui/RecordingCanvas.cpp
+++ b/libs/hwui/RecordingCanvas.cpp
@@ -594,7 +594,14 @@ void RecordingCanvas::callDrawGLFunction(Functor* functor) {
 }
 
 size_t RecordingCanvas::addOp(RecordedOp* op) {
-    // TODO: validate if "addDrawOp" quickrejection logic is useful before adding
+    // skip op with empty clip
+    if (op->localClip && op->localClip->rect.isEmpty()) {
+        // NOTE: this rejection happens after op construction/content ref-ing, so content ref'd
+        // and held by renderthread isn't affected by clip rejection.
+        // Could rewind alloc here if desired, but callers would have to not touch op afterwards.
+        return -1;
+    }
+
     int insertIndex = mDisplayList->ops.size();
     mDisplayList->ops.push_back(op);
     if (mDeferredBarrierType != DeferredBarrierType::None) {
diff --git a/libs/hwui/tests/unit/ClipAreaTests.cpp b/libs/hwui/tests/unit/ClipAreaTests.cpp
index 679569ef5a78..dc2ea0784b7e 100644
--- a/libs/hwui/tests/unit/ClipAreaTests.cpp
+++ b/libs/hwui/tests/unit/ClipAreaTests.cpp
@@ -228,6 +228,7 @@ TEST(ClipArea, serializeIntersectedClip) {
 
         ClipRegion recordedClip;
         recordedClip.region.setPath(ovalPath, SkRegion(SkIRect::MakeWH(200, 200)));
+        recordedClip.rect = Rect(200, 200);
 
         Matrix4 translate10x20;
         translate10x20.loadTranslate(10, 20, 0);
diff --git a/libs/hwui/tests/unit/MatrixTests.cpp b/libs/hwui/tests/unit/MatrixTests.cpp
new file mode 100644
index 000000000000..da2263788c25
--- /dev/null
+++ b/libs/hwui/tests/unit/MatrixTests.cpp
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <gtest/gtest.h>
+
+#include "Matrix.h"
+#include "Rect.h"
+
+using namespace android::uirenderer;
+
+TEST(Matrix, mapRect) {
+    // Skew, so we don't hit identity/translate/simple fast paths
+    Matrix4 matrix;
+    matrix.skew(0.1f, 0.1f);
+
+    // non-zero empty rect, so sorting x/y would make rect non-empty
+    Rect empty(100, 100, -100, -100);
+    ASSERT_TRUE(empty.isEmpty());
+    matrix.mapRect(empty);
+    EXPECT_TRUE(empty.isEmpty())
+        << "Empty rect should always remain empty, regardless of mapping.";
+}
diff --git a/libs/hwui/tests/unit/RecordingCanvasTests.cpp b/libs/hwui/tests/unit/RecordingCanvasTests.cpp
index f988da399665..c39047ca1d89 100644
--- a/libs/hwui/tests/unit/RecordingCanvasTests.cpp
+++ b/libs/hwui/tests/unit/RecordingCanvasTests.cpp
@@ -58,6 +58,17 @@ TEST(RecordingCanvas, clipRect) {
             << "Clip should be serialized once";
 }
 
+TEST(RecordingCanvas, emptyClipRect) {
+    auto dl = TestUtils::createDisplayList<RecordingCanvas>(200, 200, [](RecordingCanvas& canvas) {
+        canvas.save(SaveFlags::MatrixClip);
+        canvas.clipRect(0, 0, 100, 100, SkRegion::kIntersect_Op);
+        canvas.clipRect(100, 100, 200, 200, SkRegion::kIntersect_Op);
+        canvas.drawRect(0, 0, 50, 50, SkPaint()); // rejected at record time
+        canvas.restore();
+    });
+    ASSERT_EQ(0u, dl->getOps().size()) << "Must be zero ops. Rect should be rejected.";
+}
+
 TEST(RecordingCanvas, drawArc) {
     auto dl = TestUtils::createDisplayList<RecordingCanvas>(200, 200, [](RecordingCanvas& canvas) {
         canvas.drawArc(0, 0, 200, 200, 0, 180, true, SkPaint());