From 0394fa5ef5550348f6f8339a9292887da4b2ca60 Mon Sep 17 00:00:00 2001 From: Hermet Park Date: Fri, 14 May 2021 20:19:03 +0900 Subject: [PATCH] common: fix a potential memory leak after clear() Paints must clear canvas engine data if they were dismissed from the canvas, 1. Canvas::clear(free = false) must retain all the paints from the paints hierarchy so that user keeps the all dangled paints lifecycle. In this scenario, it could leak the engine data of paints, this patch fixes it. 2. Previously, t just keeps the immediate paints lives of canvas, but not them of children of scene nor picture. This patch changes a policy which was not considered seriously, Now it keeps the all paints lives through the tree-hieararchy. 3. Also changed the Scene::clear() behavior identical to Canvas::clear() for consistency. @API Modification: From: Result Scene::clear() noexcept; To: Result Scene::clear(bool free = true) noexcept; --- inc/thorvg.h | 8 +++++++- inc/thorvg_capi.h | 17 +++++++++++++---- src/bindings/capi/tvgCapi.cpp | 4 ++-- src/lib/tvgCanvasImpl.h | 8 +++----- src/lib/tvgPictureImpl.h | 10 ++++++---- src/lib/tvgScene.cpp | 4 ++-- src/lib/tvgSceneImpl.h | 31 +++++++++++++++++++++++++++++-- 7 files changed, 62 insertions(+), 20 deletions(-) diff --git a/inc/thorvg.h b/inc/thorvg.h index 867b574e..ab69109b 100644 --- a/inc/thorvg.h +++ b/inc/thorvg.h @@ -1095,12 +1095,18 @@ public: /** * @brief Sets the total number of the paints pushed into the scene to be zero. + * Depending on the value of the @p free argument, the paints are freed or not. * + * @param[in] free If @c true, the memory occupied by paints is deallocated, otherwise it is not. + * + * @return Result::Success when succeed + * + * @warning If you don't free the paints they become dangled. They are supposed to be reused, otherwise you are responsible for their lives. Thus please use the @p free argument only when you know how it works, otherwise it's not recommended. * @warning Please do not use it, this API is not official one. It could be modified in the next version. * * @BETA_API */ - Result clear() noexcept; + Result clear(bool free = true) noexcept; /** * @brief Creates a new Scene object. diff --git a/inc/thorvg_capi.h b/inc/thorvg_capi.h index 7895c652..a2a184d3 100644 --- a/inc/thorvg_capi.h +++ b/inc/thorvg_capi.h @@ -1724,12 +1724,21 @@ TVG_EXPORT Tvg_Result tvg_scene_push(Tvg_Paint* scene, Tvg_Paint* paint); /*! -* \brief Sets the total number of the paints pushed into the scene to be zero. (BETA version) +* \brief Clears a Tvg_Scene objects from pushed paints. * -* \warning Please do not use it, this API is not official one. It could be modified in the next version. +* Tvg_Paint objects stored in the scene are released if @p free is set to @c true, otherwise the memory is not deallocated and +* all paints should be released manually in order to avoid memory leaks. +* +* \param[in] scene The Tvg_Scene object to be cleared. +* \param[in] free If @c true the memory occupied by paints is deallocated, otherwise it is not. +* +* \return Tvg_Result enumeration. +* \retval TVG_RESULT_SUCCESS Succeed. +* \retval TVG_RESULT_INVALID_ARGUMENT An invalid Tvg_Canvas pointer. +* +* \warning Please use the @p free argument only when you know how it works, otherwise it's not recommended. */ -TVG_EXPORT Tvg_Result tvg_scene_clear(Tvg_Paint* scene); - +TVG_EXPORT Tvg_Result tvg_scene_clear(Tvg_Paint* scene, bool free); /** \} */ // end defgroup ThorVGCapi_Scene diff --git a/src/bindings/capi/tvgCapi.cpp b/src/bindings/capi/tvgCapi.cpp index 6eab385d..3be0085a 100644 --- a/src/bindings/capi/tvgCapi.cpp +++ b/src/bindings/capi/tvgCapi.cpp @@ -564,10 +564,10 @@ TVG_EXPORT Tvg_Result tvg_scene_push(Tvg_Paint* scene, Tvg_Paint* paint) } -TVG_EXPORT Tvg_Result tvg_scene_clear(Tvg_Paint* scene) +TVG_EXPORT Tvg_Result tvg_scene_clear(Tvg_Paint* scene, bool free) { if (!scene) return TVG_RESULT_INVALID_ARGUMENT; - return (Tvg_Result) reinterpret_cast(scene)->clear(); + return (Tvg_Result) reinterpret_cast(scene)->clear(free); } diff --git a/src/lib/tvgCanvasImpl.h b/src/lib/tvgCanvasImpl.h index 2772a23c..a225ad80 100644 --- a/src/lib/tvgCanvasImpl.h +++ b/src/lib/tvgCanvasImpl.h @@ -59,11 +59,9 @@ struct Canvas::Impl if (!renderer || !renderer->clear()) return Result::InsufficientCondition; //free paints - if (free) { - for (auto paint = paints.data; paint < (paints.data + paints.count); ++paint) { - (*paint)->pImpl->dispose(*renderer); - delete(*paint); - } + for (auto paint = paints.data; paint < (paints.data + paints.count); ++paint) { + (*paint)->pImpl->dispose(*renderer); + if (free) delete(*paint); } paints.clear(); diff --git a/src/lib/tvgPictureImpl.h b/src/lib/tvgPictureImpl.h index 1985fa77..4424a609 100644 --- a/src/lib/tvgPictureImpl.h +++ b/src/lib/tvgPictureImpl.h @@ -44,15 +44,17 @@ struct Picture::Impl { } + ~Impl() + { + if (paint) delete(paint); + } + bool dispose(RenderMethod& renderer) { bool ret = true; if (paint) { ret = paint->pImpl->dispose(renderer); - delete(paint); - paint = nullptr; - } - else if (pixels) { + } else if (pixels) { ret = renderer.dispose(rdata); rdata = nullptr; } diff --git a/src/lib/tvgScene.cpp b/src/lib/tvgScene.cpp index d9926c67..873053bf 100644 --- a/src/lib/tvgScene.cpp +++ b/src/lib/tvgScene.cpp @@ -62,9 +62,9 @@ Result Scene::reserve(uint32_t size) noexcept } -Result Scene::clear() noexcept +Result Scene::clear(bool free) noexcept { - pImpl->paints.clear(); + pImpl->clear(free); return Result::Success; } diff --git a/src/lib/tvgSceneImpl.h b/src/lib/tvgSceneImpl.h index a83813ca..f53fec62 100644 --- a/src/lib/tvgSceneImpl.h +++ b/src/lib/tvgSceneImpl.h @@ -33,14 +33,22 @@ struct Scene::Impl { Array paints; uint8_t opacity; //for composition + RenderMethod* renderer = nullptr; //keep it for explicit clear + + ~Impl() + { + for (auto paint = paints.data; paint < (paints.data + paints.count); ++paint) { + delete(*paint); + } + } bool dispose(RenderMethod& renderer) { for (auto paint = paints.data; paint < (paints.data + paints.count); ++paint) { (*paint)->pImpl->dispose(renderer); - delete(*paint); } - paints.clear(); + + this->renderer = nullptr; return true; } @@ -69,6 +77,9 @@ struct Scene::Impl /* FXIME: it requires to return list of children engine data This is necessary for scene composition */ + + this->renderer = &renderer; + return nullptr; } @@ -158,6 +169,22 @@ struct Scene::Impl return ret.release(); } + + void clear(bool free) + { + //Clear render target before drawing + if (!renderer || !renderer->clear()) { + paints.clear(); + return; + } + //free paints + for (auto paint = paints.data; paint < (paints.data + paints.count); ++paint) { + (*paint)->pImpl->dispose(*renderer); + if (free) delete(*paint); + } + paints.clear(); + renderer = nullptr; + } }; #endif //_TVG_SCENE_IMPL_H_