From 5a7e828842c26f64bb6e0ef3e0019e1949b245ee Mon Sep 17 00:00:00 2001 From: Chet Haase Date: Fri, 4 Feb 2011 12:50:55 -0800 Subject: [PATCH] Fix crash when Paths are GCd in hw accelerated apps A recent change to optimize path rendering didn't account for the destruction of native objects by the VM finalizer. We may be done with the Java level version before we're done with the native structure that's used by the display list. For example, a drawing method on a View that creates a temporary path to render into the canvas will implicitly create a native structure that is put onto the GL display list. That temporary path may go away, but the native version should stick around as long as the display list does. The fix is to refcount the original native version of the path and only delete it when the refcoutn reaches zero (which means that it is no longer needed by any display list). This is a similar mechanism used for bitmaps and shaders. Change-Id: I4de1047415066d425d1c689aa60827f97729b470 --- core/jni/android/graphics/Path.cpp | 3 ++- libs/hwui/DisplayListRenderer.cpp | 17 +++++++++++++++ libs/hwui/DisplayListRenderer.h | 9 ++++++++ libs/hwui/ResourceCache.cpp | 35 ++++++++++++++++++++++++++++++ libs/hwui/ResourceCache.h | 4 ++++ 5 files changed, 67 insertions(+), 1 deletion(-) diff --git a/core/jni/android/graphics/Path.cpp b/core/jni/android/graphics/Path.cpp index 90c4dd453c2ed..eb9e004c9861c 100644 --- a/core/jni/android/graphics/Path.cpp +++ b/core/jni/android/graphics/Path.cpp @@ -36,7 +36,8 @@ public: static void finalizer(JNIEnv* env, jobject clazz, SkPath* obj) { #ifdef USE_OPENGL_RENDERER if (android::uirenderer::Caches::hasInstance()) { - android::uirenderer::Caches::getInstance().pathCache.removeDeferred(obj); + android::uirenderer::Caches::getInstance().resourceCache.destructor(obj); + return; } #endif delete obj; diff --git a/libs/hwui/DisplayListRenderer.cpp b/libs/hwui/DisplayListRenderer.cpp index 2df52ae65e129..d5d2ba07154c0 100644 --- a/libs/hwui/DisplayListRenderer.cpp +++ b/libs/hwui/DisplayListRenderer.cpp @@ -95,6 +95,10 @@ void DisplayList::clearResources() { delete mPaths.itemAt(i); } mPaths.clear(); + for (size_t i = 0; i < mOriginalPaths.size(); i++) { + caches.resourceCache.decrementRefcount(mOriginalPaths.itemAt(i)); + } + mOriginalPaths.clear(); for (size_t i = 0; i < mMatrices.size(); i++) { delete mMatrices.itemAt(i); @@ -146,6 +150,13 @@ void DisplayList::initFromDisplayListRenderer(const DisplayListRenderer& recorde mPaths.add(paths.itemAt(i)); } + const Vector &originalPaths = recorder.getOriginalPaths(); + for (size_t i = 0; i < originalPaths.size(); i++) { + SkPath* path = originalPaths.itemAt(i); + mOriginalPaths.add(path); + caches.resourceCache.incrementRefcount(path); + } + const Vector &matrices = recorder.getMatrices(); for (size_t i = 0; i < matrices.size(); i++) { mMatrices.add(matrices.itemAt(i)); @@ -519,6 +530,12 @@ void DisplayListRenderer::reset() { } mBitmapResources.clear(); + for (size_t i = 0; i < mOriginalPaths.size(); i++) { + SkPath* resource = mOriginalPaths.itemAt(i); + caches.resourceCache.decrementRefcount(resource); + } + mOriginalPaths.clear(); + for (size_t i = 0; i < mShaders.size(); i++) { caches.resourceCache.decrementRefcount(mShaders.itemAt(i)); } diff --git a/libs/hwui/DisplayListRenderer.h b/libs/hwui/DisplayListRenderer.h index 2d0e30a48e057..f39f37f860eb1 100644 --- a/libs/hwui/DisplayListRenderer.h +++ b/libs/hwui/DisplayListRenderer.h @@ -190,6 +190,7 @@ private: Vector mPaints; Vector mPaths; + Vector mOriginalPaths; Vector mMatrices; Vector mShaders; @@ -293,6 +294,10 @@ public: return mPaths; } + const Vector& getOriginalPaths() const { + return mOriginalPaths; + } + const Vector& getMatrices() const { return mMatrices; } @@ -371,6 +376,9 @@ private: if (pathCopy == NULL || pathCopy->getGenerationID() != path->getGenerationID()) { if (pathCopy == NULL) { pathCopy = path; + mOriginalPaths.add(path); + Caches& caches = Caches::getInstance(); + caches.resourceCache.incrementRefcount(path); } else { pathCopy = new SkPath(*path); mPaths.add(pathCopy); @@ -452,6 +460,7 @@ private: Vector mPaints; DefaultKeyedVector mPaintMap; + Vector mOriginalPaths; Vector mPaths; DefaultKeyedVector mPathMap; diff --git a/libs/hwui/ResourceCache.cpp b/libs/hwui/ResourceCache.cpp index 70d117a3622bb..87fdfb5212a13 100644 --- a/libs/hwui/ResourceCache.cpp +++ b/libs/hwui/ResourceCache.cpp @@ -65,6 +65,10 @@ void ResourceCache::incrementRefcount(SkBitmap* bitmapResource) { incrementRefcount((void*)bitmapResource, kBitmap); } +void ResourceCache::incrementRefcount(SkPath* pathResource) { + incrementRefcount((void*)pathResource, kPath); +} + void ResourceCache::incrementRefcount(SkiaShader* shaderResource) { shaderResource->getSkShader()->safeRef(); incrementRefcount((void*) shaderResource, kShader); @@ -94,6 +98,10 @@ void ResourceCache::decrementRefcount(SkBitmap* bitmapResource) { decrementRefcount((void*) bitmapResource); } +void ResourceCache::decrementRefcount(SkPath* pathResource) { + decrementRefcount((void*) pathResource); +} + void ResourceCache::decrementRefcount(SkiaShader* shaderResource) { shaderResource->getSkShader()->safeUnref(); decrementRefcount((void*) shaderResource); @@ -122,6 +130,24 @@ void ResourceCache::recycle(SkBitmap* resource) { } } +void ResourceCache::destructor(SkPath* resource) { + Mutex::Autolock _l(mLock); + ResourceReference* ref = mCache->indexOfKey(resource) >= 0 ? mCache->valueFor(resource) : NULL; + if (ref == NULL) { + // If we're not tracking this resource, just delete it + if (Caches::hasInstance()) { + Caches::getInstance().pathCache.removeDeferred(resource); + } + delete resource; + return; + } + ref->destroyed = true; + if (ref->refCount == 0) { + deleteResourceReference(resource, ref); + return; + } +} + void ResourceCache::destructor(SkBitmap* resource) { Mutex::Autolock _l(mLock); ResourceReference* ref = mCache->indexOfKey(resource) >= 0 ? mCache->valueFor(resource) : NULL; @@ -192,6 +218,15 @@ void ResourceCache::deleteResourceReference(void* resource, ResourceReference* r delete bitmap; } break; + case kPath: + { + SkPath* path = (SkPath*)resource; + if (Caches::hasInstance()) { + Caches::getInstance().pathCache.removeDeferred(path); + } + delete path; + } + break; case kShader: { SkiaShader* shader = (SkiaShader*)resource; diff --git a/libs/hwui/ResourceCache.h b/libs/hwui/ResourceCache.h index 1bb4390f82d9e..2a38910951c44 100644 --- a/libs/hwui/ResourceCache.h +++ b/libs/hwui/ResourceCache.h @@ -32,6 +32,7 @@ enum ResourceType { kBitmap, kShader, kColorFilter, + kPath, }; class ResourceReference { @@ -53,15 +54,18 @@ class ResourceCache { public: ResourceCache(); ~ResourceCache(); + void incrementRefcount(SkPath* resource); void incrementRefcount(SkBitmap* resource); void incrementRefcount(SkiaShader* resource); void incrementRefcount(SkiaColorFilter* resource); void incrementRefcount(const void* resource, ResourceType resourceType); void decrementRefcount(void* resource); void decrementRefcount(SkBitmap* resource); + void decrementRefcount(SkPath* resource); void decrementRefcount(SkiaShader* resource); void decrementRefcount(SkiaColorFilter* resource); void recycle(SkBitmap* resource); + void destructor(SkPath* resource); void destructor(SkBitmap* resource); void destructor(SkiaShader* resource); void destructor(SkiaColorFilter* resource);