Closed
Bug 720428
Opened 13 years ago
Closed 13 years ago
Runfield is slower with quartz azure canvas
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jrmuizel, Unassigned)
Details
Attachments
(1 file, 2 obsolete files)
16.06 KB,
patch
|
bas.schouten
:
review+
joe
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•13 years ago
|
||
Attachment #592552 -
Flags: review?(matt.woodrow)
Reporter | ||
Comment 2•13 years ago
|
||
This version fixes lots of bugs
Attachment #592552 -
Attachment is obsolete: true
Attachment #592552 -
Flags: review?(matt.woodrow)
Attachment #592719 -
Flags: review?(matt.woodrow)
Attachment #592719 -
Flags: review?(bas.schouten)
Comment 3•13 years ago
|
||
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 :)
Attachment #592719 -
Flags: review?(matt.woodrow) → review+
Reporter | ||
Comment 4•13 years ago
|
||
Simplify based on work done in the cairo backend.
Attachment #592719 -
Attachment is obsolete: true
Attachment #592719 -
Flags: review?(bas.schouten)
Attachment #592840 -
Flags: review?(matt.woodrow)
Attachment #592840 -
Flags: review?(bas.schouten)
Reporter | ||
Updated•13 years ago
|
Attachment #592840 -
Flags: review?(joe)
Comment 5•13 years ago
|
||
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.
Attachment #592840 -
Flags: review?(joe) → review+
Comment 6•13 years ago
|
||
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.
Attachment #592840 -
Flags: review?(bas.schouten) → review+
Reporter | ||
Comment 7•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•13 years ago
|
Attachment #592840 -
Flags: review?(matt.woodrow)
You need to log in
before you can comment on or make changes to this bug.
Description
•