From d7a0b010aeabf7ec1d021fe44bf6f8422e2da4cd Mon Sep 17 00:00:00 2001 From: Timothy Nikkel Date: Mon, 15 Aug 2016 19:58:35 -0500 Subject: [PATCH] Backed out changeset 37340346a89e (Bug 1289628 - Return ISurfaceProvider objects from SurfaceCache lookup functions. r=dholbert,edwin) for causing bug 1292290. --- image/FrameAnimator.cpp | 23 ++--------- image/LookupResult.h | 30 +++++++------- image/RasterImage.cpp | 23 +++-------- image/SurfaceCache.cpp | 86 ++++++++++++++++++++--------------------- image/SurfaceCache.h | 39 ++++++++++--------- image/VectorImage.cpp | 57 +++++++++------------------ image/VectorImage.h | 6 +-- 7 files changed, 106 insertions(+), 158 deletions(-) diff --git a/image/FrameAnimator.cpp b/image/FrameAnimator.cpp index be89df75a4cd..223dacf99525 100644 --- a/image/FrameAnimator.cpp +++ b/image/FrameAnimator.cpp @@ -7,7 +7,6 @@ #include "mozilla/MemoryReporting.h" #include "mozilla/Move.h" -#include "mozilla/NotNull.h" #include "imgIContainer.h" #include "LookupResult.h" #include "MainThreadUtils.h" @@ -302,9 +301,7 @@ FrameAnimator::GetCompositedFrame(uint32_t aFrameNum) // If we have a composited version of this frame, return that. if (mLastCompositedFrameIndex == int32_t(aFrameNum)) { - RefPtr provider = - new SimpleSurfaceProvider(WrapNotNull(mCompositingFrame.get())); - return LookupResult(Move(provider), MatchType::EXACT); + return LookupResult(mCompositingFrame->DrawableRef(), MatchType::EXACT); } // Otherwise return the raw frame. DoBlend is required to ensure that we only @@ -314,9 +311,7 @@ FrameAnimator::GetCompositedFrame(uint32_t aFrameNum) RasterSurfaceKey(mSize, DefaultSurfaceFlags(), aFrameNum)); - MOZ_ASSERT(!result || - !result.Provider()->DrawableRef() || - !result.Provider()->DrawableRef()->GetIsPaletted(), + MOZ_ASSERT(!result || !result.DrawableRef()->GetIsPaletted(), "About to return a paletted frame"); return result; } @@ -386,18 +381,8 @@ FrameAnimator::GetRawFrame(uint32_t aFrameNum) const RasterSurfaceKey(mSize, DefaultSurfaceFlags(), aFrameNum)); - if (!result) { - return RawAccessFrameRef(); - } - - DrawableFrameRef drawableRef = result.Provider()->DrawableRef(); - if (!drawableRef) { - MOZ_ASSERT_UNREACHABLE("Animated image frames should be locked and " - "in-memory; how did we lose one?"); - return RawAccessFrameRef(); - } - - return drawableRef->RawAccessRef(); + return result ? result.DrawableRef()->RawAccessRef() + : RawAccessFrameRef(); } //****************************************************************************** diff --git a/image/LookupResult.h b/image/LookupResult.h index 3123f2dcf77b..67aebf576fe9 100644 --- a/image/LookupResult.h +++ b/image/LookupResult.h @@ -13,7 +13,7 @@ #include "mozilla/Attributes.h" #include "mozilla/Move.h" -#include "ISurfaceProvider.h" +#include "imgFrame.h" namespace mozilla { namespace image { @@ -30,7 +30,7 @@ enum class MatchType : uint8_t /** * LookupResult is the return type of SurfaceCache's Lookup*() functions. It - * combines an ISurfaceProvider with relevant metadata tracked by SurfaceCache. + * combines a surface with relevant metadata tracked by SurfaceCache. */ class MOZ_STACK_CLASS LookupResult { @@ -44,35 +44,35 @@ public: } LookupResult(LookupResult&& aOther) - : mProvider(Move(aOther.mProvider)) + : mDrawableRef(Move(aOther.mDrawableRef)) , mMatchType(aOther.mMatchType) { } - LookupResult(RefPtr&& aProvider, MatchType aMatchType) - : mProvider(Move(aProvider)) + LookupResult(DrawableFrameRef&& aDrawableRef, MatchType aMatchType) + : mDrawableRef(Move(aDrawableRef)) , mMatchType(aMatchType) { - MOZ_ASSERT(!mProvider || !(mMatchType == MatchType::NOT_FOUND || - mMatchType == MatchType::PENDING), + MOZ_ASSERT(!mDrawableRef || !(mMatchType == MatchType::NOT_FOUND || + mMatchType == MatchType::PENDING), "Only NOT_FOUND or PENDING make sense with no surface"); - MOZ_ASSERT(mProvider || mMatchType == MatchType::NOT_FOUND || - mMatchType == MatchType::PENDING, + MOZ_ASSERT(mDrawableRef || mMatchType == MatchType::NOT_FOUND || + mMatchType == MatchType::PENDING, "NOT_FOUND or PENDING do not make sense with a surface"); } LookupResult& operator=(LookupResult&& aOther) { MOZ_ASSERT(&aOther != this, "Self-move-assignment is not supported"); - mProvider = Move(aOther.mProvider); + mDrawableRef = Move(aOther.mDrawableRef); mMatchType = aOther.mMatchType; return *this; } - ISurfaceProvider* Provider() { return mProvider.get(); } - const ISurfaceProvider* Provider() const { return mProvider.get(); } + DrawableFrameRef& DrawableRef() { return mDrawableRef; } + const DrawableFrameRef& DrawableRef() const { return mDrawableRef; } - /// @return true if this LookupResult contains a surface provider. - explicit operator bool() const { return bool(mProvider); } + /// @return true if this LookupResult contains a surface. + explicit operator bool() const { return bool(mDrawableRef); } /// @return what kind of match this is (exact, substitute, etc.) MatchType Type() const { return mMatchType; } @@ -80,7 +80,7 @@ public: private: LookupResult(const LookupResult&) = delete; - RefPtr mProvider; + DrawableFrameRef mDrawableRef; MatchType mMatchType; }; diff --git a/image/RasterImage.cpp b/image/RasterImage.cpp index 3ecd85938dcf..7119bc64cb89 100644 --- a/image/RasterImage.cpp +++ b/image/RasterImage.cpp @@ -354,29 +354,18 @@ RasterImage::LookupFrame(uint32_t aFrameNum, return DrawableFrameRef(); } - // OK, we've got an ISurfaceProvider. Ask it to give us a surface containing - // the frame we're looking for. (This could be computationally expensive in - // some cases.) - DrawableFrameRef drawableRef = result.Provider()->DrawableRef(); - if (!drawableRef) { - // An ISurfaceProvider will only fail to give us a surface in catastrophic - // cases such as the operating system freeing our volatile buffers. Our best - // bet is to throw everything out and attempt to recover. - RecoverFromInvalidFrames(aSize, aFlags); + if (result.DrawableRef()->GetCompositingFailed()) { return DrawableFrameRef(); } - if (drawableRef->GetCompositingFailed()) { - return DrawableFrameRef(); - } - - MOZ_ASSERT(!drawableRef->GetIsPaletted(), "Should not have a paletted frame"); + MOZ_ASSERT(!result.DrawableRef()->GetIsPaletted(), + "Should not have a paletted frame"); // Sync decoding guarantees that we got the frame, but if it's owned by an // async decoder that's currently running, the contents of the frame may not // be available yet. Make sure we get everything. if (mHasSourceData && (aFlags & FLAG_SYNC_DECODE)) { - drawableRef->WaitUntilFinished(); + result.DrawableRef()->WaitUntilFinished(); } // If we could have done some decoding in this function we need to check if @@ -384,11 +373,11 @@ RasterImage::LookupFrame(uint32_t aFrameNum, // to avoid calling IsAborted if we weren't passed any sync decode flag because // IsAborted acquires the monitor for the imgFrame. if (aFlags & (FLAG_SYNC_DECODE | FLAG_SYNC_DECODE_IF_FAST) && - drawableRef->IsAborted()) { + result.DrawableRef()->IsAborted()) { return DrawableFrameRef(); } - return Move(drawableRef); + return Move(result.DrawableRef()); } uint32_t diff --git a/image/SurfaceCache.cpp b/image/SurfaceCache.cpp index 69c124ef75e8..ee3475708773 100644 --- a/image/SurfaceCache.cpp +++ b/image/SurfaceCache.cpp @@ -55,7 +55,6 @@ class SurfaceCacheImpl; // The single surface cache instance. static StaticRefPtr sInstance; - /////////////////////////////////////////////////////////////////////////////// // SurfaceCache Implementation /////////////////////////////////////////////////////////////////////////////// @@ -147,17 +146,14 @@ public: MOZ_ASSERT(mImageKey, "Must have a valid image key"); } - already_AddRefed Provider() const + DrawableFrameRef DrawableRef() const { if (MOZ_UNLIKELY(IsPlaceholder())) { - MOZ_ASSERT_UNREACHABLE("Shouldn't call Provider() on a placeholder"); - return nullptr; + MOZ_ASSERT_UNREACHABLE("Shouldn't call DrawableRef() on a placeholder"); + return DrawableFrameRef(); } - MOZ_ASSERT(mProvider); - - RefPtr provider = mProvider; - return provider.forget(); + return mProvider->DrawableRef(); } void SetLocked(bool aLocked) @@ -202,30 +198,18 @@ public: SurfaceMemoryCounter counter(aCachedSurface->GetSurfaceKey(), aCachedSurface->IsLocked()); - if (aCachedSurface->IsPlaceholder()) { - mCounters.AppendElement(counter); - return; - } + if (!aCachedSurface->IsPlaceholder()) { + DrawableFrameRef surfaceRef = aCachedSurface->DrawableRef(); + if (surfaceRef) { + counter.SubframeSize() = Some(surfaceRef->GetSize()); - RefPtr provider = aCachedSurface->Provider(); - if (!provider) { - MOZ_ASSERT_UNREACHABLE("Not a placeholder, but no ISurfaceProvider?"); - mCounters.AppendElement(counter); - return; + size_t heap = 0, nonHeap = 0; + surfaceRef->AddSizeOfExcludingThis(mMallocSizeOf, heap, nonHeap); + counter.Values().SetDecodedHeap(heap); + counter.Values().SetDecodedNonHeap(nonHeap); + } } - DrawableFrameRef surfaceRef = provider->DrawableRef(); - if (!surfaceRef) { - mCounters.AppendElement(counter); - return; - } - - counter.SubframeSize() = Some(surfaceRef->GetSize()); - size_t heap = 0, nonHeap = 0; - surfaceRef->AddSizeOfExcludingThis(mMallocSizeOf, heap, nonHeap); - counter.Values().SetDecodedHeap(heap); - counter.Values().SetDecodedNonHeap(nonHeap); - mCounters.AppendElement(counter); } @@ -237,6 +221,7 @@ public: private: nsExpirationState mExpirationState; RefPtr mProvider; + DrawableFrameRef mDrawableRef; const Cost mCost; const ImageKey mImageKey; const SurfaceKey mSurfaceKey; @@ -614,9 +599,10 @@ public: return LookupResult(MatchType::PENDING); } - RefPtr provider = surface->Provider(); - if (!provider) { - MOZ_ASSERT_UNREACHABLE("Not a placeholder, but no ISurfaceProvider?"); + DrawableFrameRef ref = surface->DrawableRef(); + if (!ref) { + // The surface was released by the operating system. Remove the cache + // entry as well. Remove(surface); return LookupResult(MatchType::NOT_FOUND); } @@ -627,7 +613,7 @@ public: MOZ_ASSERT(surface->GetSurfaceKey() == aSurfaceKey, "Lookup() not returning an exact match?"); - return LookupResult(Move(provider), MatchType::EXACT); + return LookupResult(Move(ref), MatchType::EXACT); } LookupResult LookupBestMatch(const ImageKey aImageKey, @@ -639,18 +625,30 @@ public: return LookupResult(MatchType::NOT_FOUND); } - RefPtr surface; - MatchType matchType = MatchType::NOT_FOUND; - Tie(surface, matchType) = cache->LookupBestMatch(aSurfaceKey); - if (!surface) { - return LookupResult(matchType); // Lookup in the per-image cache missed. - } + // Repeatedly look up the best match, trying again if the resulting surface + // has been freed by the operating system, until we can either lock a + // surface for drawing or there are no matching surfaces left. + // XXX(seth): This is O(N^2), but N is expected to be very small. If we + // encounter a performance problem here we can revisit this. - RefPtr provider = surface->Provider(); - if (!provider) { - MOZ_ASSERT_UNREACHABLE("Not a placeholder, but no ISurfaceProvider?"); + RefPtr surface; + DrawableFrameRef ref; + MatchType matchType = MatchType::NOT_FOUND; + while (true) { + Tie(surface, matchType) = cache->LookupBestMatch(aSurfaceKey); + + if (!surface) { + return LookupResult(matchType); // Lookup in the per-image cache missed. + } + + ref = surface->DrawableRef(); + if (ref) { + break; + } + + // The surface was released by the operating system. Remove the cache + // entry as well. Remove(surface); - return LookupResult(MatchType::NOT_FOUND); } MOZ_ASSERT_IF(matchType == MatchType::EXACT, @@ -665,7 +663,7 @@ public: MarkUsed(surface, cache); } - return LookupResult(Move(provider), matchType); + return LookupResult(Move(ref), matchType); } bool CanHold(const Cost aCost) const diff --git a/image/SurfaceCache.h b/image/SurfaceCache.h index 1c6e0b6ae5dd..021c382186cd 100644 --- a/image/SurfaceCache.h +++ b/image/SurfaceCache.h @@ -194,33 +194,34 @@ struct SurfaceCache static void Shutdown(); /** - * Looks up and returns the requested cache entry. + * Looks up the requested cache entry and returns a drawable reference to its + * associated surface. * * If the image associated with the cache entry is locked, then the entry will * be locked before it is returned. * - * This function returns an ISurfaceProvider, but it does not guarantee that - * the ISurfaceProvider can give the caller a drawable surface. Lookup() - * callers should check that ISurfaceProvider::DrawableRef() returns a - * non-empty value; if not, some sort of serious failure has occurred, and the - * best bet is to remove all existing surfaces for the image from the cache - * using RemoveImage() and try to recover. The most likely cause for this kind - * of failure is the surface's volatile buffer being freed by the operating - * system due to extreme memory pressure. + * If a matching ISurfaceProvider was found in the cache, but SurfaceCache + * couldn't obtain a surface from it (e.g. because it had stored its surface + * in a volatile buffer which was discarded by the OS) then it is + * automatically removed from the cache and an empty LookupResult is returned. + * Note that this will never happen to ISurfaceProviders associated with a + * locked image; SurfaceCache tells such ISurfaceProviders to keep a strong + * references to their data internally. * * @param aImageKey Key data identifying which image the cache entry * belongs to. * @param aSurfaceKey Key data which uniquely identifies the requested * cache entry. - * @return a LookupResult which will contain an ISurfaceProvider - * for the requested surface if a matching cache entry - * was found. + * @return a LookupResult, which will either contain a + * DrawableFrameRef to a surface, or an empty + * DrawableFrameRef if the cache entry was not found. */ static LookupResult Lookup(const ImageKey aImageKey, const SurfaceKey& aSurfaceKey); /** - * Looks up and returns the best matching cache entry. + * Looks up the best matching cache entry and returns a drawable reference to + * its associated surface. * * The result may vary from the requested cache entry only in terms of size. * @@ -228,12 +229,12 @@ struct SurfaceCache * belongs to. * @param aSurfaceKey Key data which uniquely identifies the requested * cache entry. - * @return a LookupResult which will contain either an - * ISurfaceProvider for a surface similar to the one - * the caller requested, or no ISurfaceProvider if no - * acceptable match was found. Callers can use - * LookupResult::IsExactMatch() to check whether the - * returned ISurfaceProvider exactly matches + * @return a LookupResult, which will either contain a + * DrawableFrameRef to a surface similar to the + * the one the caller requested, or an empty + * DrawableFrameRef if no acceptable match was found. + * Callers can use LookupResult::IsExactMatch() to check + * whether the returned surface exactly matches * @aSurfaceKey. */ static LookupResult LookupBestMatch(const ImageKey aImageKey, diff --git a/image/VectorImage.cpp b/image/VectorImage.cpp index d7663223c694..5c6db4722b26 100644 --- a/image/VectorImage.cpp +++ b/image/VectorImage.cpp @@ -859,55 +859,34 @@ VectorImage::Draw(gfxContext* aContext, SVGDrawingParameters params(aContext, aSize, aRegion, aSamplingFilter, svgContext, animTime, aFlags); - // If we have an prerasterized version of this image that matches the - // drawing parameters, use that. - RefPtr svgDrawable = LookupCachedSurface(params); - if (svgDrawable) { - Show(svgDrawable, params); + if (aFlags & FLAG_BYPASS_SURFACE_CACHE) { + CreateSurfaceAndShow(params); return DrawResult::SUCCESS; } - // We didn't get a hit in the surface cache, so we'll need to rerasterize. - CreateSurfaceAndShow(params); - return DrawResult::SUCCESS; -} - -already_AddRefed -VectorImage::LookupCachedSurface(const SVGDrawingParameters& aParams) -{ - // If we're not allowed to use a cached surface, don't attempt a lookup. - if (aParams.flags & FLAG_BYPASS_SURFACE_CACHE) { - return nullptr; - } - LookupResult result = SurfaceCache::Lookup(ImageKey(this), - VectorSurfaceKey(aParams.size, - aParams.svgContext, - aParams.animationTime)); - if (!result) { - return nullptr; // No matching surface. - } + VectorSurfaceKey(params.size, + params.svgContext, + params.animationTime)); - DrawableFrameRef drawableRef = result.Provider()->DrawableRef(); - if (!drawableRef) { - // Something went wrong. (Probably the OS freeing our volatile buffer due to - // low memory.) Attempt to recover. + // Draw. + if (result) { + RefPtr surface = result.DrawableRef()->GetSurface(); + if (surface) { + RefPtr svgDrawable = + new gfxSurfaceDrawable(surface, result.DrawableRef()->GetSize()); + Show(svgDrawable, params); + return DrawResult::SUCCESS; + } + + // We lost our surface due to some catastrophic event. RecoverFromLossOfSurfaces(); - return nullptr; } - RefPtr surface = drawableRef->GetSurface(); - if (!surface) { - // Something went wrong. (Probably a GPU driver crash or device reset.) - // Attempt to recover. - RecoverFromLossOfSurfaces(); - return nullptr; - } + CreateSurfaceAndShow(params); - RefPtr svgDrawable = - new gfxSurfaceDrawable(surface, drawableRef->GetSize()); - return svgDrawable.forget(); + return DrawResult::SUCCESS; } void diff --git a/image/VectorImage.h b/image/VectorImage.h index 3753785975c5..3bbe4a813bb0 100644 --- a/image/VectorImage.h +++ b/image/VectorImage.h @@ -78,14 +78,10 @@ protected: virtual nsresult StopAnimation() override; virtual bool ShouldAnimate() override; -private: - /// Attempt to find a cached surface matching @aParams in the SurfaceCache. - already_AddRefed - LookupCachedSurface(const SVGDrawingParameters& aParams); - void CreateSurfaceAndShow(const SVGDrawingParameters& aParams); void Show(gfxDrawable* aDrawable, const SVGDrawingParameters& aParams); +private: nsresult Init(const char* aMimeType, uint32_t aFlags); /**