Closed
Bug 720428
Opened 12 years ago
Closed 12 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•12 years ago
|
||
Attachment #592552 -
Flags: review?(matt.woodrow)
Reporter | ||
Comment 2•12 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•12 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•12 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•12 years ago
|
Attachment #592840 -
Flags: review?(joe)
Comment 5•12 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•12 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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3f26b7bee352
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•12 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
•