Part 1 adds a new MatchType enum that's used by LookupResult to provide more granular information than just "was this an exact match or not?". Some of the values in the enum are not used yet, but they'll be used in part 2. MatchType provides more information than is actually necessary - callers of Insert*() just want to know "Did I get a surface, and if not, do I need to start a new decoder?" - but when I pare the information down to just the bare minimum necessary, it becomes harder to follow the code and harder to write asserts in part 2. These names are easier to understand than alternatives I've tried, and there's no substantial cost to providing this level of detail, so in the interest of readability, I think we should do so. One oddity is that the term "PENDING" is used when we encounter a placeholder. Why not just reword these enum values to use the word "PLACEHOLDER"? Well, there are other ways in which a surface could be in the "PENDING" state. Specifically, in part 2 we also return SUBSTITUTE_BECAUSE_PENDING if we found an exactly matching surface, but it isn't yet fully decoded. This has nothing to do with placeholders, so "PENDING" seems like a usefully more general term. It also makes it easier to name the enum values.

Part 2 actually adds placeholder support. The way that placeholders work should be explained fairly well in the comments to SurfaceCache::InsertPlaceholder(), but I'll summarize them here as well. A placeholder is just a special type of CachedSurface - specifically, it's a CachedSurface with a null mSurface member. That means that in almost every respect, placeholders behave just like regular surfaces, and we don't need any special methods to interact with them. There are a few exceptions: (1) Inserting placeholders is accomplished with a special function, InsertPlaceholder(), rather than by reusing Insert(). This was done partially for readability and partially so that we can continue to assert that no null surfaces are inserted via the Insert() codepath. (2) Placeholders are never returned from Lookup*() methods. Indeed, it wouldn't be possible to do so, because those methods return DrawableFrameRefs rather than CachedSurface objects, and DrawableFrameRefs can't exist without a 'real' surface to back them. Instead, Lookup*() methods notice the presence of placeholders and encode that information in the MatchType they return. Note that in the case of LookupBestMatch(), we have to decide on a MatchType in ImageCache::LookupBestMatch() rather than SurfaceCacheImpl::LookupBestMatch(), because only in the ImageCache code do we know whether we encountered a placeholder or not. (3) Placeholders are implicitly removed when you insert a new surface with the same ImageKey and SurfaceKey. This is handled in SurfaceCacheImpl::Insert(). It's pretty straightforward - if we ran into a placeholder when checking for a duplicate surface, we remove the placeholder before continuing. Unfortunately, this means that we do more surface lookups than we really need to. As noted in the comments in the code, I intend to fix this in a followup. Indeed, I've already got the patch mostly ready to go, but it seems a bit complex to land as part of this bug (which is already complex enough) and risk confounding regression hunting. You'll see a comment about std::tie in this patch. Painfully, this is yet another thing we can't use due to stlport not including it. Either Botond or I will implement mozilla::Tie sometime soon, and I'll come back and update this code to use it when Tie is available. Having Tie will make the code involving Pair look much prettier.

I expect this to fix bug 1182951 as well, but due to Bugzilla circular dependency shenanigans I can't add this bug as a blocker of that bug.

In this updated version of part 2, all that has changed is the |if| condition in RasterImage.cpp that determines whether or not we start a decoder. In the previous version of part 2, we'd always start a decoder if LookupFrameInternal() didn't return any surface at all (i.e., if |!result|). That's wrong, though: unless we're sync decoding, |!result| shouldn't be enough to make us start a decoder. Outside of the sync decoding case, we should only start a decoder if there's no surface or placeholder that matched our lookup - in other words, if |result.Type()| is either NOT_FOUND or SUBSTITUTE_BECAUSE_NOT_FOUND. If there was a placeholder that matched our lookup, we'd still end up with |!result| (and |result.Type()| would be PENDING), but we shouldn't start a decoder just because of that.

Wow, that's the greenest try job I've ever seen. Shouldn't have any problems landing this one.

One more minor tweak, which I meant to include in the previous tweak. This updated version of the patch checks the return value of SurfaceCache::InsertPlaceholder in RasterImage::CreateDecoder.

