diff --git a/image/src/Decoder.cpp b/image/src/Decoder.cpp index 93b769e194e7..70ef6bee8aeb 100644 --- a/image/src/Decoder.cpp +++ b/image/src/Decoder.cpp @@ -368,13 +368,13 @@ Decoder::InternalAddFrame(uint32_t aFrameNum, return RawAccessFrameRef(); } - bool succeeded = + InsertOutcome outcome = SurfaceCache::Insert(frame, ImageKey(&mImage), RasterSurfaceKey(imageSize.ToIntSize(), aDecodeFlags, aFrameNum), Lifetime::Persistent); - if (!succeeded) { + if (outcome != InsertOutcome::SUCCESS) { ref->Abort(); return RawAccessFrameRef(); } diff --git a/image/src/SurfaceCache.cpp b/image/src/SurfaceCache.cpp index afe2ea1e2120..701febd97907 100644 --- a/image/src/SurfaceCache.cpp +++ b/image/src/SurfaceCache.cpp @@ -304,19 +304,22 @@ public: Mutex& GetMutex() { return mMutex; } - bool Insert(imgFrame* aSurface, - const Cost aCost, - const ImageKey aImageKey, - const SurfaceKey& aSurfaceKey, - Lifetime aLifetime) + InsertOutcome Insert(imgFrame* aSurface, + const Cost aCost, + const ImageKey aImageKey, + const SurfaceKey& aSurfaceKey, + Lifetime aLifetime) { - MOZ_ASSERT(!Lookup(aImageKey, aSurfaceKey), - "Inserting a duplicate surface into the SurfaceCache"); + // If this is a duplicate surface, refuse to replace the original. + if (MOZ_UNLIKELY(Lookup(aImageKey, aSurfaceKey))) { + return InsertOutcome::FAILURE_ALREADY_PRESENT; + } // If this is bigger than we can hold after discarding everything we can, // refuse to cache it. - if (!CanHoldAfterDiscarding(aCost)) - return false; + if (MOZ_UNLIKELY(!CanHoldAfterDiscarding(aCost))) { + return InsertOutcome::FAILURE; + } // Remove elements in order of cost until we can fit this in the cache. Note // that locked surfaces aren't in mCosts, so we never remove them here. @@ -341,7 +344,7 @@ public: if (cache->IsLocked() && aLifetime == Lifetime::Persistent) { surface->SetLocked(true); if (!surface->IsLocked()) { - return false; + return InsertOutcome::FAILURE; } } @@ -350,7 +353,7 @@ public: cache->Insert(aSurfaceKey, surface); StartTracking(surface); - return true; + return InsertOutcome::SUCCESS; } void Remove(CachedSurface* aSurface) @@ -795,14 +798,14 @@ SurfaceCache::Lookup(const ImageKey aImageKey, return sInstance->Lookup(aImageKey, aSurfaceKey); } -/* static */ bool +/* static */ InsertOutcome SurfaceCache::Insert(imgFrame* aSurface, const ImageKey aImageKey, const SurfaceKey& aSurfaceKey, Lifetime aLifetime) { if (!sInstance) { - return false; + return InsertOutcome::FAILURE; } MutexAutoLock lock(sInstance->GetMutex()); diff --git a/image/src/SurfaceCache.h b/image/src/SurfaceCache.h index aadfe1a929fa..895f9bb6b158 100644 --- a/image/src/SurfaceCache.h +++ b/image/src/SurfaceCache.h @@ -114,6 +114,12 @@ MOZ_BEGIN_ENUM_CLASS(Lifetime, uint8_t) Persistent MOZ_END_ENUM_CLASS(Lifetime) +MOZ_BEGIN_ENUM_CLASS(InsertOutcome, uint8_t) + SUCCESS, // Success (but see Insert documentation). + FAILURE, // Couldn't insert (e.g., for capacity reasons). + FAILURE_ALREADY_PRESENT // A surface with the same key is already present. +MOZ_END_ENUM_CLASS(InsertOutcome) + /** * SurfaceCache is an imagelib-global service that allows caching of temporary * surfaces. Surfaces normally expire from the cache automatically if they go @@ -173,9 +179,8 @@ struct SurfaceCache const SurfaceKey& aSurfaceKey); /** - * Insert a surface into the cache. It is an error to call this function - * without first calling Lookup to verify that the surface is not already in - * the cache. + * Insert a surface into the cache. If a surface with the same ImageKey and + * SurfaceKey is already in the cache, Insert returns FAILURE_ALREADY_PRESENT. * * Each surface in the cache has a lifetime, either Transient or Persistent. * Transient surfaces can expire from the cache at any time. Persistent @@ -187,13 +192,14 @@ struct SurfaceCache * LockImage() gets called. * * If a surface cannot be rematerialized, it may be important to know whether - * it was inserted into the cache successfully. Insert() returns false if it + * it was inserted into the cache successfully. Insert() returns FAILURE if it * failed to insert the surface, which could happen because of capacity * reasons, or because it was already freed by the OS. If you aren't inserting * a surface with persistent lifetime, or if the surface isn't associated with - * a locked image, the return value is useless: the surface might expire - * immediately after being inserted, even though Insert() returned true. Thus, - * most callers do not need to check the return value. + * a locked image, checking for SUCCESS or FAILURE is useless: the surface + * might expire immediately after being inserted, even though Insert() + * returned SUCCESS. Thus, many callers do not need to check the result of + * Insert() at all. * * @param aTarget The new surface (wrapped in an imgFrame) to insert into * the cache. @@ -202,14 +208,18 @@ struct SurfaceCache * @param aLifetime Whether this is a transient surface that can always be * allowed to expire, or a persistent surface that * shouldn't expire if the image is locked. - * @return false if the surface could not be inserted. Only check this if - * inserting a persistent surface associated with a locked image (see - * above for more information). + * @return SUCCESS if the surface was inserted successfully. (But see above + * for more information about when you should check this.) + * FAILURE if the surface could not be inserted, e.g. for capacity + * reasons. (But see above for more information about when you + * should check this.) + * FAILURE_ALREADY_PRESENT if a surface with the same ImageKey and + * SurfaceKey already exists in the cache. */ - static bool Insert(imgFrame* aSurface, - const ImageKey aImageKey, - const SurfaceKey& aSurfaceKey, - Lifetime aLifetime); + static InsertOutcome Insert(imgFrame* aSurface, + const ImageKey aImageKey, + const SurfaceKey& aSurfaceKey, + Lifetime aLifetime); /** * Checks if a surface of a given size could possibly be stored in the cache.