1057894 - Add RAII smart handles for imgFrame locking

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

These days imgFrame objects can be locked in two ways:

(1) A cheap lock that ensures that the volatile buffer hasn't been purged. This is necessary for drawing. We usually do this implicitly by calling imgFrame::GetSurface(), but imgFrame::LockImageData() essentially does the same thing, plus...

(2) A potentially very expensive lock to allow reading and writing pixel data, where we rematerialize the imgFrame if it has been optimized away or stored in a different representation. We take this kind of lock using imgFrame::LockImageData(), and release it using imgFrame::UnlockImageData().

The current approach we're using, where the first kind of lock is mostly implicit and the second kind of lock is handled in a C-like, non-RAII manner, is hard to reason about and hard to maintain. It's also preventing me from storing imgFrame objects in the SurfaceCache directly right now, because for reasonable cache behavior I need to be able to return a handle that keeps the imgFrame locked for drawing (lock type #1, in other words) while it's alive.

In this bug I'm going to add new RAII smart handles for imgFrame that automatically perform the two types of locking. I'll also be converting existing code to this model if it's easy to do so. There's some code that isn't so easy to convert, and I'll handle that in followups. That also means that this bug won't be able to *remove* the old, unsafe API. Again, that'll get taken care of in followups.

Comment 1

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

Attachment: (Part 1) - Make VolatileBufferPtr's moveable and resettable

First, some preliminary work. We'll need VolatileBufferPtr objects to be moveable and (partly because we need it for move support) resettable.

Comment 2

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

Attachment: (Part 2) - Add RAII smart handles for imgFrame locking

This patch actually adds the smart handles.

We'll put them to use in the followups; it turned out that all of them were a big more complicated that I wanted for this bug.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8478045 [details] [diff] [review]
(Part 2) - Add RAII smart handles for imgFrame locking

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

2 drive-by nits that I noticed while skimming this in preparation for reviewing bug 1054079:

::: image/src/imgFrame.h
@@ +251,5 @@
> + * and GetDrawTarget() are guaranteed to succeed. This guarantee is stronger
> + * than DrawableFrameRef, so everything that a valid DrawableFrameRef guarantees
> + * is also guaranteed by a valid RawAccessFrameRef.
> + *
> + * This may be considerably more expensive is necessary just for drawing, so

(Looks like you may be missing a word or two there, in "This may be considerably more expensive is necessary")

@@ +270,5 @@
> +  {
> +    MOZ_ASSERT(mFrame, "Need a frame");
> +
> +    nsresult rv = mFrame->LockImageData();
> +    

(nit: drop whitespace-on-blank-line)

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 8478043 [details] [diff] [review]
(Part 1) - Make VolatileBufferPtr's moveable and resettable

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

::: memory/mozalloc/VolatileBuffer.h
@@ +89,5 @@
>    bool WasBufferPurged() const {
>      return mPurged;
>    }
>  
> +  void Reset() {

Isn't this the same as Set(nullptr)? Set also has the advantage of calling Unlock which clears mMapping and mPurged.

Comment 5

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

(In reply to Michael Wu [:mwu] from comment #4)
> Isn't this the same as Set(nullptr)? Set also has the advantage of calling
> Unlock which clears mMapping and mPurged.

So Reset() does call Unlock(), but I take your point that it's basically the same thing as Set(nullptr). I need this method to be public, though. Would you prefer to make Set() public? Or maybe keep Reset() but implement it explicitly as "Set(nullptr);"?

Comment 6

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

(In reply to Seth Fowler [:seth] from comment #5)
> (In reply to Michael Wu [:mwu] from comment #4)
> > Isn't this the same as Set(nullptr)? Set also has the advantage of calling
> > Unlock which clears mMapping and mPurged.
> 
> So Reset() does call Unlock(), but I take your point that it's basically the
> same thing as Set(nullptr). I need this method to be public, though. Would
> you prefer to make Set() public? Or maybe keep Reset() but implement it
> explicitly as "Set(nullptr);"?

Sorry, actually come to think of it, Set() is basically already public because of the operator=() overload in VolatileBufferPtr. I guess we can remove Reset(). I'll upload a new version of the patch.

Comment 7

[Michael Wu [:mwu]]

Oh, I actually meant Lock() rather than Unlock(). Unlock unlocks the buffer, but only Lock will clear mMapping and mPurged.

Comment 8

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

Attachment: (Part 1) - Make VolatileBufferPtr's moveable

Removed Reset().

Comment 9

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

Attachment: (Part 2) - Add RAII smart handles for imgFrame locking

Replaced calls to Reset() with use of operator=(), and fixed Daniel's nits.

Comment 10

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

(In reply to Michael Wu [:mwu] from comment #7)
> Oh, I actually meant Lock() rather than Unlock(). Unlock unlocks the buffer,
> but only Lock will clear mMapping and mPurged.

Ah whoops, somehow I missed that. At any rate I'm using Set() (via operator=) so it should be alright now.

Comment 11

[Michael Wu [:mwu]]

Comment on attachment 8479476 [details] [diff] [review]
(Part 1) - Make VolatileBufferPtr's moveable

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

Looks good.

Comment 12

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

Attachment: (Part 2) - Add RAII smart handles for imgFrame locking

I noticed a mistake in the implementation of RawAccessFrameRef::operator= while working on a later patch. Here's an updated version.

Comment 13

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

Attachment: (Part 2) - Add RAII smart handles for imgFrame locking

Some more tweaks.

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8481882 [details] [diff] [review]
(Part 2) - Add RAII smart handles for imgFrame locking

>+public:
>+  DrawableFrameRef() { }

>+public:
>+  RawAccessFrameRef() { }

Why do we need these constructors? Seems like they would be useless.

Comment 15

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

Thanks for the review, Timothy!

(In reply to Timothy Nikkel (:tn) from comment #14)
> Why do we need these constructors? Seems like they would be useless.

We end up using them everywhere. It's handy to have a way to indicate that you *don't* have a DrawableFrameRef/RawAccessFrameRef, for example because you couldn't create one because the OS released the imgFrame's memory.

Comment 16

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

I don't have any try jobs specifically for this bug, but I have try jobs for bugs based on top of this that are all green, so I'm going to assume that means this will be green too.

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/28f288ae4918
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf8b258eabc

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28f288ae4918
https://hg.mozilla.org/mozilla-central/rev/cdf8b258eabc