From 584ca89d3b7a7781ea0407ee4d1c953fc7085e75 Mon Sep 17 00:00:00 2001 From: Mike Reed Date: Tue, 15 Nov 2016 11:52:55 -0500 Subject: [PATCH] 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 Reviewed-by: Florin Malita --- src/utils/SkCanvasStack.cpp | 7 ++-- src/utils/SkCanvasStack.h | 9 ++++- src/utils/SkCanvasStateUtils.cpp | 4 +- tests/CanvasTest.cpp | 68 ++++++++++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 6 deletions(-) diff --git a/src/utils/SkCanvasStack.cpp b/src/utils/SkCanvasStack.cpp index 58607d7d29..8e4bd37540 100644 --- a/src/utils/SkCanvasStack.cpp +++ b/src/utils/SkCanvasStack.cpp @@ -13,18 +13,19 @@ SkCanvasStack::~SkCanvasStack() { this->removeAll(); } -void SkCanvasStack::pushCanvas(SkCanvas* canvas, const SkIPoint& origin) { +void SkCanvasStack::pushCanvas(std::unique_ptr 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::pushCanvas(SkCanvas* canvas, const SkIPoint& origin) { } 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 762ab9f76f..0e6e4b6381 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, const SkIPoint& origin); void removeAll() override; /* @@ -42,6 +48,7 @@ private: struct CanvasData { SkIPoint origin; SkRegion requiredClip; + std::unique_ptr ownedCanvas; }; SkTArray fCanvasData; diff --git a/src/utils/SkCanvasStateUtils.cpp b/src/utils/SkCanvasStateUtils.cpp index 6ee1c338fd..c1307b27cc 100644 --- a/src/utils/SkCanvasStateUtils.cpp +++ b/src/utils/SkCanvasStateUtils.cpp @@ -337,8 +337,8 @@ std::unique_ptr SkCanvasStateUtils::MakeFromCanvasState(const SkCanvas 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 3f418db4ae..1824c25aad 100644 --- a/tests/CanvasTest.cpp +++ b/tests/CanvasTest.cpp @@ -734,3 +734,71 @@ DEF_TEST(DeferredCanvas, r) { 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 c0 = std::unique_ptr(new LifeLineCanvas(w, h, &life[0])); + std::unique_ptr c1 = std::unique_ptr(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 c0 = std::unique_ptr(new LifeLineCanvas(w, h, &life[0])); + std::unique_ptr c1 = std::unique_ptr(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]); +}