Use the CGIOSurfaceContext APIs on Lion

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Blocks 1 bug)

unspecified
mozilla18
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 10 obsolete attachments)

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
Assignee

Description

8 years ago
No description provided.
Assignee

Comment 1

8 years ago
Posted 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.
Assignee

Comment 3

8 years ago
Posted 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
Assignee

Comment 4

7 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

7 years ago
Posted 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
Assignee

Comment 6

7 years ago
Posted 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)
Assignee

Comment 7

7 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

7 years ago
people.mozilla.com/~bgirard/cleopatra?report=61b0e48c6b1c88b4c1699db40cdad6bf7bd6c3df
Assignee

Comment 9

7 years ago
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-
Assignee

Comment 11

7 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #644560 - Attachment is obsolete: true
Attachment #645946 - Flags: review?(jmuizelaar)
Assignee

Comment 12

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

7 years ago
I'll post actual patch queue but the divides are not very clean.
Assignee

Comment 15

7 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.
(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

7 years ago
fail
Assignee

Comment 18

7 years ago
Attachment #645946 - Attachment is obsolete: true
Attachment #645946 - Flags: review?(jmuizelaar)
Attachment #646593 - Flags: review?(jmuizelaar)
Assignee

Comment 19

7 years ago
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+
Assignee

Comment 25

7 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

7 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

7 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

7 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

7 years ago
can not*
Assignee

Comment 31

7 years ago
Carry forward r+
Attachment #647527 - Flags: review+
Assignee

Updated

7 years ago
Attachment #646594 - Attachment is obsolete: true
Assignee

Comment 32

7 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

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

Comment 38

7 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+
Assignee

Updated

7 years ago
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?
Assignee

Comment 42

7 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

7 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

7 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

Updated

7 years ago
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/c1e3387058f1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Assignee

Updated

7 years ago
Depends on: 799326

Updated

7 years ago
Depends on: 801821

Updated

7 years ago
Depends on: 801962
Depends on: 805002
(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.