Closed Bug 534787 Opened 12 years ago Closed 12 years ago

Properly flush and markdirty surfaces in libpr0n

Categories

(Core :: ImageLib, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .17-fixed

People

(Reporter: bas.schouten, Assigned: joe)

References

Details

Attachments

(2 files, 3 obsolete files)

Currently we don't properly flush surfaces and mark them dirty in libpr0n. This ruins the usage of snapshots for detecting surface invalidations.
Assignee: nobody → joe
Blocks: 527707
Component: ImageLib → Graphics
QA Contact: imagelib → thebes
Component: Graphics → ImageLib
QA Contact: thebes → imagelib
Attached patch Hack currently used in D2D build (obsolete) — Splinter Review
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.
Attached patch somewhat less hacky patch (obsolete) — Splinter Review
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?
Attachment #418285 - Flags: review-
(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?
Attached patch Flush/MarkDirty as required (obsolete) — Splinter Review
Attachment #418222 - Attachment is obsolete: true
Attachment #418285 - Attachment is obsolete: true
Attachment #426321 - Flags: review?(jmuizelaar)
Depends on: 545513
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?
Attachment #426321 - Attachment is obsolete: true
Attachment #426321 - Flags: review?(jmuizelaar)
Dependent bug 545513 had is fixed in changeset 7c7debeec5a2, resolving this bug completely.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 548988
The LockImageData and UnlockImageData were not sufficient for animated GIFs, see bug 548988. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 548967
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.
Attachment #429374 - Flags: review?(jmuizelaar)
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.
Attachment #429374 - Flags: review?(jmuizelaar) → review+
Pushed to trunk changeset b49a6a8a4973.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Attached patch 1.9.2 versionSplinter Review
Attachment #501022 - Flags: review?(jmuizelaar)
Attachment #501022 - Flags: approval1.9.2.14?
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.
Attachment #501022 - Flags: review?(jmuizelaar) → review+
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
Attachment #501022 - Flags: approval1.9.2.14? → approval1.9.2.14+
Blocks: 597174
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.
Attachment #501022 - Flags: approval1.9.2.14+ → approval1.9.2.14-
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.
Attachment #501022 - Flags: approval1.9.2.16+
You need to log in before you can comment on or make changes to this bug.