Closed Bug 724666 Opened 12 years ago Closed 12 years ago

Use the CGIOSurfaceContext APIs on Lion

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

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.
Attached patch WIP (obsolete) — Splinter Review
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.
This doesn't look like the right patch.
Attached patch WIP (obsolete) — Splinter Review
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
Blocks: 772148
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.
Attached patch WIP rebased (obsolete) — Splinter Review
This now works but only works for the first frame. Any ideas jeff?
Attachment #595118 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
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)
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.
people.mozilla.com/~bgirard/cleopatra?report=61b0e48c6b1c88b4c1699db40cdad6bf7bd6c3df
Attached patch patch + Accelerated composition (obsolete) — Splinter Review
No more slow readback.
Attachment #644409 - Attachment is obsolete: true
Attachment #644409 - Flags: review?(jmuizelaar)
Attachment #644560 - Flags: review?(jmuizelaar)
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-
Attached patch patch (obsolete) — Splinter Review
Attachment #644560 - Attachment is obsolete: true
Attachment #645946 - Flags: review?(jmuizelaar)
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 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.
I'll post actual patch queue but the divides are not very clean.
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.
(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?
fail
Attachment #645946 - Attachment is obsolete: true
Attachment #645946 - Flags: review?(jmuizelaar)
Attachment #646593 - Flags: review?(jmuizelaar)
Attachment #646594 - Flags: review?(jmuizelaar)
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 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 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+
(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.
(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.
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
You can use RefPtr on the stack/as a member variable argg.
Try 2:
https://hg.mozilla.org/try/rev/b0579c969309
can not*
Carry forward r+
Attachment #647527 - Flags: review+
Attachment #646594 - Attachment is obsolete: true
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]
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)
Attachment #647728 - Flags: review?(jmuizelaar) → review+
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
(this is on OSX 10.6)
Comment on attachment 647966 [details] [diff] [review]
part 1b: Remove debugging printfs from part 1

Opps, thanks :)
Attachment #647966 - Flags: review?(bgirard) → review+
Blocks: 786626
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?
(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.
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+
One more try run to make sure we don't break the build on windows:
https://tbpl.mozilla.org/?tree=Try&rev=52b0020f49db
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/c1e3387058f1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 799326
Depends on: 801821
Depends on: 801962
Depends on: 805002
Blocks: 831023
(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.