Closed Bug 722217 Opened 8 years ago Closed 7 years ago

[Azure] Keep only a single snapshot in the Cairo backend

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: joe, Assigned: nrc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

roc changed the Direct2D backend to keep only a single snapshot, which is cached, and that makes fine sense to me. This patch changes the Cairo backend to do the same.

One question: SourceSurfaceCairo::GetDataSurface() currently just lets a data surface reference its context. I've thought about this a bit, and it seems correct, but I want to make sure that you think the same.
Attachment #592593 - Flags: review?(jmuizelaar)
Comment on attachment 592593 [details] [diff] [review]
Only create a single snapshot in the Cairo backend

Review of attachment 592593 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetCairo.cpp
@@ +732,3 @@
>  {
> +  if (mSnapshot) {
> +    mSnapshot->DrawTargetWillChange();

Maybe add the special case for one reference
Attachment #592593 - Flags: review?(jmuizelaar) → review+
We should probably do this for the skia backend too.
George Wright, I choose you!
Nick/Anthony, can you drive this to completion?
Joe: what needs doing? Any more than run it through Try, fix the bugs, and land it? (I guess we should address jrmuizel's comment too). Do you want one of us to look at the Skia version too, or can we leave that with gw280?
(In reply to Nick Cameron [:nrc] from comment #5)
> Joe: what needs doing? Any more than run it through Try, fix the bugs, and
> land it? (I guess we should address jrmuizel's comment too).

I think that's all that's necessary, yep.

> Do you want one
> of us to look at the Skia version too, or can we leave that with gw280?

gw280!
Assignee: joe → ncameron
Rebased Joe's patch, carrying r=jrmuizel (still need to address his comment above)
Attachment #592593 - Attachment is obsolete: true
Attachment #657178 - Flags: review+
Try push: https://tbpl.mozilla.org/?tree=Try&rev=79d25c2b1723

It seems to have broken all the image/encoders tests, which use a canvas for their implementation (WinXP, R). I'm ignoring the mac gradient fails, I think they are just because Azure/Cairo has never been tested on Mac before. I don't know why the image encoder tests would only fail on WinXP.
Attached patch patchSplinter Review
Addressed jrmuizel's comment and rebased again, carrying r=jrmuizel
Attachment #657178 - Attachment is obsolete: true
Attachment #658351 - Flags: review+
The Win7 (sorry, not XP) test fails I noted above turn out to be there without this patch, so I'll fix them elsewhere. This patch is fine.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=473353e9c37f
https://hg.mozilla.org/mozilla-central/rev/751b35e798d5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.