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.

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.

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

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.)

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.

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 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

Pushed to mozilla-central in changeset 7c7debeec5a2.

Why do we need this on the 1.9.2 branch?

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

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.

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

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.

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 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.

(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.)

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

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?

Ahh, sorry, there's the regenerated one.

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.