1148213 - Implement image locking for VectorImage

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

We should implement image locking for VectorImage, so that rasterized versions of VectorImages that are currently visible stay in the SurfaceCache.

This wasn't done in the past because we hadn't come up with a good way to age old rasterized versions out of the SurfaceCache, so animations that continually changed the scale at which we were drawing the VectorImage could cause tons of different rasterized variants to be created, and none of them could be discarded because the image was locked. Obviously this could result in extremely excessive memory usage.

However, RasterImage suffers from the same issues now due to downscale-during-decode, and we started using the following protocol when caching a new version of the image to resolve the problem:

- Unlock all of the image's existing surfaces in the SurfaceCache, without unlocking the image itself.
- Add the new surface to the SurfaceCache. Since the image is locked, the new surface also gets locked.
- Send an invalidation. This will cause us to redraw every instance of the image which is still visible. Any surface we touch during this process gets locked again. The others are allowed to age out of the cache.

We can use the same protocol for VectorImage. The downside is that the invalidation causes a wasteful second paint. That's not true in the RasterImage case because we redecode asynchronously, so the first paint doesn't succeed or at least had to use a fallback version of the image. But with VectorImage, we'll always succeed on the first paint, so we're actually introducing a new invalidation here.

IMO, we should just not worry about this for now. We should always be painting pre-rasterized surfaces in response to these invalidations, which is reasonably fast, and is comparable to the work that RasterImage does in a similar situation. If we start seeing performance problems because of this, we could mitigate them by only invalidating periodically and letting some extra surfaces accumulate in the meantime. The best memory-performance tradeoffs aren't obvious here, though, so I don't think guessing about the best behavior is a good idea; let's start with the simple thing.

Comment 1

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

Attachment: Implement image locking for VectorImage.

Here's the patch. I've taken pains to ensure that our behavior here is exactly
the same as RasterImage.

Comment 2

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

BTW, one additional benefit of this move is that with this patch, we remove one of the two remaining SurfaceCache callers that uses Lifetime::Transient. Soon we will be able to totally remove that feature from the SurfaceCache.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8584816 [details] [diff] [review]
Implement image locking for VectorImage.

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

r=me, just a few nits:

::: image/src/VectorImage.cpp
@@ +973,5 @@
> +  }
> +
> +  MOZ_ASSERT(mLockCount > 0, "Calling UnlockImage with mLockCount == 0!");
> +  if (mLockCount == 0) {
> +    return NS_ERROR_ABORT;

You're checking the condition twice here - once in the assertion, and once afterwards.

Consider replacing the assertion with...
  MOZ_ASSERT_UNREACHABLE("Calling UnlockImage with mLockCount == 0!");
...inside of the "if"-guarded clause.

@@ +978,5 @@
> +  }
> +
> +  mLockCount--;
> +
> +  if (mLockCount == 0 ) {

Nit: drop extra space char after "0"

::: image/src/VectorImage.h
@@ +102,5 @@
>    nsRefPtr<SVGLoadEventListener>     mLoadEventListener;
>    nsRefPtr<SVGParseCompleteListener> mParseCompleteListener;
>  
> +  /// Count of locks on this image (roughly correlated to visible instances).
> +  uint32_t mLockCount;

Extra "/" ?

Comment 4

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

Thanks for the review! I'll make those changes.


(In reply to Daniel Holbert [:dholbert] from comment #3)
> Extra "/" ?

That's actually a single-line C++-style Doxygen comment.

Comment 5

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

Attachment: Implement image locking for VectorImage.

I implemented the review changes and pushed a try job:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=437734fe57d0

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Seth Fowler [:seth] from comment #4)
> (In reply to Daniel Holbert [:dholbert] from comment #3)
> > Extra "/" ?
> 
> That's actually a single-line C++-style Doxygen comment.

Gotcha -- I thought it might be a mistake, since that seems to be the only "///" comment in that file. (But now I see there are some others elsewhere in imagelib.)

Thanks!

Comment 7

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

Yeah, I'm not sure if anyone but ImageLib is using them. =)

Try looks good. Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f12a294cf85

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5f12a294cf85