1054079 - Store imgFrame objects in the imagelib SurfaceCache

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

We want to be able to store things like HQ scaled frames in the imagelib surface cache, and that means that SurfaceCache should store imgFrame objects instead of SourceSurface objects.

This should also make it much easier to produce good memory reporting for the surface cache, and it means that things in the surface cache can take advantage of volatile buffers.

Comment 1

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

Attachment: Store imgFrame objects, not SourceSurface objects, in the SurfaceCache

Here's the patch. Matt, I'm hoping that you can take a look at the Moz2D-related things in the patch. Daniel, I'm hoping you can look at everything else.

Try job here:
https://tbpl.mozilla.org/?tree=Try&rev=b43517917519

Comment 2

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

Comment on attachment 8473353 [details] [diff] [review]
Store imgFrame objects, not SourceSurface objects, in the SurfaceCache

Hmm, actually now that I'm looking at this patch, I realize that bug 1054076 has most of the Moz2D code. Matt, I'll ask you to review that one instead.

Comment 3

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

Wow, these are some rather odd test failures. I'll look into this tomorrow. Looks like some strange color issues; maybe I'm creating the wrong type of surface?

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8473353 [details] [diff] [review]
Store imgFrame objects, not SourceSurface objects, in the SurfaceCache

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

One general comment request: Could you add a header-comment somewhere related to SurfaceCache (maybe just above the class definition) to clarify that the "surfaces" it deals with are really imgFrames? (or abstracted away behind imgFrames)

(Right now, the variable/function-naming & documentation in SurfaceCache is all in terms of "surfaces", while the variables themselves all have type "imgFrame". This distinction is a bit confusing, but a comment would help I think.)

::: image/src/SurfaceCache.h
@@ +122,5 @@
>     * without first calling Lookup to verify that the surface is not already in
>     * the cache.
>     *
>     * @param aTarget      The new surface (in the form of a DrawTarget) to insert
>     *                     into the cache.

This documentation (for SurfaceCache::Insert) needs updating, to mention the right variable-name -- s/aTarget/aSurface/ -- and the right type -- s/DrawTarget/imgFrame/.

