Currently we don't properly flush surfaces and mark them dirty in libpr0n. This ruins the usage of snapshots for detecting surface invalidations.

Adding the hacks I did to make this work for the D2D builds. Note this doesn't actually do it 'properly' or fully adhere to cairo conventions.

Here is something I've come up with that's a little less hacky than what Bas posted. The only part I'm not sure about is whether we need to MarkDirty in ImageUpdated(), and thus whether we need to Flush() before we call ImageUpdated(). We don't actually optimize to a system surface until imgContainer::DecodeComplete() is called, so I guess I'm wondering whether Bas has hooked d2d into regular image surfaces too. If so, we'll have to figure something else out, probably an UpdatingImage() pre-flight hook.

Actually, the thebes optimize function returns null for d2d. so d2d uses the image surfaces directly.

This is marked as blocking the Direct 2D patch. If that's true, we should review this and get it landed.

Comment on attachment 418285 [details] [diff] [review] somewhat less hacky patch >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 >@@ -718,16 +718,17 @@ nsresult imgFrame::ImageUpdated(const ns > // you, gif decoder) > nsIntRect boundsRect(0, 0, mSize.width, mSize.height); > mDecoded.IntersectRect(mDecoded, boundsRect); > > #ifdef XP_MACOSX > if (mQuartzSurface) > mQuartzSurface->Flush(); > #endif >+ Spurious whitespace change. > > void imgFrame::GetImageData(PRUint8 **aData, PRUint32 *length) const > { > if (mImageSurface) >+ mImageSurface->Flush(); >+ What drawing is this flush flushing? >+ if (mImageSurface) > *aData = mImageSurface->Data(); > else if (mPalettedImageData) > *aData = mPalettedImageData + PaletteDataLength(); > else > *aData = nsnull; > > *length = GetImageDataLength(); > } >@@ -822,37 +826,47 @@ nsresult imgFrame::LockImageData() > gfxImageSurface::ImageFormatARGB32); > if (!mImageSurface || mImageSurface->CairoStatus()) > return NS_ERROR_OUT_OF_MEMORY; > > gfxContext context(mImageSurface); > context.SetOperator(gfxContext::OPERATOR_SOURCE); > if (mSinglePixel) > context.SetDeviceColor(mSinglePixelColor); >- else >+ else { > context.SetSource(mOptSurface); >+ } Spurious formatting change. > context.Paint(); > > mOptSurface = nsnull; > #ifdef CAN_USE_WIN_SURFACE > mWinSurface = nsnull; > #endif > #ifdef XP_MACOSX > mQuartzSurface = nsnull; > #endif > } > >+ // We might write to the bits in this image surface, so we need to make the >+ // surface ready for that. >+ if (mImageSurface) >+ mImageSurface->Flush(); >+ Should we be flushing the other surface types too? > return NS_OK; > } > > nsresult imgFrame::UnlockImageData() > { > if (mPalettedImageData) > return NS_ERROR_NOT_AVAILABLE; > >+ // Assume we've been written to. >+ if (mImageSurface) >+ mImageSurface->MarkDirty(); >+ > #ifdef XP_MACOSX > if (mQuartzSurface) > mQuartzSurface->Flush(); > #endif > return NS_OK; > } Why do we markdirty image surfaces but flush quartz surfaces?

(In reply to comment #5) > (From update of attachment 418285 [details] [diff] [review]) > > > > void imgFrame::GetImageData(PRUint8 **aData, PRUint32 *length) const > > { > > if (mImageSurface) > >+ mImageSurface->Flush(); > >+ > > What drawing is this flush flushing? So, just so we're all on one page. Apparently the cairo 'rule' is to flush -anytime- before you write directly to the surface data. And MarkDirty after. The Flush is what detaches the snapshots of a surface. If you haven't flushed but MarkDirty(while something else might've attached a snapshot - say D2D ;)), cairo will assert.

(In reply to comment #5) > >+ // We might write to the bits in this image surface, so we need to make the > >+ // surface ready for that. > >+ if (mImageSurface) > >+ mImageSurface->Flush(); > >+ > > Should we be flushing the other surface types too? Other surface types aren't written to directly, so no. > > > return NS_OK; > > } > > > > nsresult imgFrame::UnlockImageData() > > { > > if (mPalettedImageData) > > return NS_ERROR_NOT_AVAILABLE; > > > >+ // Assume we've been written to. > >+ if (mImageSurface) > >+ mImageSurface->MarkDirty(); > >+ > > #ifdef XP_MACOSX > > if (mQuartzSurface) > > mQuartzSurface->Flush(); > > #endif > > return NS_OK; > > } > > Why do we markdirty image surfaces but flush quartz surfaces? Ask Vlad.

(In reply to comment #7) > > > > > return NS_OK; > > > } > > > > > > nsresult imgFrame::UnlockImageData() > > > { > > > if (mPalettedImageData) > > > return NS_ERROR_NOT_AVAILABLE; > > > > > >+ // Assume we've been written to. > > >+ if (mImageSurface) > > >+ mImageSurface->MarkDirty(); > > >+ > > > #ifdef XP_MACOSX > > > if (mQuartzSurface) > > > mQuartzSurface->Flush(); > > > #endif > > > return NS_OK; > > > } > > > > Why do we markdirty image surfaces but flush quartz surfaces? > > Ask Vlad. So this change came from: b=416181, animated image used as repeated background broken ; r=stuart It looks like the quartz-image-surface (ab)uses the flush method to get the cairo_image_surface data into a CGImage. Can we add a comment saying that?

This patch doesn't work yet. Animated GIFs behave correctly, however when we have a progressively loading image (i.e. a big one like http://apod.nasa.gov/apod/image/0304/bluemarble2k_big.jpg). The image doesn't appear to be properly flushed when new data arrives and more of the image is added.

Right. This patch actually needs to be reworked on top of dependent bug 545513 (it's possible that everything needed is done in that bug.) Can you comment on whether that bug fixes it?

Dependent bug 545513 had is fixed in changeset 7c7debeec5a2, resolving this bug completely.

The LockImageData and UnlockImageData were not sufficient for animated GIFs, see bug 548988. Reopening.

This is essentially Joe's patch, I just created one that applies on the current tree. This should fix the animated GIF and partially black image issues. Should be able to go for a quick review.

Comment on attachment 429374 [details] [diff] [review] Properly Flush/MarkDirty Surfaces I was talking to Joe about the Flush() in GetImageData() and he didn't think we needed it. So take that one out. Everything else looks fine.

Pushed to trunk changeset b49a6a8a4973.

Why do we need this on the 1.9.2 branch?

This bug's patch is apparently one of the requirements to fix bug 597174 (for distros that build with newer system-cairo versions than the one that we bundle).

Comment on attachment 501022 [details] [diff] [review] 1.9.2 version (s)r=dveditz on the basis of the code being the same give or take context differences. Still need a review on bug 597174 before it makes sense to take this patch, right? Code-freeze for 1.9.2.14 in less than a week, Tuesday Jan 11.

Comment on attachment 501022 [details] [diff] [review] 1.9.2 version Ah, this one (and two others) are in place of bug 597174... got it. Approved for 1.9.2.14, a=dveditz for release-drivers

Comment on attachment 501022 [details] [diff] [review] 1.9.2 version Missed non-blocker code-freeze and 1.9.2.14 -- rescinding approval, you can try again next time.

Comment on attachment 501022 [details] [diff] [review] 1.9.2 version 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.