From 88e0891f6657573a5ad918c2d76d6c02bb8ceba3 Mon Sep 17 00:00:00 2001
From: Stan Iliev <stani@google.com>
Date: Tue, 22 Nov 2016 18:19:29 -0500
Subject: [PATCH] Fix draw order for non-RenderNode draw commands

Fix a drawing order issue in Skia pipeline. Add unit test in both
HWUI and Skia to test the fix.

Test: built and ran on angler-eng and HWUI unit tests passed.
Bug: 32506749
Change-Id: I7f13457726a8664f18a46aca2279b876acec2944
---
 .../hwui/pipeline/skia/SkiaRecordingCanvas.cpp | 18 ++++++------------
 libs/hwui/pipeline/skia/SkiaRecordingCanvas.h  |  1 -
 libs/hwui/tests/unit/FrameBuilderTests.cpp     | 10 +++++++++-
 .../tests/unit/RenderNodeDrawableTests.cpp     | 10 +++++++++-
 4 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/libs/hwui/pipeline/skia/SkiaRecordingCanvas.cpp b/libs/hwui/pipeline/skia/SkiaRecordingCanvas.cpp
index 6df544fa9e77d..95db2586f1bc9 100644
--- a/libs/hwui/pipeline/skia/SkiaRecordingCanvas.cpp
+++ b/libs/hwui/pipeline/skia/SkiaRecordingCanvas.cpp
@@ -32,7 +32,6 @@ namespace skiapipeline {
 
 void SkiaRecordingCanvas::initDisplayList(uirenderer::RenderNode* renderNode, int width,
         int height) {
-    mBarrierPending = false;
     mCurrentBarrier = nullptr;
     SkASSERT(mDisplayList.get() == nullptr);
 
@@ -76,8 +75,6 @@ void SkiaRecordingCanvas::drawCircle(uirenderer::CanvasPropertyPrimitive* x,
 }
 
 void SkiaRecordingCanvas::insertReorderBarrier(bool enableReorder) {
-    mBarrierPending = enableReorder;
-
     if (nullptr != mCurrentBarrier) {
         // finish off the existing chunk
         SkDrawable* drawable =
@@ -86,6 +83,12 @@ void SkiaRecordingCanvas::insertReorderBarrier(bool enableReorder) {
         mCurrentBarrier = nullptr;
         drawDrawable(drawable);
     }
+    if (enableReorder) {
+        mCurrentBarrier = (StartReorderBarrierDrawable*)
+                mDisplayList->allocateDrawable<StartReorderBarrierDrawable>(
+                mDisplayList.get());
+        drawDrawable(mCurrentBarrier);
+    }
 }
 
 void SkiaRecordingCanvas::drawLayer(uirenderer::DeferredLayerUpdater* layerUpdater) {
@@ -97,15 +100,6 @@ void SkiaRecordingCanvas::drawLayer(uirenderer::DeferredLayerUpdater* layerUpdat
 }
 
 void SkiaRecordingCanvas::drawRenderNode(uirenderer::RenderNode* renderNode) {
-    // lazily create the chunk if needed
-    if (mBarrierPending) {
-        mCurrentBarrier = (StartReorderBarrierDrawable*)
-                mDisplayList->allocateDrawable<StartReorderBarrierDrawable>(
-                mDisplayList.get());
-        drawDrawable(mCurrentBarrier);
-        mBarrierPending = false;
-    }
-
     // record the child node
     mDisplayList->mChildNodes.emplace_back(renderNode, asSkCanvas(), true, mCurrentBarrier);
     auto& renderNodeDrawable = mDisplayList->mChildNodes.back();
diff --git a/libs/hwui/pipeline/skia/SkiaRecordingCanvas.h b/libs/hwui/pipeline/skia/SkiaRecordingCanvas.h
index 8aef97f615ce7..10829f87efb92 100644
--- a/libs/hwui/pipeline/skia/SkiaRecordingCanvas.h
+++ b/libs/hwui/pipeline/skia/SkiaRecordingCanvas.h
@@ -76,7 +76,6 @@ class SkiaRecordingCanvas : public SkiaCanvas {
 private:
     SkLiteRecorder mRecorder;
     std::unique_ptr<SkiaDisplayList> mDisplayList;
-    bool mBarrierPending;
     StartReorderBarrierDrawable* mCurrentBarrier;
 
     /**
diff --git a/libs/hwui/tests/unit/FrameBuilderTests.cpp b/libs/hwui/tests/unit/FrameBuilderTests.cpp
index 8c956e5bcc843..950b2c45f8930 100644
--- a/libs/hwui/tests/unit/FrameBuilderTests.cpp
+++ b/libs/hwui/tests/unit/FrameBuilderTests.cpp
@@ -1542,6 +1542,14 @@ RENDERTHREAD_TEST(FrameBuilder, zReorder) {
         canvas.insertReorderBarrier(false);
         drawOrderedRect(&canvas, 8);
         drawOrderedNode(&canvas, 9, -10.0f); // in reorder=false at this point, so played inorder
+        canvas.insertReorderBarrier(true); //reorder a node ahead of drawrect op
+        drawOrderedRect(&canvas, 11);
+        drawOrderedNode(&canvas, 10, -1.0f);
+        canvas.insertReorderBarrier(false);
+        canvas.insertReorderBarrier(true); //test with two empty reorder sections
+        canvas.insertReorderBarrier(true);
+        canvas.insertReorderBarrier(false);
+        drawOrderedRect(&canvas, 12);
     });
     FrameBuilder frameBuilder(SkRect::MakeWH(100, 100), 100, 100,
             sLightGeometry, Caches::getInstance());
@@ -1549,7 +1557,7 @@ RENDERTHREAD_TEST(FrameBuilder, zReorder) {
 
     ZReorderTestRenderer renderer;
     frameBuilder.replayBakedOps<TestDispatcher>(renderer);
-    EXPECT_EQ(10, renderer.getIndex());
+    EXPECT_EQ(13, renderer.getIndex());
 };
 
 RENDERTHREAD_TEST(FrameBuilder, projectionReorder) {
diff --git a/libs/hwui/tests/unit/RenderNodeDrawableTests.cpp b/libs/hwui/tests/unit/RenderNodeDrawableTests.cpp
index ae4f0f42e6696..fafce86740ead 100644
--- a/libs/hwui/tests/unit/RenderNodeDrawableTests.cpp
+++ b/libs/hwui/tests/unit/RenderNodeDrawableTests.cpp
@@ -114,13 +114,21 @@ TEST(RenderNodeDrawable, zReorder) {
         canvas.insertReorderBarrier(false);
         drawOrderedRect(&canvas, 8);
         drawOrderedNode(&canvas, 9, -10.0f); // in reorder=false at this point, so played inorder
+        canvas.insertReorderBarrier(true); //reorder a node ahead of drawrect op
+        drawOrderedRect(&canvas, 11);
+        drawOrderedNode(&canvas, 10, -1.0f);
+        canvas.insertReorderBarrier(false);
+        canvas.insertReorderBarrier(true); //test with two empty reorder sections
+        canvas.insertReorderBarrier(true);
+        canvas.insertReorderBarrier(false);
+        drawOrderedRect(&canvas, 12);
     });
 
     //create a canvas not backed by any device/pixels, but with dimensions to avoid quick rejection
     ZReorderCanvas canvas(100, 100);
     RenderNodeDrawable drawable(parent.get(), &canvas, false);
     canvas.drawDrawable(&drawable);
-    EXPECT_EQ(10, canvas.getIndex());
+    EXPECT_EQ(13, canvas.getIndex());
 }
 
 TEST(RenderNodeDrawable, composeOnLayer)