Last Comment Bug 688368 - [Azure] Make SourceSurfaceSkia copy-on-write
: [Azure] Make SourceSurfaceSkia copy-on-write
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Matt Woodrow (:mattwoodrow)
:
Mentors:
Depends on:
Blocks: 687187
  Show dependency treegraph
 
Reported: 2011-09-21 22:32 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2011-11-03 08:45 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add Copy-on-write support (11.46 KB, patch)
2011-09-21 22:32 PDT, Matt Woodrow (:mattwoodrow)
joe: review+
Details | Diff | Review
Make SourceSurfaceSkia data surfaces copy-on-write (12.19 KB, patch)
2011-10-31 20:30 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Review

Description Matt Woodrow (:mattwoodrow) 2011-09-21 22:32:50 PDT
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.
Comment 1 Joe Drew (not getting mail) 2011-09-22 13:47:45 PDT
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.
Comment 2 Matt Woodrow (:mattwoodrow) 2011-10-31 20:30:01 PDT
Created attachment 570920 [details] [diff] [review]
Make SourceSurfaceSkia data surfaces copy-on-write
Comment 3 Matt Woodrow (:mattwoodrow) 2011-11-02 12:57:09 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a204fcf29e60
Comment 4 Marco Bonardo [::mak] 2011-11-03 08:45:42 PDT
https://hg.mozilla.org/mozilla-central/rev/a204fcf29e60

Note You need to log in before you can comment on or make changes to this bug.