Last Comment Bug 720428 - Runfield is slower with quartz azure canvas
: Runfield is slower with quartz azure canvas
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-23 10:52 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-01-31 09:39 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make snapshots work more like the other backends. (9.84 KB, patch)
2012-01-29 15:54 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
Make snapshots work more like the other backends.v2 (18.61 KB, patch)
2012-01-30 07:57 PST, Jeff Muizelaar [:jrmuizel]
matt.woodrow: review+
Details | Diff | Review
Make snapshots work more like the other backends.v3 (16.06 KB, patch)
2012-01-30 13:13 PST, Jeff Muizelaar [:jrmuizel]
bas: review+
joe: review+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2012-01-23 10:52:21 PST
This is because of the large use of getImageData()

The old path:
 - data = CGContextGetData()
 - copy that data into an image surface
 - unpremultiply into the destination

The new path:
 Snapshot()
 - CGImage image = CGBitmapContextCreateImage
 DataSourceSurface()
 - Draw CGImage to CGBitmapContext
 - CGContextGetData()
 - unpremultiply into the destination

The extra CGImage is costing us a bunch of nasty vm churn.

One possible solution, would be to keep the CGContext around in the SoureSurfaceCG but that's sort of unappealing to me.
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-01-29 15:54:40 PST
Created attachment 592552 [details] [diff] [review]
Make snapshots work more like the other backends.
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-01-30 07:57:35 PST
Created attachment 592719 [details] [diff] [review]
Make snapshots work more like the other backends.v2

This version fixes lots of bugs
Comment 3 Matt Woodrow (:mattwoodrow) 2012-01-30 13:13:06 PST
Comment on attachment 592719 [details] [diff] [review]
Make snapshots work more like the other backends.v2

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

::: gfx/2d/DrawTargetCG.cpp
@@ +122,5 @@
> +        // mSnapshot can be treated as independent of this DrawTarget since we know
> +        // this DrawTarget won't change again.
> +        deathGrip->MarkIndependent();
> +        // mSnapshot will be cleared now.
> +  }

You could clear mSnapshot here instead, and avoid needing to do this.

Indenting is a bit broken too.

::: gfx/2d/SourceSurfaceCG.cpp
@@ +379,5 @@
> +  if (mDrawTarget) {
> +    size_t stride = CGBitmapContextGetBytesPerRow(mCg);
> +    size_t height = CGBitmapContextGetHeight(mCg);
> +    //XXX: infalliable malloc?
> +    mData = malloc(stride * height);

Fallible malloc would be nice here :)
Comment 4 Jeff Muizelaar [:jrmuizel] 2012-01-30 13:13:41 PST
Created attachment 592840 [details] [diff] [review]
Make snapshots work more like the other backends.v3

Simplify based on work done in the cairo backend.
Comment 5 Joe Drew (not getting mail) 2012-01-30 13:27:09 PST
Comment on attachment 592840 [details] [diff] [review]
Make snapshots work more like the other backends.v3

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

::: gfx/2d/DrawTargetCG.cpp
@@ +108,4 @@
>  
>  
>  
> +DrawTargetCG::DrawTargetCG() : mSnapshot(NULL)

newline

@@ +198,4 @@
>          //XXX: The size here is in default user space units, of the layer relative to the graphics context.
>          // is the clip bounds still correct if, for example, we have a scale applied to the context?
>          mLayer = CGLayerCreateWithContext(baseCg, mClipBounds.size, NULL);
> +        //XXX: if the size is 0x0 we get a NULL CGContext back from GetContext

file a bug

@@ +231,4 @@
>                             const DrawSurfaceOptions &aSurfOptions,
>                             const DrawOptions &aDrawOptions)
>  {
> +  MarkChanged();

It might be a good idea to make an RAII class that does CGContext{Save,Restore}GState and this.

::: gfx/2d/DrawTargetCG.h
@@ +41,5 @@
>  #include "Rect.h"
>  #include "PathCG.h"
> +#include "SourceSurfaceCG.h"
> +
> +class SourceSurfaceCGBitmapContext;

Why both?

::: gfx/2d/SourceSurfaceCG.cpp
@@ +342,5 @@
> +
> +      colorSpace = CGColorSpaceCreateDeviceRGB();
> +      bitinfo = kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host;
> +      bitsPerComponent = 8;
> +      bitsPerPixel = 32;

These should be initialized with their declaration.

@@ +346,5 @@
> +      bitsPerPixel = 32;
> +
> +      dataProvider = CGDataProviderCreateWithData (mData,
> +                                               mData,
> +					       mSize.height * mStride,

incorrectly indented

@@ +376,5 @@
> +void
> +SourceSurfaceCGBitmapContext::DrawTargetWillChange()
> +{
> +  if (mDrawTarget) {
> +    size_t stride = CGBitmapContextGetBytesPerRow(mCg);

This probably doesn't need to be if (mDrawTarget).

@@ +391,5 @@
> +SourceSurfaceCGBitmapContext::~SourceSurfaceCGBitmapContext()
> +{
> +  if (!mImage && !mCg)
> +    // neither mImage or mCg owns the data
> +    free(mData);

braces, all over the place

::: gfx/2d/SourceSurfaceCG.h
@@ +116,5 @@
> +  virtual SurfaceType GetType() const { return SURFACE_COREGRAPHICS_CGCONTEXT; }
> +  virtual IntSize GetSize() const;
> +  virtual SurfaceFormat GetFormat() const { return FORMAT_B8G8R8A8; }
> +
> +  CGImageRef GetImage() { EnsureImage(); return mImage; }

This isn't a simple getter, so it should probably go in the .cpp file.
Comment 6 Bas Schouten (:bas.schouten) 2012-01-30 14:44:21 PST
Comment on attachment 592840 [details] [diff] [review]
Make snapshots work more like the other backends.v3

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

::: gfx/2d/DrawTargetCG.cpp
@@ +170,5 @@
> +  else if (aSurface->GetType() == SURFACE_COREGRAPHICS_CGCONTEXT)
> +    return static_cast<SourceSurfaceCGBitmapContext*>(aSurface)->GetImage();
> +  else if (aSurface->GetType() == SURFACE_DATA)
> +    return static_cast<DataSourceSurfaceCG*>(aSurface)->GetImage();
> +  assert(0);

nit: Curly braces

@@ +198,4 @@
>          //XXX: The size here is in default user space units, of the layer relative to the graphics context.
>          // is the clip bounds still correct if, for example, we have a scale applied to the context?
>          mLayer = CGLayerCreateWithContext(baseCg, mClipBounds.size, NULL);
> +        //XXX: if the size is 0x0 we get a NULL CGContext back from GetContext

nit: Does this belong inside this patch?

::: gfx/2d/SourceSurfaceCG.cpp
@@ +346,5 @@
> +      bitsPerPixel = 32;
> +
> +      dataProvider = CGDataProviderCreateWithData (mData,
> +                                               mData,
> +					       mSize.height * mStride,

nit: Tabs

::: gfx/2d/SourceSurfaceCG.h
@@ +123,5 @@
> +
> +  virtual int32_t Stride() { return mStride; }
> +
> +private:
> +  //XXX: do the other backends friend their DrawTarget?

They do.
Comment 7 Jeff Muizelaar [:jrmuizel] 2012-01-30 23:30:36 PST
https://hg.mozilla.org/mozilla-central/rev/3f26b7bee352

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