545513 - need a pre-write hook for decoders (something to match ImageUpdated/FrameUpdated calls)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

In order to properly Flush and MarkDirty cairo surfaces when decoding & displaying images (bug 534787), we need to know _before_ a write will happen, so we can Flush the relevant surface. This bug will add this "pre-flight" hook to imgFrame/imgContainer, and call it from all the image decoders.

Comment 1

[Joe Drew (not getting mail)]

Attachment: explicitly lock/unlock images when decoding

Rather than add a pre-update hook, which needs to be manually spliced into all the decoders, I've taken the step to explicitly Lock and Unlock images when we call decoders, and to automatically manage the locking and unlocking when appending new frames. This morphs this bug a bit, but its effect is the same: we will always flush and markdirty at the appropriate time.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I don't really like the ability to have multiple/recursive locks. Can we avoid this at all?

Comment 3

[Joe Drew (not getting mail)]

No; adding LockImageData calls to imgContainer means that we can't rely on there only being one call open at any time. (This wouldn't be an issue, since locking is mostly idempotent, except that Cairo's Flush and MarkDirty aren't.)

Comment 4

[Jeff Muizelaar [:jrmuizel]]

After discussing this with Joe, I believe I convinced him that we don't need to support recursive locking here and can just have a check for lockedness and abort if any recursive locking occurs.

Comment 5

[Bas Schouten (:bas.schouten)]

Attachment: explicitly lock/unlock images when decoding v2

Implemented as a boolean as requested by Joe. Used similar NS_ABORT_IF_FALSE/runtime checking logic style that was used with the recursive lock.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 427781 [details] [diff] [review]
explicitly lock/unlock images when decoding v2

Overall I don't like the complexity of this patch. It's not immediately obvious which lock pairs with which unlock. However,
alternative solutions seem like a lot more work, or would
just spread the complexity around...

>diff --git a/modules/libpr0n/src/imgFrame.cpp b/modules/libpr0n/src/imgFrame.cpp
>--- a/modules/libpr0n/src/imgFrame.cpp
>+++ b/modules/libpr0n/src/imgFrame.cpp
>   if (mPalettedImageData)
>     return NS_ERROR_NOT_AVAILABLE;
> 
>+  NS_ABORT_IF_FALSE(!mLocked, "Trying to lock already locked image data.");

>+  if (mLocked) {
>+    return NS_ERROR_FAILURE;
>+  }

I really don't like having this check after an ABORT_IF_FALSE, but
was unable to convince Joe otherwise.

>+  mLocked = PR_TRUE;
>+
>   if ((mOptSurface || mSinglePixel) && !mImageSurface) {
>     // Recover the pixels
>     mImageSurface = new gfxImageSurface(gfxIntSize(mSize.width, mSize.height),
>                                         gfxImageSurface::ImageFormatARGB32);
>     if (!mImageSurface || mImageSurface->CairoStatus())
>       return NS_ERROR_OUT_OF_MEMORY;
> 
>     gfxContext context(mImageSurface);
>@@ -840,16 +847,23 @@ nsresult imgFrame::LockImageData()
>   return NS_OK;
> }
> 
> nsresult imgFrame::UnlockImageData()
> {
>   if (mPalettedImageData)
>     return NS_ERROR_NOT_AVAILABLE;
> 
>+  NS_ABORT_IF_FALSE(mLocked, "Unlocking an unlocked image!");
>+  if (!mLocked) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
Same here

Comment 7

[Bas Schouten (:bas.schouten)]

Pushed to mozilla-central in changeset 7c7debeec5a2.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.9.2 version

Comment 9

[Daniel Veditz [:dveditz]]

Why do we need this on the 1.9.2 branch?

Comment 10

[Daniel Veditz [:dveditz]]

Answering my own question, "distros want it" (bug 597174).

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 501021 [details] [diff] [review]
1.9.2 version

waiting on review to verify we don't need and equivalent to the missing WriteToDecoder() chunk from the the trunk patch.

Comment 12

[Daniel Veditz [:dveditz]]

Still waiting on reviews and tomorrow is the code-freeze...

Comment 13

[Joe Drew (not getting mail)]

Comment on attachment 501021 [details] [diff] [review]
1.9.2 version

Two issues here:

There is no mInDecoder on 1.9.2 - you can just remove that NS_ABORT_IF_FALSE, leaving the comment.

The reason there's no imgContainer::WriteToDecoder is because it's done inside imgRequest::OnDataAvailable in 1.9.2. The locking/unlocking dance will still need to be done inside there.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.9.2 version, v2

Thanks for the review, this should address it. 

The decoder->WriteFrom() is also used in imgContainer::ReloadImages(), do we need to add the locking/unlocking there too?

Comment 15

[Joe Drew (not getting mail)]

Comment on attachment 503107 [details] [diff] [review]
1.9.2 version, v2

I think there's some other bug's patch mixed in here too (changes to ImageComplete, boundsRect, available, etc).

Yes, I think ReloadImages also needs the lock/unlock dance.

Comment 16

[Joe Drew (not getting mail)]

(Strictly speaking, ReloadImages won't need the Unlock, because we know we have no frames when we're reloading images. But it's harmless and maintains the idiom of WriteFrom being sandwiched between Unlock and Lock, so let's do it anyways.)

Comment 17

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.9.2 version, v3

v3, with lock/unlock in imgContainer::ReloadImages()

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 503332 [details] [diff] [review]
1.9.2 version, v3

Crap sorry, I thought I'd commented on this a while ago.

It looks like this patch was combined with another, because it contains changes to things like ImageComplete and boundsRect. Could you regenerate it without those unrelated chunks?

Comment 19

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.9.2 version, v4

Ahh, sorry, there's the regenerated one.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 506332 [details] [diff] [review]
1.9.2 version, v4

Approved for 1.9.2.16, a=dveditz for release-drivers

Code-freeze for 1.9.2.16 non-blockers is TOMORROW -- please land this ASAP if you intend to.

Comment 21

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/06f5a0cac308