@@ +126,5 @@
>     *                     into the cache.
>     * @param aImageKey    Key data identifying which image the surface belongs to.
>     * @param aSurfaceKey  Key data which uniquely identifies the requested surface.
>     */
> +  static void Insert(imgFrame*         aSurface,

(Here's the actual parameter, to provide some context for previous comment.)

::: image/src/VectorImage.cpp
@@ +925,2 @@
>    // to draw before returning from this function.
> +  RefPtr<SourceSurface> surface = frame->GetSurface();

This comment (about keeping the pixel data alive) might need a bit of massaging, given that the imgFrame isn't locked right now.

Also, per our conversation a few minutes ago, I think we need to null-check |surface| here before we draw from it, since the imgFrame isn't locked anymore at this point.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8473353 [details] [diff] [review]
Store imgFrame objects, not SourceSurface objects, in the SurfaceCache

Yikes, sorry -- splinter didn't provide very many lines of context for that last comment of mine (about rewording this comment & null-checking 'surface')

Here's the full quote from the patch file, for reference:

>   // Draw. Note that if SurfaceCache::Insert failed for whatever reason,
>-  // then |target| is all that is keeping the pixel data alive, so we have
>+  // then |frame| is all that is keeping the pixel data alive, so we have
>   // to draw before returning from this function.
>+  RefPtr<SourceSurface> surface = frame->GetSurface();
>   nsRefPtr<gfxDrawable> drawable =
>     new gfxSurfaceDrawable(surface, ThebesIntSize(aParams.size));
>   Show(drawable, aParams);

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8473353 [details] [diff] [review]
Store imgFrame objects, not SourceSurface objects, in the SurfaceCache

(Clearing review flag & setting feedback+ for now; holding off on final review, pending investigation of the reftest failures.)

Comment 7

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

Attachment: Store imgFrame objects, not SourceSurface objects, in the SurfaceCache

Here's a new version of the patch which should address the comments so far. Critically, the SurfaceCache now returns a DrawableFrameRef from Lookup() instead of an already_AddRefed<imgFrame>. The DrawableFrameRef keeps the imgFrame's surface in memory (that is, it keeps the underlying volatile buffer from being purged) while we draw. When we need to write to an imgFrame, a RawAccessFrameRef is used to keep the imgFrame in memory and stored with an appropriate representation. The lifetime of the RawAccessFrameRef now extends to the end of CreateSurfaceAndShow; since a RawAccessFrameRef provides all the guarantees of a DrawableFrameRef and more, that's enough to make sure it will be safe to draw.

(You may remember I had talked about moving some of these methods, like GetDrawTarget(), onto the appropriate RAII object instead of keeping them on imgFrame directly. It ends up being fairly nontrivial to do that, so that will have to wait for a followup.)

I've pushed a try job here:

https://tbpl.mozilla.org/?tree=Try&rev=a7144efbf1f0

As of now, it seems to be green everyone except Android (which just looks like an infrastructure issue; I've retriggered) and D2D versions of Windows, where we're getting some tiny rendering differences. I suspect a quick talk with the graphics folks should be enough to sort that out. The random garbage that was present in the previous version of this patch is gone though; those D2D oranges aren't anything serious.

Comment 8

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

Comment on attachment 8478114 [details] [diff] [review]
Store imgFrame objects, not SourceSurface objects, in the SurfaceCache

I take it back - looks like there's an issue on linux too! Sigh. It's probably not a big deal, but I'll unset the review flag until I can take a look tomorrow.

Comment 9

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

Attachment: (Part 1) - Add a way to check if it's safe to render content to data surfaces to gfxPlatform

Alright, this bug ended up being a little more complex than I expected, but I think I've gotten it all worked out now.

First up: part 1 gives us a new method on gfxPlatform that tells us whether it's safe to render content to data surfaces - in other words, whether data surfaces use the same backend that we use for normal content rendering. We need this to know when it's safe to render SVG content to a surface backed by a volatile buffer (which will be a data surface).

Comment 10

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

Attachment: (Part 2) - Make it possible to initialize an imgFrame with a gfxDrawable

This part gives imgFrame two Init() methods, rather than one. The new Init() method initializes the imgFrame with a gfxDrawable. This will allow us to store a rasterized version of SVG content in an imgFrame and get the benefits of doing that (primarily, volatile buffers support) on platforms that support it. On platforms where we can't get away with drawing content into a surface backed by a volatile buffer, we fall back to creating a normal offscreen surface. Fortunately, the platforms where we can get away with this include B2G and Android, that platforms where we need it the most.

This code sorta exists at the intersection of the expertise of several different people, so I thought multiple reviewers were wise. I'm hoping Timothy can review from an imagelib perspective, Matt from a graphics perspective, and Michael from a volatile buffers perspective. The interesting stuff for Matt and Michael is probably all in imgFrame.cpp.

Comment 11

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

Attachment: (Part 3) - Store imgFrame objects, not SourceSurface objects, in the SurfaceCache

Finally, in this version of the original patch on this bug we're able to safely and correctly construct an imgFrame from a gfxDrawable and store it in the SurfaceCache, which now holds imgFrames instead of surfaces directly.

The locking behavior in VectorImage.cpp should now be correct. If the lookup in the SurfaceCache succeeds, the DrawableFrameRef that Lookup() returns will keep the surface alive long enough to draw it. If we end up executing CreateSurfaceAndShow(), it's enough to call |frame->GetSurface()| and verify that the surface returned is non-null, since the surface holds a strong reference to its data and will keep it alive.

One note: Daniel, you mentioned before that we might want to consider renaming some of the methods in SurfaceCache that mention surfaces. As I mentioned on IRC before, I think it may make more sense to rename imgFrame, but I'd prefer to save that for a followup.

Comment 12

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

Here's a try job for the final version of the patch. I pushed one earlier today for a slightly different version and everything was green, so this one should be too unless I broke something during my final cleanup before review.

https://tbpl.mozilla.org/?tree=Try&rev=9a140d5e95be

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Seth Fowler [:seth] from comment #11)
> One note: Daniel, you mentioned before that we might want to consider
> renaming some of the methods in SurfaceCache that mention surfaces.

Yup -- in particular: we have lots of things called "frame", and other things called "surfaces"; and now we'll have a "frame"-flavored thing which we're naming "aSurface" ("imgFrame* aSurface" in the CachedSurface constructor), which adds another layer of confusion to the "frame"-terminology overloading.

> As I
> mentioned on IRC before, I think it may make more sense to rename imgFrame,
> but I'd prefer to save that for a followup.

That sounds good to me. Could you file that followup before closing out this bug, to be sure that to-do item doesn't get lost?

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8478855 [details] [diff] [review]
(Part 3) - Store imgFrame objects, not SourceSurface objects, in the SurfaceCache

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

r=me, with one nit:

::: image/src/SurfaceCache.cpp
@@ +137,2 @@
>    {
> +    return mSurface->DrawableRef();

So, you're renaming this method from "Drawable()" (old name) to "Surface()" (new name) in this patch.

But given that the method will now be returning a DrawableFrameRef, the original name "Drawable" still seems pretty reasonable. (and probably better than Surface().)

Maybe we should just stick with that, or some other Drawable-flavored name?  That would make this line of code from later in the patch...
> DrawableFrameRef ref = surface->Surface();
...look a bit clearer/saner IMHO, like so:
> DrawableFrameRef ref = surface->Drawable();

Comment 15

[Daniel Holbert [:dholbert]]

Sorry, splinter is really unclear about how much context it's going to show in my bug comment... That nit was about the naming of "DrawableFrameRef Surface()" (formerly "already_AddRefed<gfxDrawable> Drawable()"), if it not clear.

Comment 16

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

(In reply to Daniel Holbert [:dholbert] from comment #13)
> Could you file that followup before closing out this
> bug, to be sure that to-do item doesn't get lost?

Done; see bug 1058941.

Comment 17

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

Thanks for the review!

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Maybe we should just stick with that, or some other Drawable-flavored name? 

I can see what you mean! But I think that perversely, the new name is actually better. The reason is that previously, a "Drawable" was a *noun* -  a gfxDrawable object. The "Drawable" in "DrawableFrameRef", though, is an *adjective* - the noun there is Frame (i.e. imgFrame), which in bug 1058941 will probably get renamed to "ImageSurface", making the handle a "DrawableSurfaceRef". An imgFrame is really a type of surface (and a DrawableFrameRef is just a kind of handle to an imgFrame), so I think the name "Surface()" is probably the best choice. (I really don't want to use "Frame()"!)

Comment 18

[Daniel Holbert [:dholbert]]

Maybe DrawableSurface or DrawableRef or DrawableSurfaceRef, then?  (per IRC)

r=me regardless; if you decide ::Surface is best, I'm not worried enough about it to push back much. :)

Comment 19

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8478838 [details] [diff] [review]
(Part 1) - Add a way to check if it's safe to render content to data surfaces to gfxPlatform

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

I'd prefer CanRenderContentToDataSurface.

Comment 20

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

Attachment: (Part 1) - Add a way to check if it's safe to render content to data surfaces to gfxPlatform

Thanks for the review, Matt. I changed the name.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8478847 [details] [diff] [review]
(Part 2) - Make it possible to initialize an imgFrame with a gfxDrawable

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

::: image/src/imgFrame.cpp
@@ +155,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  mOffset.MoveTo(aRect.x, aRect.y);
> +  mSize.SizeTo(aRect.width, aRect.height);

You could do these as:

mOffset = aRect.TopLeft();
mSize = ToIntSize(aRect.Size());

Or even just combine mOffset/mSize into an mRect var.

@@ +239,5 @@
> +
> +  if (shouldUseDataSurface) {
> +    // It's safe to use data surfaces for content on this platform, so we can
> +    // get away with using volatile buffers.
> +    bool allocatingNewBuffer = !mImageSurface;

When would we already have something in mImageSurface at this point? And do we need to check that it matches the size/format requested?

@@ +262,5 @@
> +      mImageSurface = CreateLockedSurface(mVBuf, mSize, mFormat);
> +    }
> +
> +    target = gfxPlatform::GetPlatform()->
> +      CreateDrawTargetForData(ptr, mSize, stride, mFormat);

Check this result and assert/warn if it fails. We've already done the buffer allocation, so this should only fail if our ShouldRenderContentToDataSurface() result was incorrect.

Comment 22

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

Thanks for the review Matt!

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> Or even just combine mOffset/mSize into an mRect var.

That's probably the best resolution; I'll file a followup.

> 
> @@ +239,5 @@
> > +
> > +  if (shouldUseDataSurface) {
> > +    // It's safe to use data surfaces for content on this platform, so we can
> > +    // get away with using volatile buffers.
> > +    bool allocatingNewBuffer = !mImageSurface;
> 
> When would we already have something in mImageSurface at this point? And do
> we need to check that it matches the size/format requested?

So that's a pretty good point. I copied that check from the original Init() code, and I think the idea is to guard against calling Init() twice, but that'd be frankly insane to do. It seems reasonable to do a MOZ_ASSERT instead.

Comment 23

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

(In reply to Seth Fowler [:seth] from comment #22)
> Thanks for the review Matt!
> 
> (In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> > Or even just combine mOffset/mSize into an mRect var.
> 
> That's probably the best resolution; I'll file a followup.

Filed bug 1059026.

Comment 24

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

Attachment: (Part 2) - Make it possible to initialize an imgFrame with a gfxDrawable

This new version of the patch rebases it against changes in other patches and addresses Matt's review comments.

Comment 25

[Michael Wu [:mwu]]

Comment on attachment 8479528 [details] [diff] [review]
(Part 2) - Make it possible to initialize an imgFrame with a gfxDrawable

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

Makes sense overall, though I wonder about the error paths handling DiscardTracker::InformDeallocation correctly.

::: image/src/imgFrame.cpp
@@ -193,5 @@
>      if (!mImageSurface) {
>        NS_WARNING("Failed to create VolatileDataSourceSurface");
> -      // Image surface allocation is failed, need to return
> -      // the booked buffer size.
> -      DiscardTracker::InformDeallocation(4 * mSize.width * mSize.height);

Did you mean to remove this? Though not all the error paths here call InformDeallocation..

::: image/src/imgFrame.h
@@ +65,5 @@
> +   * uses the same graphics backend as normal content drawing. The downside is
> +   * that the underlying surface may not be stored in a volatile buffer on all
> +   * platforms, and raw access to the surface (using RawAccessRef() or
> +   * LockImageData()) may be much more expensive than in the InitForDecoder()
> +   * case.

I'm not sure this is necessary any worse than the end result of using imgFrame for decoded images. We attempt to optimize images once they're decoded, and that can also throw away the volatile buffer.

Comment 26

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

Thanks for the review!

(In reply to Michael Wu [:mwu] from comment #25)
> Did you mean to remove this? Though not all the error paths here call
> InformDeallocation..

Yeah, this was intentional. We call InformDeallocation in the destructor, and if InitForDecoder() returns failure, all the callers are just going to immediately destroy the imgFrame object anyway.

> I'm not sure this is necessary any worse than the end result of using
> imgFrame for decoded images. We attempt to optimize images once they're
> decoded, and that can also throw away the volatile buffer.

You're right; I'm only trying to warn callers not to call e.g. GetDrawTarget() or LockImageData() in an attempt to further write to or read from the imgFrame after calling InitWithDrawable(). If you use InitWithDrawable(), you should only write to the imgFrame using the gfxDrawable you provide, or you can expect bad performance on some platforms.

Comment 27

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

Thanks for the review! Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cbcfd3c335f
https://hg.mozilla.org/integration/mozilla-inbound/rev/721e6ae47dbc
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bfa3ac98ba1

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cbcfd3c335f
https://hg.mozilla.org/mozilla-central/rev/721e6ae47dbc
https://hg.mozilla.org/mozilla-central/rev/3bfa3ac98ba1