1282327 - Store ISurfaceProviders in the ImageLib SurfaceCache instead of storing surfaces directly

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Right now the SurfaceCache stores, well, surfaces.  This is a relatively simple model, but it has some limitations. For animated images, we'd really rather not store every frame - instead, we'd rather compute those frames dynamically when needed. This would drastically reduce the amount of memory required to store animated images, and make it trivial to support discarding them.

Let's take the first step in that direction by modifying the SurfaceCache to store ISurfaceProviders instead of just surfaces. An ISurfaceProvider could contain a surface - and that's what they'll do for now - or it could dynamically generate one.

Comment 1

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

Attachment: Store ISurfaceProviders in the ImageLib SurfaceCache.

Here's the patch.

Note that like the IDecodingTask patch, this adds an additional memory
allocation, since we need to wrap each imgFrame in an ISurfaceProvider. Again,
this additional memory allocation will go away soon; a little bit of
architectural rearrangement is necessary.

Comment 2

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

Attachment: Store ISurfaceProviders in the ImageLib SurfaceCache.

This updated version of the patch adds an IsPlaceholder() method to
ISurfaceProviders. It turns out that that'll be useful down the road.

Comment 3

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

Here's a try job. This is totally green locally, but of course I've only tested it on a small subset of our overall test suite.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b0d725abb8a

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba925dbd233
Store ISurfaceProviders in the ImageLib SurfaceCache. r=dholbert

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5032a30641d3
Revert "Bug 1282327 - Store ISurfaceProviders in the ImageLib SurfaceCache. r=dholbert"; it was accidentally pushed.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8765313 [details] [diff] [review]
Store ISurfaceProviders in the ImageLib SurfaceCache.

Review of attachment 8765313 [details] [diff] [review]:
-----------------------------------------------------------------

One semi-minor general concern - SurfaceCache.h still has a lot of documentation that talks as if the cache entries are "surfaces", when really they're not.  e.g. "Surfaces will never expire", "Removes all cached surfaces", etc.  This is marginally problematic because: for a dynamically-generating ISurfaceProvider that's stored in the cache, it's unclear whether/how this SurfaceCache documentation actually applies.

So: I'd suggest doing a quick pass over all the mentions of "surface" in SurfaceCache.h's comments, and clarifying any that seem to be using "surfaces" to mean "cache entries", and rewording them to clarify.

(This might be good as a followup comment-only patch, if you like.)

::: image/ISurfaceProvider.h
@@ +31,5 @@
> +public:
> +  // Subclasses may or may not be XPCOM classes, so we just require that they
> +  // implement AddRef and Release.
> +  NS_IMETHOD_(MozExternalRefCountType) AddRef(void) = 0;
> +  NS_IMETHOD_(MozExternalRefCountType) Release(void) = 0;

Why the "void" param here, out of curiosity? That seems like a C-ism; couldn't this just be AddRef()/Release() instead of AddRef(void)/Release(void) ?

This API has plenty of other methods that take no arguments, which all are just declared with "()" instead of "(void)".  It looks like we've got AddRef/Release declarations with & without "void" in our codebase; I'll bet the void-flavored ones are just old/crufty/copypasted.

@@ +42,5 @@
> +  /// DrawableFrameRef yet, but it will be able to in the future.
> +  virtual bool IsPlaceholder() const = 0;
> +
> +  /// @return if DrawableRef() will return a completely decoded surface.
> +  virtual bool IsFinished() const = 0;

s/if/true if/

