Closed Bug 1057894 Opened 5 years ago Closed 5 years ago

Add RAII smart handles for imgFrame locking

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Blocks: 1057903
Blocks: 1057904
First, some preliminary work. We'll need VolatileBufferPtr objects to be moveable and (partly because we need it for move support) resettable.
Attachment #8478043 - Flags: review?(mwu)
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.
Attachment #8478045 - Flags: review?(tnikkel)
Blocks: 1057906
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 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.
(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);"?
(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.
Oh, I actually meant Lock() rather than Unlock(). Unlock unlocks the buffer, but only Lock will clear mMapping and mPurged.
Removed Reset().
Attachment #8479476 - Flags: review?(mwu)
Attachment #8478043 - Attachment is obsolete: true
Attachment #8478043 - Flags: review?(mwu)
Replaced calls to Reset() with use of operator=(), and fixed Daniel's nits.
Attachment #8479482 - Flags: review?(tnikkel)
Attachment #8478045 - Attachment is obsolete: true
Attachment #8478045 - Flags: review?(tnikkel)
(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 on attachment 8479476 [details] [diff] [review]
(Part 1) - Make VolatileBufferPtr's moveable

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

Looks good.
Attachment #8479476 - Flags: review?(mwu) → review+
Depends on: DrawAPIRefactor
I noticed a mistake in the implementation of RawAccessFrameRef::operator= while working on a later patch. Here's an updated version.
Attachment #8481873 - Flags: review?(tnikkel)
Attachment #8479482 - Attachment is obsolete: true
Attachment #8479482 - Flags: review?(tnikkel)
Some more tweaks.
Attachment #8481882 - Flags: review?(tnikkel)
Attachment #8481873 - Attachment is obsolete: true
Attachment #8481873 - Flags: review?(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.
Attachment #8481882 - Flags: review?(tnikkel) → review+
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.
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
https://hg.mozilla.org/mozilla-central/rev/28f288ae4918
https://hg.mozilla.org/mozilla-central/rev/cdf8b258eabc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.