534787 - Properly flush and markdirty surfaces in libpr0n

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bas Schouten (:bas.schouten)]

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

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Hack currently used in D2D build

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.

Comment 2

[Joe Drew (not getting mail)]

Attachment: somewhat less hacky patch

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.

Comment 3

[Bas Schouten (:bas.schouten)]

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

Comment 4

[John Daggett (:jtd)]

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

Comment 5

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 6

[Bas Schouten (:bas.schouten)]

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

Comment 7

[Joe Drew (not getting mail)]

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

Comment 8

[Jeff Muizelaar [:jrmuizel]]

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

Comment 9

[Joe Drew (not getting mail)]

Attachment: Flush/MarkDirty as required

Comment 10

[Bas Schouten (:bas.schouten)]

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.

Comment 11

[Joe Drew (not getting mail)]

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?

Comment 12

[Bas Schouten (:bas.schouten)]

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

Comment 13

[Bas Schouten (:bas.schouten)]

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

Comment 14

[Bas Schouten (:bas.schouten)]

Attachment: Properly Flush/MarkDirty Surfaces

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 15

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 16

[Bas Schouten (:bas.schouten)]

Pushed to trunk changeset b49a6a8a4973.

Comment 17

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

Attachment: 1.9.2 version

Comment 18

[Daniel Veditz [:dveditz]]

Why do we need this on the 1.9.2 branch?

Comment 19

[Daniel Holbert [:dholbert]]

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 20

[Daniel Veditz [:dveditz]]

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 21

[Daniel Veditz [:dveditz]]

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 22

[Daniel Veditz [:dveditz]]

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 23

[Daniel Veditz [:dveditz]]

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.

Comment 24

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8a83607a78fd