Comment on attachment 8634526 [details] [diff] [review] (Part 1) - Add a MatchType enum to LookupResult to let Lookup*() return more detailed information Review of attachment 8634526 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 1 -- just one nit: ::: image/LookupResult.h @@ +53,5 @@ > + { > + MOZ_ASSERT(!mDrawableRef || !(mMatchType == MatchType::NOT_FOUND || > + mMatchType == MatchType::PENDING)); > + MOZ_ASSERT(mDrawableRef || mMatchType == MatchType::NOT_FOUND || > + mMatchType == MatchType::PENDING); I think these assertions can be combined into one, and ideally we should have an assertion-message for any non-100%-trivial asserted conditions, to reduce ambiguity for future code-archeologists/developers. Maybe replace these two assertions with this one: MOZ_ASSERT(mDrawableRef != (mMatchType == MatchType::NOT_FOUND || mMatchType == MatchType::PENDING), "member-vars should agree on whether we found a surface"); (Or if you'd prefer to leave them separate, please still add a variant of that^ message either as a comment or as assertion-message.)

Comment on attachment 8635334 [details] [diff] [review] (Part 2) - Add placeholder support to the SurfaceCache Review of attachment 8635334 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/SurfaceCache.cpp @@ +138,5 @@ > , mSurfaceKey(aSurfaceKey) > , mLifetime(aLifetime) > { > + MOZ_ASSERT(mSurface || (mCost == 1 && mLifetime == Lifetime::Transient), > + "Placeholder should have trivial cost and transient lifetime"); Consider replacing mSurface with !IsPlaceholder() here, to make the asserted condition more human-readable. (and to make the mapping more between the assertion-message and the checked condition clearer). @@ +155,5 @@ > > void SetLocked(bool aLocked) > { > + if (IsPlaceholder()) { > + return; // Can't lock a placeholder. Does we actually hit this early-return? (Should this be MOZ_ASSERT_UNREACHABLE?) @@ +282,1 @@ > LookupBestMatch(const SurfaceKey& aSurfaceKey, Please declare this as MOZ_MUST_USE, to be sure (at compile time, via static analysis) that we don't drop this return value (and its already_AddRefed) on the floor & cause a leak as a result. (:mystor implemented some smarts to make this happen automagically for methods that directly return already_AddRefed, but now that we're returning a struct that *wraps* an already_AddRefed, we won't get that automagic anymore.) @@ +304,5 @@ > + ? MatchType::NOT_FOUND > + : MatchType::PENDING; > + } else if (exactMatch && matchContext.mBestMatch == exactMatch) { > + // An exact match is still decoding, but it's the best we've got. > + return MakePair(exactMatch.forget(), MatchType::EXACT); Two things here. FIRSTLY: The "matchType" variable in this function feels a bit clumsy. (We set it right away to a guessed value, and then maybe we return early & don't use it; and then we set it to something else, and then maybe set it a third time when we find out that the second value wasn't right either. And maybe we take the second early-return and disregard the updated value.) I think we clean this a bit by: - Dropping all matchType stuff before your ForEach call. - Replacing all the code after ForEach with the following: ==== MatchType matchType; if (matchContext.mBestMatch) { if (!exactMatch) { // Found a substitute, and no exact match. matchType = MatchType::SUBSTITUTE_BECAUSE_NOT_FOUND; } else if (exactMatch != matchContext.mBestMatch) { // An exact match is still decoding, & we found a temporary substitute. matchType = MatchType::SUBSTITUTE_BECAUSE_PENDING; } else { // An exact match is still decoding, and it's the best we've got. matchType = MatchType::EXACT; } } else { // Couldn't find a substitute: matchType = exactMatch ? MatchType::NOT_FOUND : MatchType::PENDING; } return MakePair(matchContext.mBestMatch.forget(), matchType); } ==== This way we set matchType exactly once, and in one localized place. This also drops your middle "return MakePair(exactMatch.forget(), MatchType::EXACT)" return-statement, since we can just share the final return statement in that scenario. SECONDLY: Supposing we find an exactMatch which is decoding -- I don't understand the difference between the "matchContext.mBestMatch == exactMatch" scenario vs. the "null matchContext.mBestMatch" scenario. If we have a decoding exactMatch which is currently decoding, why would our ForEach(TryToImproveMatch...) call sometimes return it & sometimes return null?

Comment on attachment 8635334 [details] [diff] [review] (Part 2) - Add placeholder support to the SurfaceCache Review of attachment 8635334 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/RasterImage.cpp @@ +527,5 @@ > } > > + if (result.Type() == MatchType::NOT_FOUND || > + result.Type() == MatchType::SUBSTITUTE_BECAUSE_NOT_FOUND || > + ((aFlags & FLAG_SYNC_DECODE) && !result)) { The comment below this condition ("We don't have a copy of this frame, and there's no decoder working on one") matches the first two things we're checking here, but it's not true if we enter this clause due to the 3rd (new) SYNC_DECODE condition. Please extend the comment to cover the SYNC_DECODE scenario, too. @@ +1503,5 @@ > } > > + // Add a placeholder for the first frame to the SurfaceCache so we won't > + // trigger any more decoders with the same parameters. > + if (aSize) { Why do we only do this if aSize is truthy? (Above this, it looks like we create a decoder regardless of whether aSize was truthy. Will the lack-of-a-placeholder let us trigger multiple decoders, from multiple falsey-aSize calls to this method? and is that bad?) (I'm partly confused because I don't immediately understand the significance of aSize in this function. The documentation says it's provided if we're doing "a size decode", but I'm not sure what that means or what a non-size decode would be.) ::: image/SurfaceCache.cpp @@ +462,5 @@ > Lifetime aLifetime) > { > // If this is a duplicate surface, refuse to replace the original. > + // XXX(seth): Calling Lookup() and then RemoveSurface() does the lookup > + // twice. We'll make this more efficient in a followup to bug 1176124. Consider filing the followup now, so you can use its actual bug number here. @@ +469,5 @@ > return InsertOutcome::FAILURE_ALREADY_PRESENT; > } > > + MOZ_ASSERT(result.Type() == MatchType::PENDING || > + result.Type() == MatchType::NOT_FOUND); An assertion-message here would be helpful, to explain how we know this condition should hold. Maybe something like: "LookupResult constructors should enforce that falsey instances (w/ no surface) are either PENDING or NOT_FOUND" @@ +1109,5 @@ > + } > + > + MutexAutoLock lock(sInstance->GetMutex()); > + Cost cost = 1; // Placeholders have a trivial cost. > + return sInstance->Insert(nullptr, cost, aImageKey, aSurfaceKey, Rather than using 1 as a magic number here (and checking for this same magic-number-1 in the "Placeholder should have trivial cost" assertion elsewhere), I'd prefer we added e.g.... #define PLACEHOLDER_COST 1 ...(or a static const variable), and use that instead of "1" in both locations. (This will let us drop the "Cost cost = 1;" line here, too, making this marginally shorter & clearer.) Also: out of curiosity, why do we bother giving them a cost of even 1, rather than just 0? Do we require that costs must be positive?

Thanks for the review! Some responses below: (In reply to Daniel Holbert [:dholbert] from comment #10) > @@ +155,5 @@ > > > > void SetLocked(bool aLocked) > > { > > + if (IsPlaceholder()) { > > + return; // Can't lock a placeholder. > > Does we actually hit this early-return? (Should this be > MOZ_ASSERT_UNREACHABLE?) Yeah, we should be able to. For most of the code, a placeholder is just a surface. > I think we clean this a bit by: > - Dropping all matchType stuff before your ForEach call. > - Replacing all the code after ForEach with the following: That sounds good to me; I 'll try it out. > SECONDLY: > Supposing we find an exactMatch which is decoding -- I don't understand the > difference between the "matchContext.mBestMatch == exactMatch" scenario vs. > the "null matchContext.mBestMatch" scenario. If we have a decoding > exactMatch which is currently decoding, why would our > ForEach(TryToImproveMatch...) call sometimes return it & sometimes return > null? If we have an exact match which is currently decoding, TryToImproveMatch would never return null. It would always return the decoding exact match if it couldn't find anything better. But TryToImproveMatch *can* return null if there are absolutely no surfaces that could be substituted for the requested surface - say, if we don't have any live surfaces for the image at all, or if they all have the wrong decode flags. (In reply to Daniel Holbert [:dholbert] from comment #11) > Why do we only do this if aSize is truthy? > > (Above this, it looks like we create a decoder regardless of whether aSize > was truthy. Will the lack-of-a-placeholder let us trigger multiple decoders, > from multiple falsey-aSize calls to this method? and is that bad?) > > (I'm partly confused because I don't immediately understand the significance > of aSize in this function. The documentation says it's provided if we're > doing "a size decode", but I'm not sure what that means or what a non-size > decode would be.) This *does* make sense, but it's confusing because of bad API design. =) A "size decode" is a decode pass that only decodes the image's headers. The name is terrible; it's called that because its main purpose is to determine the size of the image, since we can't fire the load event until we have it. I'd eventually like to rename it to a "header-only decode", which I think would be more self-explanatory. So a falsy aSize, as the docs say, means that we're doing a size decode. A size decode does not decode any image data or allocate any surfaces or anything like that, so we should not insert a placeholder in that scenario. We'll only ever do one size decode; this is enforced elsewhere in the RasterImage code. (Indeed, there's an assertion about it in CreateDecoder, near the top.) > Also: out of curiosity, why do we bother giving them a cost of even 1, > rather than just 0? Do we require that costs must be positive? Yes. We have various loops that remove surfaces until the currently used cost is zero. After those loops, we assert that we've actually removed all surfaces. Since placeholders are, from the perspective of most of the SurfaceCache code, just normal surfaces, they must have a nonzero cost to avoid triggering those assertions. I'm considering making the placeholder have a cost equal to the cost the surface would have, but since we can't accurately compute that in some cases (specifically, for animated image frames where the actual surfaces may be smaller than the size of the overall image), I haven't yet reached a decision on whether I think that's a good idea. It'd definitely be more appropriate for a followup, in any case.

Thanks, that all makes sense. RE the "if (aSize)" check -- might be worth tweaking the comment slightly to hint at its purpose. (since right now the comment sounds unconditional) Right now, the comment (in part 2) looks like this: >+ // Add a placeholder for the first frame to the SurfaceCache so we won't >+ // trigger any more decoders with the same parameters. >+ if (aSize) { Maybe adjust this to say "If we're actually decoding image data (and not just headers), add a placeholder [...]"

Updated part 1 per review comments. (I continued to use two asserts, just to make it easier to isolate failures, but I added messages for all of the new asserts in LookupResult.h.)

Here's the updated version of part 2. I believe this should address all of the review comments thus far.

Comment on attachment 8635588 [details] [diff] [review] (Part 2) - Add placeholder support to the SurfaceCache Review of attachment 8635588 [details] [diff] [review]: ----------------------------------------------------------------- r=me, nits below: > Bug 1176124 (Part 2) - Add placeholder support to the SurfaceCache. r=dholbert Might be nice to have a bit more detail in the commit message about what "placeholder support" means. (Perhaps add: "to prevent redundant attempts to decode an image"?, or something like that?) ::: image/RasterImage.cpp @@ +1504,5 @@ > } > > + // Add a placeholder for the first frame to the SurfaceCache so we won't > + // trigger any more decoders with the same parameters. > + if (aSize) { (Consider adjusting this comment, per comment 13 -- not sure if you saw that before posting this update.) ::: image/SurfaceCache.cpp @@ +308,5 @@ > + // The exact match is still decoding, but we found a substitute. > + matchType = MatchType::SUBSTITUTE_BECAUSE_PENDING; > + } else { > + // The exact match is still decoding, but it's the best we've got. > + matchType = MatchType::EXACT; Indentation is off on the comment here. @@ +488,5 @@ > + RemoveSurface(aImageKey, aSurfaceKey); > + } > + > + MOZ_ASSERT(result.Type() == MatchType::NOT_FOUND, > + "A LookupResult with no surface should be PENDING or NOT_FOUND"); You dropped the PENDING case from this assertion, but it looks like we still could be PENDING here. Either restore the PENDING check, or put this in an "else" clause chained off of the "if" right above it (which I think might've been what you're going for?)

Thanks for the review! I'll upload an updated version shortly. (In reply to Daniel Holbert [:dholbert] from comment #16) > You dropped the PENDING case from this assertion, but it looks like we still > could be PENDING here. Ack, copy/paste fail!

Here's the updated patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6747b7f35dcb https://hg.mozilla.org/integration/mozilla-inbound/rev/f52a8f3b15ed

https://hg.mozilla.org/mozilla-central/rev/6747b7f35dcb https://hg.mozilla.org/mozilla-central/rev/f52a8f3b15ed