::: image/SurfaceCache.cpp
@@ +193,5 @@
>        SurfaceMemoryCounter counter(aCachedSurface->GetSurfaceKey(),
>                                     aCachedSurface->IsLocked());
>  
> +      DrawableFrameRef surfaceRef = aCachedSurface->DrawableRef();
> +      if (surfaceRef) {

This seems to be changing behavior in a way that might trigger aborts in debug builds.

In particular:
 * The old code had a null-check for aCachedSurface->mSurface, implying that it might be null at this point.
 * This makes me think that maybe aCachedSurface->mProvider (its replacement) could be null here, as well (?)
 * And if mProvider is null, then the new call to aCachedSurface->DrawableRef() here will trip over a MOZ_ASSERT_UNREACHABLE and abort.

So -- is this a problem?  Perhaps we need to resurrect a version of that old null-check? Or, if you actually know that aCachedSurface->mProvider *must* be non-null at this point, you might consider adding a comment or an assertion before the DrawableRef() call, explaining why.  (Or alternately, maybe the DrawableRef() call shouldn't fatally assert anymore?)

(Note that this patch *does* seem to currently allow CachedSurface objects to have null mProvider pointers.  If it mandated that mProvider were non-null, then this would be clearly-OK.)

@@ -1036,5 @@
>  
> -  // Refuse null surfaces.
> -  if (!aSurface) {
> -    gfxDevCrash(LogReason::InvalidCacheSurface) << "Null surface in SurfaceCache::Insert";
> -    return InsertOutcome::FAILURE;

This is reverting the patch for diagnosing bug 1167557.  This makes sense, given that the new incarnation of |aSurface| is NotNull<>, so there's no reason to null-check it.

Should we also move this check/logging up a bit, to caller(s), so that we'll still benefit from it as a diagnostic?  Or, maybe this change is fine, because any bogus NonNull conversions at the callsites will end up causing an explicit abort, which is just as good as the diagnostic?

In any case, it might be worth adding a followup comment on bug 1167557 when this bug's patch lands, to mention that this diagnostic check has been changed/removed. (since it looks like the most recent incarnation of the diagnostic has only been in the tree for a month)

::: image/SurfaceCache.h
@@ +205,5 @@
>    static LookupResult LookupBestMatch(const ImageKey    aImageKey,
>                                        const SurfaceKey& aSurfaceKey);
>  
>    /**
> +   * Insert a SurfaceProvider into the cache. If a surface with the same

Nit: s/SurfaceProvider/ISurfaceProvider/

(Doesn't look like "SurfaceProvider" is an actual defined type; and you use ISurfaceProvider (the actual interface) in other documentation further up.)

::: image/VectorImage.cpp
@@ +944,5 @@
>    }
>  
>    // Attempt to cache the frame.
> +  NotNull<RefPtr<ISurfaceProvider>> provider =
> +    WrapNotNull(new SimpleSurfaceProvider(frame));

This file is missing an #include to provide the SimpleSurfaceProvider definition -- please add:
  #include "ISurfaceProvider.h"

Otherwise, this doesn't compile in non-unified mode.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8765313 [details] [diff] [review]
Store ISurfaceProviders in the ImageLib SurfaceCache.

Review of attachment 8765313 [details] [diff] [review]:
-----------------------------------------------------------------

Marking this r- for now, since I'm not ready to r+ it yet.  But it'll likely be r+ with above comments addressed

Comment 8

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

Thanks for the review!

(In reply to Daniel Holbert [:dholbert] from comment #6)
> This API has plenty of other methods that take no arguments, which all are
> just declared with "()" instead of "(void)".  It looks like we've got
> AddRef/Release declarations with & without "void" in our codebase; I'll bet
> the void-flavored ones are just old/crufty/copypasted.

It's actually taken from the generated header for nsISupports. But I don't mind changing it.

> So -- is this a problem?  Perhaps we need to resurrect a version of that old
> null-check? Or, if you actually know that aCachedSurface->mProvider *must*
> be non-null at this point, you might consider adding a comment or an
> assertion before the DrawableRef() call, explaining why.  (Or alternately,
> maybe the DrawableRef() call shouldn't fatally assert anymore?)
> 
> (Note that this patch *does* seem to currently allow CachedSurface objects
> to have null mProvider pointers.  If it mandated that mProvider were
> non-null, then this would be clearly-OK.)

Yep, |mProvider| can be null for now, as the concept of "placeholders" still exists, and that's how they're implemented. (That concept will be removed pretty soon, though.) Thanks for catching this!

> Should we also move this check/logging up a bit, to caller(s), so that we'll
> still benefit from it as a diagnostic?  Or, maybe this change is fine,
> because any bogus NonNull conversions at the callsites will end up causing
> an explicit abort, which is just as good as the diagnostic?

Yeah, exactly. NotNull takes care of this issue.

Comment 9

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

Attachment: (Part 1) - Store ISurfaceProviders in the ImageLib SurfaceCache.

This updated patch should address the review comments above. The comment updates
have been *greatly* expanded to reflect the new role of ISurfaceProviders, as
requested, and have been moved into part 2.

Comment 10

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

Attachment: (Part 2) - Update SurfaceCache documentation and method names to reflect the fact that cache entries are now ISurfaceProviders.

This patch has all of the comment updates, and there are a lot of them. You
asked for it. =)

While I was at it, I changed the name of a couple of methods that had 'Surface'
in the name.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8767382 [details] [diff] [review]
(Part 1) - Store ISurfaceProviders in the ImageLib SurfaceCache.

Review of attachment 8767382 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with one comment tweak:

::: image/ISurfaceProvider.h
@@ +39,5 @@
> +
> +  /// @return true if this ISurfaceProvider is acting as a placeholder, which is
> +  /// to say that it doesn't have a surface and hence can't return a
> +  /// DrawableFrameRef yet, but it will be able to in the future.
> +  virtual bool IsPlaceholder() const = 0;

Maybe replace
  "can't return a DrawableFrameRef yet"
...with...
  "can't return a useful DrawableFrameRef yet (with something to draw)"

(or something to that effect)

The current language, "can't return a DrawableFrameRef", is a bit confusing. DrawableFrameRef is the simply the *return type* of the DrawableRef() API. So, if the documentation says we "can't return a DrawableFrameRef", it implies that that function never terminates or would simply abort, or something. :) (which does not really seem to be the case -- rather, it just returns a default-constructed DrawableFrameRef -- which *is* (type-wise) a DrawableFrameRef!! :))

Comment 12

[Daniel Holbert [:dholbert]]

... or "a non-empty DrawableFrameRef".  (just notice usages of the term "empty DrawableFrameRef" elsewhere in SurfaceCache.h)

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8767383 [details] [diff] [review]
(Part 2) - Update SurfaceCache documentation and method names to reflect the fact that cache entries are now ISurfaceProviders.

Review of attachment 8767383 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 2

Comment 14

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

Thanks for the quick review! I didn't expect it until Tuesday. =)

I'll make that comment change.

Comment 15

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

Attachment: (Part 1) - Store ISurfaceProviders in the ImageLib SurfaceCache.

In this updated version I've clarified the comment for IsPlaceholder().

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2984a22f9dcb
(Part 1) - Store ISurfaceProviders in the ImageLib SurfaceCache. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a561bae08fbb
(Part 2) - Update SurfaceCache documentation and method names to reflect the fact that cache entries are now ISurfaceProviders. r=dholbert

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2984a22f9dcb
https://hg.mozilla.org/mozilla-central/rev/a561bae08fbb