change SkCanvasStack to take ownership of its subcanvases
Inspired by https://bugs.chromium.org/p/chromium/issues/detail?id=663959
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=4799
Change-Id: I69f7ac73386bb7ca96778e2fec4cb2757b982a52
Reviewed-on: https://skia-review.googlesource.com/4799
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Florin Malita <fmalita@chromium.org>
diff --git a/src/utils/SkCanvasStack.cpp b/src/utils/SkCanvasStack.cpp
index 58607d7..8e4bd37 100644
--- a/src/utils/SkCanvasStack.cpp
+++ b/src/utils/SkCanvasStack.cpp
@@ -13,18 +13,19 @@
this->removeAll();
}
-void SkCanvasStack::pushCanvas(SkCanvas* canvas, const SkIPoint& origin) {
+void SkCanvasStack::pushCanvas(std::unique_ptr<SkCanvas> canvas, const SkIPoint& origin) {
if (canvas) {
// compute the bounds of this canvas
const SkIRect canvasBounds = SkIRect::MakeSize(canvas->getDeviceSize());
// push the canvas onto the stack
- this->INHERITED::addCanvas(canvas);
+ this->INHERITED::addCanvas(canvas.get());
// push the canvas data onto the stack
CanvasData* data = &fCanvasData.push_back();
data->origin = origin;
data->requiredClip.setRect(canvasBounds);
+ data->ownedCanvas = std::move(canvas);
// subtract this region from the canvas objects already on the stack.
// This ensures they do not draw into the space occupied by the layers
@@ -41,8 +42,8 @@
}
void SkCanvasStack::removeAll() {
+ this->INHERITED::removeAll(); // call the baseclass *before* we actually delete the canvases
fCanvasData.reset();
- this->INHERITED::removeAll();
}
/**
diff --git a/src/utils/SkCanvasStack.h b/src/utils/SkCanvasStack.h
index 762ab9f..0e6e4b6 100644
--- a/src/utils/SkCanvasStack.h
+++ b/src/utils/SkCanvasStack.h
@@ -11,12 +11,18 @@
#include "SkNWayCanvas.h"
#include "SkTArray.h"
+/**
+ * Like NWayCanvas, in that it forwards all canvas methods to each sub-canvas that is "pushed".
+ *
+ * Unlike NWayCanvas, this takes ownership of each subcanvas, and deletes them when this canvas
+ * is deleted.
+ */
class SkCanvasStack : public SkNWayCanvas {
public:
SkCanvasStack(int width, int height);
virtual ~SkCanvasStack();
- void pushCanvas(SkCanvas* canvas, const SkIPoint& origin);
+ void pushCanvas(std::unique_ptr<SkCanvas>, const SkIPoint& origin);
void removeAll() override;
/*
@@ -42,6 +48,7 @@
struct CanvasData {
SkIPoint origin;
SkRegion requiredClip;
+ std::unique_ptr<SkCanvas> ownedCanvas;
};
SkTArray<CanvasData> fCanvasData;
diff --git a/src/utils/SkCanvasStateUtils.cpp b/src/utils/SkCanvasStateUtils.cpp
index 6ee1c33..c1307b2 100644
--- a/src/utils/SkCanvasStateUtils.cpp
+++ b/src/utils/SkCanvasStateUtils.cpp
@@ -337,8 +337,8 @@
if (!canvasLayer.get()) {
return nullptr;
}
- canvas->pushCanvas(canvasLayer.get(), SkIPoint::Make(state_v1->layers[i].x,
- state_v1->layers[i].y));
+ canvas->pushCanvas(std::move(canvasLayer), SkIPoint::Make(state_v1->layers[i].x,
+ state_v1->layers[i].y));
}
return std::move(canvas);
diff --git a/tests/CanvasTest.cpp b/tests/CanvasTest.cpp
index 3f418db..1824c25 100644
--- a/tests/CanvasTest.cpp
+++ b/tests/CanvasTest.cpp
@@ -734,3 +734,71 @@
canvas.restore();
}
+///////////////////////////////////////////////////////////////////////////////////////////////////
+
+#include "SkCanvasStack.h"
+#include "SkNWayCanvas.h"
+
+// Subclass that takes a bool*, which it updates in its construct (true) and destructor (false)
+// to allow the caller to know how long the object is alive.
+class LifeLineCanvas : public SkCanvas {
+ bool* fLifeLine;
+public:
+ LifeLineCanvas(int w, int h, bool* lifeline) : SkCanvas(w, h), fLifeLine(lifeline) {
+ *fLifeLine = true;
+ }
+ ~LifeLineCanvas() {
+ *fLifeLine = false;
+ }
+};
+
+// Check that NWayCanvas does NOT try to manage the lifetime of its sub-canvases
+DEF_TEST(NWayCanvas, r) {
+ const int w = 10;
+ const int h = 10;
+ bool life[2];
+ {
+ LifeLineCanvas c0(w, h, &life[0]);
+ REPORTER_ASSERT(r, life[0]);
+ }
+ REPORTER_ASSERT(r, !life[0]);
+
+
+ std::unique_ptr<SkCanvas> c0 = std::unique_ptr<SkCanvas>(new LifeLineCanvas(w, h, &life[0]));
+ std::unique_ptr<SkCanvas> c1 = std::unique_ptr<SkCanvas>(new LifeLineCanvas(w, h, &life[1]));
+ REPORTER_ASSERT(r, life[0]);
+ REPORTER_ASSERT(r, life[1]);
+
+ {
+ SkNWayCanvas nway(w, h);
+ nway.addCanvas(c0.get());
+ nway.addCanvas(c1.get());
+ REPORTER_ASSERT(r, life[0]);
+ REPORTER_ASSERT(r, life[1]);
+ }
+ // Now assert that the death of the nway has NOT also killed the sub-canvases
+ REPORTER_ASSERT(r, life[0]);
+ REPORTER_ASSERT(r, life[1]);
+}
+
+// Check that CanvasStack DOES manage the lifetime of its sub-canvases
+DEF_TEST(CanvasStack, r) {
+ const int w = 10;
+ const int h = 10;
+ bool life[2];
+ std::unique_ptr<SkCanvas> c0 = std::unique_ptr<SkCanvas>(new LifeLineCanvas(w, h, &life[0]));
+ std::unique_ptr<SkCanvas> c1 = std::unique_ptr<SkCanvas>(new LifeLineCanvas(w, h, &life[1]));
+ REPORTER_ASSERT(r, life[0]);
+ REPORTER_ASSERT(r, life[1]);
+
+ {
+ SkCanvasStack stack(w, h);
+ stack.pushCanvas(std::move(c0), {0,0});
+ stack.pushCanvas(std::move(c1), {0,0});
+ REPORTER_ASSERT(r, life[0]);
+ REPORTER_ASSERT(r, life[1]);
+ }
+ // Now assert that the death of the canvasstack has also killed the sub-canvases
+ REPORTER_ASSERT(r, !life[0]);
+ REPORTER_ASSERT(r, !life[1]);
+}
