720428 - Runfield is slower with quartz azure canvas

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

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]]

Attachment: Make snapshots work more like the other backends.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make snapshots work more like the other backends.v2

This version fixes lots of bugs

Comment 3

[Matt Woodrow (:mattwoodrow)]

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]]

Attachment: 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)]

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)]

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]]

https://hg.mozilla.org/mozilla-central/rev/3f26b7bee352