Closed
Bug 724666
Opened 12 years ago
Closed 12 years ago
Use the CGIOSurfaceContext APIs on Lion
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: BenWa, Assigned: BenWa)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 10 obsolete files)
58.50 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
36.15 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
2.06 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
10.42 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
It used to work but no longer works because of the changes made to the 2d api. Before landing this patch it needs to allow drawing with either Bitmap or IOSurface depending on conditions. We also need to use an MacIOSurfaceImageLayer. This has since been made easier by the refactoring in bug 715785. This patch used to work by reading back the image.
Comment 2•12 years ago
|
||
This doesn't look like the right patch.
Assignee | ||
Comment 3•12 years ago
|
||
Here we go. When I had it working we got canvas performance compare to Safari on Lion. For the best performance we need to pass the IOSurface directly to the layer however.
Attachment #595097 -
Attachment is obsolete: true
Assignee | ||
Comment 4•12 years ago
|
||
Alright picking this bug up. Jeff can you elaborate more on coupling problems caused by using CGBitmapContext? I took a look at we only assume it in 'SourceSurfaceCGBitmapContext::SourceSurfaceCGBitmapContext' as far as I can see and perhaps GetNativeSurface. We can readback from this context by doing a flush, lock, memcpy, unlock.
Assignee | ||
Comment 5•12 years ago
|
||
This now works but only works for the first frame. Any ideas jeff?
Attachment #595118 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 years ago
|
||
This patch is ready to go I think. It adds a pref, will only accelerate canvas of certain sizes (not too small, not too big) and 10.7+. The option to enable this by default isn't off the table since we're early in the cycle but let's review this first and discuss it. It may be easy to enable this for a few days with read back to get testing for the two pieces in isolation.
Assignee: nobody → bgirard
Attachment #644111 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #644409 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 7•12 years ago
|
||
Here's a profile of FishIE with 1000 running at 10 FPS. Once we fix the readback we will likely be Js engine bound instead of canvas bound.
Assignee | ||
Comment 8•12 years ago
|
||
people.mozilla.com/~bgirard/cleopatra?report=61b0e48c6b1c88b4c1699db40cdad6bf7bd6c3df
Assignee | ||
Comment 9•12 years ago
|
||
No more slow readback.
Attachment #644409 -
Attachment is obsolete: true
Attachment #644409 -
Flags: review?(jmuizelaar)
Attachment #644560 -
Flags: review?(jmuizelaar)
Comment 10•12 years ago
|
||
Comment on attachment 644560 [details] [diff] [review] patch + Accelerated composition Review of attachment 644560 [details] [diff] [review]: ----------------------------------------------------------------- Here's some review. ::: gfx/2d/DrawTargetCG.cpp @@ +13,1 @@ > This breaks the ability to build Azure stand-alone. You move what's needed from nsCoreAnimationSupport.h into gfx/2d/ and the plugin code depend on gfx. @@ +97,5 @@ > TemporaryRef<SourceSurface> > DrawTargetCG::Snapshot() > { > if (!mSnapshot) { > + if (mIsIOSurfaceContext) { I'd rather we test the type of the CGContext rather than storing additional state. @@ +869,1 @@ > This decision should be moved into the caller and out of gfx/2d. i.e. the caller should get to decide what kind of surface to use. ::: gfx/2d/SourceSurfaceCG.cpp @@ +394,5 @@ > +SourceSurfaceCGIOSurfaceContext::SourceSurfaceCGIOSurfaceContext(DrawTargetCG *aDrawTarget) > +{ > + CGContextRef cg = (CGContextRef)aDrawTarget->GetNativeSurface(NATIVE_SURFACE_CGCONTEXT_ACCELERATED); > + > + nsRefPtr<nsIOSurface> surf = nsIOSurface::IOSurfaceContextGetSurface(cg); Can't use nsRefPtr here. @@ +402,5 @@ > + > + //mImage = nsIOSurface::CreateImageFromIOSurfaceContext(cg); > + mImage = NULL; > + > + // TODO Composite the IOSurface directly This TODO is already done. ::: gfx/layers/basic/BasicLayerManager.cpp @@ +709,5 @@ > (aLayer->GetContentFlags() & Layer::CONTENT_OPAQUE) && > !transform.HasNonAxisAlignedTransform()) { > > + gfx::Rect opaqueRect = dt->GetTransform().TransformBounds( > + gfx::Rect(bounds.x, bounds.y, bounds.width, bounds.height)); I'd like it more if these changes were in a separate patch. ::: gfx/thebes/gfxPlatformMac.cpp @@ +404,5 @@ > + gfxImageSurface::ImageFormatARGB32); > + // FIXME We don't want to be using this path since we're doing a useless copy, > + // however we want to replace this path to use the IOSurface directly > + // so there's no point in optimizing this. > + // We could fix this by telling gfxImageSurface it owns data. Seems like the comment could be updated.
Attachment #644560 -
Flags: review?(jmuizelaar) → review-
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #644560 -
Attachment is obsolete: true
Attachment #645946 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 12•12 years ago
|
||
This patch diff doesn't capture the moves: thebes/nsCoreAnimationSupport.h -> 2d/QuartzSupport.h thebes/nsCoreAnimationSupport.mm -> 2d/QuartzSupport.mm thebes/nsIOSurface.h -> 2d/MacIOSurface.h
Comment 13•12 years ago
|
||
Comment on attachment 645946 [details] [diff] [review] patch Review of attachment 645946 [details] [diff] [review]: ----------------------------------------------------------------- I would like this much better if the QuartzSupport refactor was a separate patch than everything else.
Assignee | ||
Comment 14•12 years ago
|
||
I'll post actual patch queue but the divides are not very clean.
Assignee | ||
Comment 15•12 years ago
|
||
Down to 1 more new failures: http://philip.html5.org/tests/canvas/suite/tests/2d.path.rect.winding.html We pass a new test case, and we fail one test case by -1 in the red channel only.
Comment 16•12 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #15) > Down to 1 more new failures: > http://philip.html5.org/tests/canvas/suite/tests/2d.path.rect.winding.html > > We pass a new test case, and we fail one test case by -1 in the red channel > only. What results does Safari have?
Assignee | ||
Comment 17•12 years ago
|
||
fail
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #645946 -
Attachment is obsolete: true
Attachment #645946 -
Flags: review?(jmuizelaar)
Attachment #646593 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #646594 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 20•12 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=56480e19f079
Assignee | ||
Comment 21•12 years ago
|
||
Try run canvas turned on: https://tbpl.mozilla.org/?tree=Try&rev=fb80633da546
Comment 22•12 years ago
|
||
Comment on attachment 646593 [details] [diff] [review] Part 1: Refactor nsCoreAnimation support in azure QuartzSupport Review of attachment 646593 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/QuartzSupport.h @@ +4,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef nsCoreAnimationSupport_h__ > +#define nsCoreAnimationSupport_h__ Wrong header name ::: gfx/thebes/nsCoreAnimationSupport.mm @@ +1000,5 @@ > + return ioSurface.forget(); > +} > + > + > +CGContextType GetContextType(CGContextRef ref) I'd say you can move this directly into the DrawTarget. We shouldn't need to lookup this symbol. See the declaration of CGContextSetCTM at the top of DrawTargetCG.cpp
Attachment #646593 -
Flags: review?(jmuizelaar) → review+
Comment 23•12 years ago
|
||
Comment on attachment 646593 [details] [diff] [review] Part 1: Refactor nsCoreAnimation support in azure QuartzSupport Review of attachment 646593 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/QuartzSupport.h @@ +77,5 @@ > + CG_CONTEXT_TYPE_UNKNOWN = 0, > + // These are found by inspection, it's possible they could be changed > + CG_CONTEXT_TYPE_BITMAP = 4, > + CG_CONTEXT_TYPE_IOSURFACE = 8 > +}; This can also be moved to the draw target
Comment 24•12 years ago
|
||
Comment on attachment 646594 [details] [diff] [review] Part 2: Add CGIOSurfaceContext to azure Review of attachment 646594 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/DrawTargetCG.cpp @@ +101,5 @@ > + } else { > + return BACKEND_COREGRAPHICS; > + } > +} > + Please add a comment about how it might be worth splitting DrawTargetCG into sub classes for Bitmap and IOSurface ::: gfx/2d/Makefile.in @@ +58,5 @@ > + $(NULL) > + > +EXPORTS_mozilla/gfx += \ > + QuartzSupport.h \ > + MacIOSurface.h \ Indentation looks weird here ::: gfx/layers/basic/BasicCanvasLayer.cpp @@ +117,5 @@ > BasicCanvasLayer::UpdateSurface(gfxASurface* aDestSurface, Layer* aMaskLayer) > { > if (mDrawTarget) { > mDrawTarget->Flush(); > + mSurface = gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mDrawTarget); Why is this needed now and does it regress performance of non-accelerated? ::: gfx/layers/opengl/CanvasLayerOGL.cpp @@ +297,5 @@ > gl()->ApplyFilterToBoundTexture(mFilter); > > program->Activate(); > + if (mLayerProgram == gl::RGBARectLayerProgramType) { > + program->SetTexCoordMultiplier(mDrawTarget->GetSize().width, mDrawTarget->GetSize().height); Add a comment that this is for the IOSurface ::: gfx/thebes/gfxPlatformMac.cpp @@ +421,3 @@ > > + return surf.forget(); > + } I hope that this get's the lifetimes of things correct.
Attachment #646594 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 25•12 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #24) > Comment on attachment 646594 [details] [diff] [review] > Part 2: Add CGIOSurfaceContext to azure > > Review of attachment 646594 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/2d/DrawTargetCG.cpp > @@ +101,5 @@ > > + } else { > > + return BACKEND_COREGRAPHICS; > > + } > > +} > > + > > Please add a comment about how it might be worth splitting DrawTargetCG into > sub classes for Bitmap and IOSurface > > ::: gfx/2d/Makefile.in > @@ +58,5 @@ > > + $(NULL) > > + > > +EXPORTS_mozilla/gfx += \ > > + QuartzSupport.h \ > > + MacIOSurface.h \ > > Indentation looks weird here > > ::: gfx/layers/basic/BasicCanvasLayer.cpp > @@ +117,5 @@ > > BasicCanvasLayer::UpdateSurface(gfxASurface* aDestSurface, Layer* aMaskLayer) > > { > > if (mDrawTarget) { > > mDrawTarget->Flush(); > > + mSurface = gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mDrawTarget); > > Why is this needed now and does it regress performance of non-accelerated? > When I wrote the patch on mac we used to cache this surface on user data so requerying would return the same surface if the same surface could still be used. But a patch landed a few days ago that changed this. This is needed because we have to create a new surface in the accelerated case because we need to flush and copy to a new surface currently. > ::: gfx/layers/opengl/CanvasLayerOGL.cpp > @@ +297,5 @@ > > gl()->ApplyFilterToBoundTexture(mFilter); > > > > program->Activate(); > > + if (mLayerProgram == gl::RGBARectLayerProgramType) { > > + program->SetTexCoordMultiplier(mDrawTarget->GetSize().width, mDrawTarget->GetSize().height); > > Add a comment that this is for the IOSurface > > ::: gfx/thebes/gfxPlatformMac.cpp > @@ +421,3 @@ > > > > + return surf.forget(); > > + } > > I hope that this get's the lifetimes of things correct.
Assignee | ||
Comment 26•12 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #24) > ::: gfx/thebes/gfxPlatformMac.cpp > @@ +421,3 @@ > > > > + return surf.forget(); > > + } > > I hope that this get's the lifetimes of things correct. I'm not sure what the concern is here since we copy the image data every time.
Assignee | ||
Comment 27•12 years ago
|
||
If this passes I'm ready to land it turned off. They are only a few outstanding issues to turn it on by default but I'm hitting a ton of bit rot so land land land! https://hg.mozilla.org/try/rev/a86dd2e0bdf4
Assignee | ||
Comment 28•12 years ago
|
||
You can use RefPtr on the stack/as a member variable argg. Try 2: https://hg.mozilla.org/try/rev/b0579c969309
Assignee | ||
Comment 29•12 years ago
|
||
can not*
Assignee | ||
Comment 30•12 years ago
|
||
Attachment #646593 -
Attachment is obsolete: true
Attachment #647526 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Attachment #646594 -
Attachment is obsolete: true
Assignee | ||
Comment 32•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1471876c787a https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2d4f38c58a The pref wont work properly until we land part 3 to address review comments.
Whiteboard: [leave open]
Assignee | ||
Comment 33•12 years ago
|
||
Some code got lost in the rebasing and this add additional changes that were needed because of the changes in CanvasLayerOGL.cpp I also look into how safari deals with minimum size. They also enforce alignment in their IOSurface that we don't.
Attachment #647728 -
Flags: review?(jmuizelaar)
Updated•12 years ago
|
Attachment #647728 -
Flags: review?(jmuizelaar) → review+
Comment 34•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1471876c787a https://hg.mozilla.org/mozilla-central/rev/fd2d4f38c58a
Comment 35•12 years ago
|
||
You left a bunch of debugging printfs in part 1. In particular, I'm seeing this spammed in my console: printf("*****Attach*****\n\n\n\n\n"); from gfx/2d/QuartzSupport.mm
Comment 36•12 years ago
|
||
(this is on OSX 10.6)
Comment 37•12 years ago
|
||
Attachment #647966 -
Flags: review?(bgirard)
Assignee | ||
Comment 38•12 years ago
|
||
Comment on attachment 647966 [details] [diff] [review] part 1b: Remove debugging printfs from part 1 Opps, thanks :)
Attachment #647966 -
Flags: review?(bgirard) → review+
Comment 41•12 years ago
|
||
I'm currently getting a build warning for an unused function that Part 2 added here, back in July: { gfx/layers/opengl/CanvasLayerOGL.cpp:161:1: warning: ‘GLuint MakeIOSurfaceTexture(void*, mozilla::gl::GLContext*)’ defined but not used [-Wunused-function] } This function has some platform-specific #ifdeff'ing around it, but aside from that, it's not used anywhere on any platform: https://mxr.mozilla.org/mozilla-central/search?string=MakeIOSurfaceTexture Should it be removed, or will this bug eventually be adding clients of this method?
Assignee | ||
Comment 42•12 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #41) > I'm currently getting a build warning for an unused function that Part 2 > added here, back in July: > { > gfx/layers/opengl/CanvasLayerOGL.cpp:161:1: warning: ‘GLuint > MakeIOSurfaceTexture(void*, mozilla::gl::GLContext*)’ defined but not used > [-Wunused-function] > } > > This function has some platform-specific #ifdeff'ing around it, but aside > from that, it's not used anywhere on any platform: > https://mxr.mozilla.org/mozilla-central/search?string=MakeIOSurfaceTexture > > Should it be removed, or will this bug eventually be adding clients of this > method? No, it's meant to be used by part 3. I didn't think it would take me so long to come back to this.
Assignee | ||
Comment 43•12 years ago
|
||
No regression when running the canvas tests: http://philip.html5.org/tests/canvas/suite/tests/index.2d.html If this try run is good I'm going to land it pref-ed off so we can test this a few days before turning it on: https://tbpl.mozilla.org/?tree=Try&rev=7b5a76faaabe
Attachment #647728 -
Attachment is obsolete: true
Attachment #667767 -
Flags: review+
Assignee | ||
Comment 44•12 years ago
|
||
One more try run to make sure we don't break the build on windows: https://tbpl.mozilla.org/?tree=Try&rev=52b0020f49db
Assignee | ||
Comment 45•12 years ago
|
||
Attachment #667767 -
Attachment is obsolete: true
Attachment #667951 -
Flags: review+
Assignee | ||
Comment 46•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e3387058f1
Target Milestone: --- → mozilla18
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
Comment 47•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c1e3387058f1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 48•11 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #42) > (In reply to Daniel Holbert [:dholbert] from comment #41) > > I'm currently getting a build warning for an unused function that Part 2 > > added here, back in July: > > { > > gfx/layers/opengl/CanvasLayerOGL.cpp:161:1: warning: ‘GLuint > > MakeIOSurfaceTexture(void*, mozilla::gl::GLContext*)’ defined but not used > > [-Wunused-function] > > } [...] > No, it's meant to be used by part 3. I didn't think it would take me so long > to come back to this. Looks like part 3 only uses it on mac (#ifdef XP_MAC), so we still spam this build warning everywhere else. Filed bug 831023 for that.
You need to log in
before you can comment on or make changes to this bug.
Description
•