1176124 - Add placeholders in the SurfaceCache to track when we've started decoding a frame, even if we haven't allocated it yet

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a MatchType enum to LookupResult to let Lookup*() return more detailed information

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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add placeholder support to the SurfaceCache

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.

Comment 3

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ef48e3692e3

Comment 4

[Seth Fowler [:seth] [:s2h]]

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add placeholder support to the SurfaceCache

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.

Comment 6

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62962ca7b4f4

Comment 7

[Seth Fowler [:seth] [:s2h]]

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

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add placeholder support to the SurfaceCache

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 9

[Daniel Holbert [:dholbert]]

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 10

[Daniel Holbert [:dholbert]]

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 11

[Daniel Holbert [:dholbert]]

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?

Comment 12

[Seth Fowler [:seth] [:s2h]]

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.

Comment 13

[Daniel Holbert [:dholbert]]

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 [...]"

Comment 14

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a MatchType enum to LookupResult to let Lookup*() return more detailed information

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.)

Comment 15

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add placeholder support to the SurfaceCache

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

Comment 16

[Daniel Holbert [:dholbert]]

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?)

Comment 17

[Seth Fowler [:seth] [:s2h]]

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!

Comment 18

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add placeholder support to the SurfaceCache so we can avoid launching redundant decoders

Here's the updated patch.

Comment 19

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59b5cbadc6d2

Comment 20

[Seth Fowler [:seth] [:s2h]]

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

Comment 21

[Carsten Book [:Tomcat]]

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