Closed Bug 1282327 Opened 3 years ago Closed 3 years ago

Store ISurfaceProviders in the ImageLib SurfaceCache instead of storing surfaces directly

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
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.
Attachment #8765305 - Flags: review?(dholbert)
This updated version of the patch adds an IsPlaceholder() method to
ISurfaceProviders. It turns out that that'll be useful down the road.
Attachment #8765313 - Flags: review?(dholbert)
Attachment #8765305 - Attachment is obsolete: true
Attachment #8765305 - Flags: review?(dholbert)
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
Blocks: 1282352
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba925dbd233
Store ISurfaceProviders in the ImageLib SurfaceCache. r=dholbert
Pushed by mfowler@mozilla.com:
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 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 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
Attachment #8765313 - Flags: review?(dholbert) → review-
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.
Blocks: 1283967
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.
Attachment #8767382 - Flags: review?(dholbert)
Attachment #8765313 - Attachment is obsolete: true
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.
Attachment #8767383 - Flags: review?(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!! :))
Attachment #8767382 - Flags: review?(dholbert) → review+
... or "a non-empty DrawableFrameRef".  (just notice usages of the term "empty DrawableFrameRef" elsewhere in SurfaceCache.h)
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
Attachment #8767383 - Flags: review?(dholbert) → review+
Thanks for the quick review! I didn't expect it until Tuesday. =)

I'll make that comment change.
In this updated version I've clarified the comment for IsPlaceholder().
Attachment #8767382 - Attachment is obsolete: true
Pushed by mfowler@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/2984a22f9dcb
https://hg.mozilla.org/mozilla-central/rev/a561bae08fbb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1298551
You need to log in before you can comment on or make changes to this bug.