Closed Bug 1074842 Opened 6 years ago Closed 6 years ago

Don't copy DrawTargetCG data into the snapshot if the DrawTarget goes away

Categories

(Core :: Graphics, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mstange, Assigned: mstange)

Details

Attachments

(1 file, 2 obsolete files)

There are a few places where we're using snapshots like this:

> TemporaryRef<SourceSurface> GetSomeSurface()
> {
>   RefPtr<DrawTarget> dt = CreateDT();
>   dt->DoSomething();
>   return dt->Snapshot();
> }
> 
> RefPtr<SourceSurface> surf = GetSomeSurface();

If dt is a DrawTargetCG, this pattern always results in an unnecessary copy when the DrawTarget is destroyed at the end of GetSomeSurface(). We should instead move the ownership of the DT's data into the snapshot.

I've encountered this problem in nsFilterInstance::BuildSourcePaint, nsFilterInstance::BuildSourceImage, and AdjustedTargetForFilter::DoSourcePaint.
Attached patch v1 (obsolete) — Splinter Review
Attachment #8497510 - Flags: review?(bas)
Attached patch v2 (obsolete) — Splinter Review
using mfbt Swap() instead of std::swap
Attachment #8497510 - Attachment is obsolete: true
Attachment #8497510 - Flags: review?(bas)
Attachment #8497512 - Flags: review?(bas)
Attached patch v3Splinter Review
this one actually builds
Attachment #8497512 - Attachment is obsolete: true
Attachment #8497512 - Flags: review?(bas)
Attachment #8497544 - Flags: review?(bas)
Comment on attachment 8497544 [details] [diff] [review]
v3

Stealing review from Bas to Jeff.
Attachment #8497544 - Flags: review?(bas) → review?(jmuizelaar)
Comment on attachment 8497544 [details] [diff] [review]
v3

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

Do any other backends suffer from this same problem?
Attachment #8497544 - Flags: review?(jmuizelaar) → review+
I just looked at DrawTargetSkia and DrawTargetCairo: Both don't call MarkChanged() from their destructor, so I don't really know how they handle this case at all. Maybe the skia and cairo objects handle it correctly internally?
https://hg.mozilla.org/mozilla-central/rev/2d37465635cb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.