The default bug view has changed. See this FAQ.

[Azure] Make SourceSurfaceSkia copy-on-write

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

unspecified
mozilla10
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 561659 [details] [diff] [review]
Add Copy-on-write support

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)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 2

6 years ago
Created attachment 570920 [details] [diff] [review]
Make SourceSurfaceSkia data surfaces copy-on-write
(Assignee)

Updated

6 years ago
Attachment #561659 - Attachment is obsolete: true
Attachment #561659 - Flags: review?(bas.schouten)
(Assignee)

Comment 3

6 years ago
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
Last Resolved: 6 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.