Closed Bug 688368 Opened 8 years ago Closed 8 years ago

[Azure] Make SourceSurfaceSkia copy-on-write

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Add Copy-on-write support (obsolete) — Splinter Review
Instead of always copying pixel data from DrawTargets into SourceSurfaces, we should allow these to share memory until the DrawTarget is modified.
Attachment #561659 - Flags: review?(bas.schouten)
Attachment #561659 - Flags: review?(joe)
Comment on attachment 561659 [details] [diff] [review]
Add Copy-on-write support

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +190,3 @@
>      return NULL;
>    }
> +  source->mDrawTarget = this;

I don't think this is still necessary, is it?

@@ +714,5 @@
> +void
> +DrawTargetSkia::RemoveSnapshot(SourceSurfaceSkia* aSnapshot)
> +{
> +  std::vector<SourceSurfaceSkia*>::iterator iter = std::find(mSnapshots.begin(), mSnapshots.end(), aSnapshot);
> +  mSnapshots.erase(iter);

Should this be if (iter != mSnapshots.end())? I thought that was causing a crash before.

::: gfx/2d/SourceSurfaceSkia.h
@@ +67,5 @@
>                      SurfaceFormat aFormat);
>  
> +  bool InitWithBitmap(const SkBitmap& aBitmap,
> +                      SurfaceFormat aFormat,
> +                      DrawTargetSkia* aOwner);

You should probably add documentation saying that if aOwner is NULL here, we'll copy the data, and otherwise we'll just copy-on-write reference it.

@@ +84,5 @@
>    SkBitmap mBitmap;
>    SurfaceFormat mFormat;
>    IntSize mSize;
>    int32_t mStride;
> +  RefPtr<DrawTargetSkia> mDrawTarget;

I made this a raw pointer and made sure that I called MarkIndependent() in the destructor of both SourceSurfaceCairo and DrawTargetCairo; I was a little worried about things being kept alive longer than they should be. I haven't thought long about what's right, though.
Attachment #561659 - Flags: review?(joe) → review+
Attachment #561659 - Attachment is obsolete: true
Attachment #561659 - Flags: review?(bas.schouten)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a204fcf29e60
Assignee: nobody → matt.woodrow
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/a204fcf29e60
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.