1296147 - Add a DrawableSurface type to allow surfaces to be computed lazily

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Right now the surface cache stores ISurfaceProvider objects. An ISurfaceProvider, internally, either contains or can generate a surface (represented as an imgFrame object). These surfaces are usually backed by a volatile buffer that can be freed by the OS if they're not locked. Right now, the mechanism we use to check if the volatile buffer has been freed is to try to get a valid DrawableFrameRef for the surface, but the problem is that to do this, we need to actually generate the surface. We don't want to do that within the surface cache code for ISurfaceProviders that dynamically generate their surfaces, because doing so could be quite expensive, and we'd be doing it while holding a global lock. That's the problem that bug 1289628 intended to solve.

Unfortunately, the approach in that bug changed our behavior when volatile buffers are being freed in a way that triggered bug 1292290. So we need a different approach.

This bug implements an alternate approach that I considered for bug 1289628 but rejected because it's a little more complex. However, it should avoid the problem that we encountered in bug 1289628 (since it maintains precisely the same behavior we have now with respect to volatile buffers), and I think there are actually some advantages to this approach, so maybe it's a blessing in disguise.

The new approach is to add a new type, DrawableSurface, which wraps a DrawableFrameRef and provides the same functionality that DrawableFrameRef does. The twist is that it may compute the DrawableFrameRef it wraps lazily; it can be initialized with an ISurfaceProvider rather than a DrawableFrameRef, and in that situation it will get the DrawableFrameRef from the ISurfaceProvider the first time calling code requests it.

It's worth noting that we don't actually use this yet, but we will soon. To take advantage of it, bug 1293472 needs to land.

Comment 1

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

Attachment: (Part 1a) - Add a DrawableSurface RAII type to allow lazy surface generation.

This patch introduces the actual DrawableSurface type and switches us over to
using it in the SurfaceCache code.

There are too many things called surfaces, I know. =) I have patches already
written that try to clean that up a little, but they can't land until some
blocking bugs land.

Note that the practical behavior of this code is exactly the same as the
existing code; we aren't using the new functionality yet. This is just
refactoring at this point.

One other thing that's worth noting is that there *is* one situation where we
dereference a DrawableSurface with the lock held: the memory reporting code.
We'll fix that in bug 1293472. It doesn't matter for now, as we're not actually
using the laziness anyway.

Comment 2

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

Attachment: (Part 1b) - Use DrawableSurface in ImageLib code.

This patch propagates the changes in part 1a to the rest of the ImageLib code.
It's an extremely straightforward translation.

Comment 3

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

Attachment: (Part 2) - Rename imgFrame::GetSurface() to imgFrame::GetSourceSurface() for clarity.

It's a bit confusing how in some places in part 1b we're calling
result.Surface()->GetSurface(). Since LookupResult::Surface() is called a lot
more than imgFrame::GetSurface() (at least, outside of test code), it seems
logical to privelege that method from a naming perspective, so this patch gives
imgFrame::GetSurface() a longer name (GetSourceSurface) in order to clarify
things a bit.

Comment 4

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

Here's a try job:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=055836225d1b

Comment 5

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

Thanks for the review, Edwin!

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8782215 [details] [diff] [review]
(Part 1a) - Add a DrawableSurface RAII type to allow lazy surface generation.

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

r=me, just one nit:

::: image/ISurfaceProvider.h
@@ +94,5 @@
> + * be dereferenced (i.e., operator->() should not be called) until you're
> + * sure that you want to draw it.
> + */
> +class DrawableSurface final
> +{

Looks like this class (DrawableSurface) can & should be declared as MOZ_RAII, since that's how it's used and since this patch's commit message calls it an RAII class. :)

You'll need to add #include "mozilla/Attributes.h" as well, to provide MOZ_RAII here.

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8782217 [details] [diff] [review]
(Part 2) - Rename imgFrame::GetSurface() to imgFrame::GetSourceSurface() for clarity.

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

::: image/ISurfaceProvider.h
@@ +155,5 @@
>  };
>  
>  
>  // Surface() is implemented here so that DrawableSurface's definition is visible.
> +inline DrawableSurface

nit: belongs in part 1a, I think.

Comment 8

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

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

Comment 9

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

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Looks like this class (DrawableSurface) can & should be declared as
> MOZ_RAII, since that's how it's used and since this patch's commit message
> calls it an RAII class. :)

Alas, I just looked at the definition of MOZ_RAII, and it looks like we can't use it here, because DrawableSurface temporaries are legal. They make sense; for example, we use them frequently for returning an "empty" DrawableSurface in the same way that you might return indicate failure by returning nullptr from a function that returns a pointer. It's probably best to think of DrawableSurface as a move-only smart pointer.

What we *can* do is mark DrawableSurface MOZ_STACK_CLASS, which is the same as MOZ_RAII except that temporaries are allowed. I'll make that change.

Comment 10

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

I'll update the misleading commit message, too.

Comment 11

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

Attachment: (Part 1a) - Add a DrawableSurface smart pointer type to allow lazy surface generation.

Marked DrawableSurface MOZ_STACK_CLASS, and moved the line from part 2 that
belonged here into this part.

Comment 12

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

Attachment: (Part 1b) - Use DrawableSurface in ImageLib code.

Comment 13

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

Attachment: (Part 2) - Rename imgFrame::GetSurface() to imgFrame::GetSourceSurface() for clarity.

Removed the line should should've been in part 1a.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9b465ebb5f
(Part 1) - Add a DrawableSurface smart pointer type to allow lazy surface generation. r=dholbert,edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7caba6197ba
(Part 2) - Rename imgFrame::GetSurface() to imgFrame::GetSourceSurface() for clarity. r=edwin

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f9b465ebb5f
https://hg.mozilla.org/mozilla-central/rev/e7caba6197ba