Last Comment Bug 692879 - Implement CoreGraphics Azure backend
: Implement CoreGraphics Azure backend
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla12
Assigned To: Jeff Muizelaar [:jrmuizel]
:
:
Mentors:
Depends on: 729145 729850 752341 898267
Blocks: 379992 525650 726764 839383
  Show dependency treegraph
 
Reported: 2011-10-07 12:12 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2013-07-25 21:22 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First implementionation (65.65 KB, patch)
2011-11-16 17:28 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Add a initializer that takes an existing CGContextRef (66.07 KB, patch)
2011-11-16 17:45 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Mostly finished CG backend. This passes tests on try. (86.01 KB, patch)
2012-01-04 18:31 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Done version (88.22 KB, patch)
2012-01-09 13:40 PST, Jeff Muizelaar [:jrmuizel]
matt.woodrow: review+
Details | Diff | Splinter Review
Adjust the reftests so this passes (8.60 KB, patch)
2012-01-09 13:44 PST, Jeff Muizelaar [:jrmuizel]
matt.woodrow: review+
Details | Diff | Splinter Review
Handle failing tests on OS X 10.5 (2.44 KB, patch)
2012-01-16 14:01 PST, Jeff Muizelaar [:jrmuizel]
matt.woodrow: review+
Details | Diff | Splinter Review
Enable azure canvas on OS X (616 bytes, patch)
2012-01-16 14:25 PST, Jeff Muizelaar [:jrmuizel]
matt.woodrow: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2011-10-07 12:12:29 PDT

    
Comment 1 Jeff Muizelaar [:jrmuizel] 2011-11-16 17:28:48 PST
Created attachment 575052 [details] [diff] [review]
First implementionation
Comment 2 Jeff Muizelaar [:jrmuizel] 2011-11-16 17:45:47 PST
Created attachment 575056 [details] [diff] [review]
Add a initializer that takes an existing CGContextRef
Comment 3 Matt Woodrow (:mattwoodrow) 2011-11-16 17:49:59 PST
Comment on attachment 575052 [details] [diff] [review]
First implementionation

Review of attachment 575052 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +4363,5 @@
>    data.mDrawTarget = mTarget;
> +  } else {
> +    surf = (gfxASurface*)mTarget->GetNativeSurface(NATIVE_SURFACE_GFXASURFACE);
> +    data.mSurface = surf;
> +  }

We shouldn't need this any more, all the LayerManager implementations should be able to handle a DrawTarget as long as gfxPlatform::GetThebesSurfaceForDrawTarget works.

::: gfx/2d/Factory.cpp
@@ +157,5 @@
> +  case NATIVE_FONT_MAC_FONT_FACE:
> +    {
> +      return new ScaledFontCG(static_cast<CGFontRef>(aNativeFont.mFont), aSize);
> +    }
> +#endif

Maybe make this a pref too so the skia/CG choice isn't compile time.

::: gfx/2d/Types.h
@@ +82,4 @@
>  
>  enum BackendType
>  {
> +  BACKEND_DEFAULT,

This got removed in favor of making backend decisions in gfxPlatform

::: gfx/thebes/gfxPlatform.cpp
@@ +538,5 @@
>    nsRefPtr<gfxImageSurface> image =
>      new gfxImageSurface(data->GetData(), gfxIntSize(size.width, size.height),
>                          data->Stride(), format);
> +
> +  image->SetData(&kDrawSourceSurface, data.forget().drop(), DataSourceSurfaceDestroy);

Separate patch!

::: gfx/thebes/gfxPlatformMac.cpp
@@ +143,4 @@
>  RefPtr<DrawTarget>
>  gfxPlatformMac::CreateOffscreenDrawTarget(const IntSize& aSize, SurfaceFormat aFormat)
>  {
> +  return Factory::CreateDrawTarget(BACKEND_COREGRAPHICS, aSize, aFormat);

Pref please :)
Comment 4 Matt Woodrow (:mattwoodrow) 2011-11-16 19:04:18 PST
Comment on attachment 575056 [details] [diff] [review]
Add a initializer that takes an existing CGContextRef

Review of attachment 575056 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetCG.cpp
@@ +162,5 @@
> +class UnboundnessFixer
> +{
> +    CGRect clipBounds;
> +    CGLayerRef layer;
> +    CGContextRef cg;

mClipBounds etc?

@@ +166,5 @@
> +    CGContextRef cg;
> +  public:
> +    UnboundnessFixer() : cg(NULL) {}
> +
> +    CGContextRef Check(CGContextRef baseCg, CompositionOp blend)

I currently have a thing for RAII classes, so I'd much prefer if this was the constructor, and Fix() was the dtor.

@@ +208,5 @@
>  {
>    CGImageRef image;
>    CGImageRef subimage = NULL;
> +  if (aSurface->GetType() == SURFACE_COREGRAPHICS_IMAGE) {
> +    CGContextSaveGState(mCg);

Do we have an idea of how expensive save/restore is with Quartz?

We could instead always keep the CGContext values set to the Azure defaults, and then only set/unset values that are different. Maybe a stack helper?

@@ +253,5 @@
> +
> +class GradientStopsCG : public GradientStops
> +{
> +  public:
> +  //XXX: The skia backend uses a vector and passes in aNumStops. It should do better

In what way?

@@ +294,5 @@
> +  return new GradientStopsCG(aStops, aNumStops);
> +}
> +
> +static void
> +DrawGradient(CGContextRef cg, const Pattern &aPattern)

aCG?

@@ +299,5 @@
> +{
> +  if (aPattern.GetType() == PATTERN_LINEAR_GRADIENT) {
> +    const LinearGradientPattern& pat = static_cast<const LinearGradientPattern&>(aPattern);
> +    GradientStopsCG *stops = static_cast<GradientStopsCG*>(pat.mStops.get());
> +    // XXX: we should take the m out of the properties of LinearGradientPatterns

Disagree.

@@ +319,5 @@
> +    CGFloat startRadius = pat.mRadius1;
> +    CGPoint endCenter   = { pat.mCenter2.x, pat.mCenter2.y };
> +    CGFloat endRadius   = pat.mRadius2;
> +
> +    //XXX: are there degenerate radial gradients that we should avoid drawing?

Same centres and same radii?

@@ +343,5 @@
> +releaseInfo(void *info)
> +{
> +  CGImageRef image = static_cast<CGImageRef>(info);
> +  CGImageRelease(image);
> +}

Naming consistency--

@@ +368,5 @@
> +  CGImageRef image = static_cast<SourceSurfaceCG*>(pat.mSurface.get())->GetImage();
> +  CGFloat xStep, yStep;
> +  switch (pat.mExtendMode) {
> +    case EXTEND_CLAMP:
> +      // The 1 << 22 comes from Webkit see comments there for more info

Copy them here? Or at least reference the file/line.

@@ +457,5 @@
> +
> +  UnboundnessFixer fixer;
> +  CGContextRef cg = fixer.Check(mCg, aDrawOptions.mCompositionOp);
> +  CGContextSetAlpha(mCg, aDrawOptions.mAlpha);
> +  CGContextSetBlendMode(mCg, ToBlendMode(aDrawOptions.mCompositionOp));

An RAII stack class to avoid repeating all of these would be nice. Just sayin'

@@ +891,5 @@
> +                          gfxASurface::ImageFormatARGB32 : 
> +                          gfxASurface::ImageFormatRGB24);
> +  }
> +  return mImageSurface.get();
> +}

I don't think we want this at all anymore. gfxPlatform::GetThebesSurfaceForDrawTarget is used for this conversion.

Then we can get rid of the nsRefPtr.

@@ +900,5 @@
> +  CGContextSaveGState(mCg);
> +
> +  CGContextBeginPath(mCg);
> +  if (aPath->GetBackendType() != BACKEND_COREGRAPHICS) {
> +    return;

As mentioned for skia, maybe we should be asserting for these cases.

::: gfx/2d/DrawTargetCG.h
@@ +126,5 @@
>                          const DrawOptions &aOptions = DrawOptions());
>  
>  
> +  //XXX: why do we take a reference to SurfaceFormat?
> +  bool Init(const IntSize &aSize, SurfaceFormat&);

I don't know, why do you?
Comment 5 Jeff Muizelaar [:jrmuizel] 2012-01-04 18:31:17 PST
Created attachment 585964 [details] [diff] [review]
Mostly finished CG backend. This passes tests on try.
Comment 6 Jeff Muizelaar [:jrmuizel] 2012-01-09 13:36:09 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)

> 
> @@ +166,5 @@
> > +    CGContextRef cg;
> > +  public:
> > +    UnboundnessFixer() : cg(NULL) {}
> > +
> > +    CGContextRef Check(CGContextRef baseCg, CompositionOp blend)
> 
> I currently have a thing for RAII classes, so I'd much prefer if this was
> the constructor, and Fix() was the dtor.

Yeah, I can see that, in this case my preference was for explicit.

> 
> @@ +208,5 @@
> >  {
> >    CGImageRef image;
> >    CGImageRef subimage = NULL;
> > +  if (aSurface->GetType() == SURFACE_COREGRAPHICS_IMAGE) {
> > +    CGContextSaveGState(mCg);
> 
> Do we have an idea of how expensive save/restore is with Quartz?
> 
> We could instead always keep the CGContext values set to the Azure defaults,
> and then only set/unset values that are different. Maybe a stack helper?

I believe it is quite cheap, so I'll avoid this until it shows up in profiles.

> 
> @@ +253,5 @@
> > +
> > +class GradientStopsCG : public GradientStops
> > +{
> > +  public:
> > +  //XXX: The skia backend uses a vector and passes in aNumStops. It should do better
> 
> In what way?

By using the size() instead of passing in aNumStops.

> 
> @@ +294,5 @@
> > +  return new GradientStopsCG(aStops, aNumStops);
> > +}
> > +
> > +static void
> > +DrawGradient(CGContextRef cg, const Pattern &aPattern)
> 
> aCG?

I really dislike the look of aCg. I think ignoring the convention will be ok in this case.

> 
> @@ +299,5 @@
> > +{
> > +  if (aPattern.GetType() == PATTERN_LINEAR_GRADIENT) {
> > +    const LinearGradientPattern& pat = static_cast<const LinearGradientPattern&>(aPattern);
> > +    GradientStopsCG *stops = static_cast<GradientStopsCG*>(pat.mStops.get());
> > +    // XXX: we should take the m out of the properties of LinearGradientPatterns
> 
> Disagree.

The point of the 'm' is to distinguish locals from members in member functions. LinearGradientPattern doesn't have any need for this. For example, we use .x instead of .mX on Point()

> 
> @@ +319,5 @@
> > +    CGFloat startRadius = pat.mRadius1;
> > +    CGPoint endCenter   = { pat.mCenter2.x, pat.mCenter2.y };
> > +    CGFloat endRadius   = pat.mRadius2;
> > +
> > +    //XXX: are there degenerate radial gradients that we should avoid drawing?
> 
> Same centres and same radii?

Not sure.

> 
> @@ +343,5 @@
> > +releaseInfo(void *info)
> > +{
> > +  CGImageRef image = static_cast<CGImageRef>(info);
> > +  CGImageRelease(image);
> > +}
> 
> Naming consistency--
> 
> @@ +368,5 @@
> > +  CGImageRef image = static_cast<SourceSurfaceCG*>(pat.mSurface.get())->GetImage();
> > +  CGFloat xStep, yStep;
> > +  switch (pat.mExtendMode) {
> > +    case EXTEND_CLAMP:
> > +      // The 1 << 22 comes from Webkit see comments there for more info
> 
> Copy them here? Or at least reference the file/line.
> 
> @@ +457,5 @@
> > +
> > +  UnboundnessFixer fixer;
> > +  CGContextRef cg = fixer.Check(mCg, aDrawOptions.mCompositionOp);
> > +  CGContextSetAlpha(mCg, aDrawOptions.mAlpha);
> > +  CGContextSetBlendMode(mCg, ToBlendMode(aDrawOptions.mCompositionOp));
> 
> An RAII stack class to avoid repeating all of these would be nice. Just
> sayin'

:) I'm going to stick with how it is for now. I might get sick of it later.


> @@ +900,5 @@
> > +  CGContextSaveGState(mCg);
> > +
> > +  CGContextBeginPath(mCg);
> > +  if (aPath->GetBackendType() != BACKEND_COREGRAPHICS) {
> > +    return;
> 
> As mentioned for skia, maybe we should be asserting for these cases.
> 
> ::: gfx/2d/DrawTargetCG.h
> @@ +126,5 @@
> >                          const DrawOptions &aOptions = DrawOptions());
> >  
> >  
> > +  //XXX: why do we take a reference to SurfaceFormat?
> > +  bool Init(const IntSize &aSize, SurfaceFormat&);
> 
> I don't know, why do you?

Nope.
Comment 7 Jeff Muizelaar [:jrmuizel] 2012-01-09 13:40:32 PST
Created attachment 587110 [details] [diff] [review]
Done version
Comment 8 Jeff Muizelaar [:jrmuizel] 2012-01-09 13:44:31 PST
Created attachment 587116 [details] [diff] [review]
Adjust the reftests so this passes
Comment 9 Matt Woodrow (:mattwoodrow) 2012-01-09 16:08:42 PST
Comment on attachment 587110 [details] [diff] [review]
Done version

Review of attachment 587110 [details] [diff] [review]:
-----------------------------------------------------------------

Looks given, especially since it passes tests.

More than I can really review effectively, we can land this as is and iterate on it as needed.

::: gfx/2d/DrawTargetCG.cpp
@@ +580,5 @@
> +
> +  CGContextConcatCTM(cg, GfxMatrixToCGAffineTransform(mTransform));
> +
> +  if (aPath->GetBackendType() != BACKEND_COREGRAPHICS) {
> +    return;

I wonder if we should make these a hard assert? Confusing backend types isn't allowed in the spec afaik

@@ +588,5 @@
> +  const PathCG *cgPath = static_cast<const PathCG*>(aPath);
> +  CGContextAddPath(cg, cgPath->GetPath());
> +
> +  SetStrokeOptions(cg, aStrokeOptions);
> +  //XXX: handle dashing

This is done!

::: gfx/2d/DrawTargetCG.h
@@ +110,5 @@
> +    for (int i=0; i<aStrokeOptions.mDashLength; i++) {
> +      dashes[i] = aStrokeOptions.mDashPattern[i];
> +    }
> +    CGContextSetLineDash(cg, aStrokeOptions.mDashOffset, dashes, aStrokeOptions.mDashLength);
> +    delete dashes;

delete []

@@ +190,5 @@
> +  //PathCG mCurrentPath;
> +
> +  // needed for thebes interop
> +  // XXX: using nsRefPtr here is really BAD
> +  nsRefPtr<gfxImageSurface> mImageSurface;

Where? I can't see any other references to this. Don't let Bas see you including thebes headers into Azure!

::: gfx/2d/ScaledFontCG.h
@@ +45,5 @@
> +
> +namespace mozilla {
> +namespace gfx {
> +
> +class ScaledFontCG : public ScaledFont

This can go :)

::: gfx/2d/SourceSurfaceCG.cpp
@@ +64,4 @@
>  TemporaryRef<DataSourceSurface>
>  SourceSurfaceCG::GetDataSurface()
>  {
> +  //XXX: we should be more disaplined about who takes a reference and where

disciplined

Would it make more sense for the DataSourceSurfaceCG ctor to take the reference instead?

::: gfx/thebes/gfxPlatformMac.cpp
@@ +163,4 @@
>  bool
>  gfxPlatformMac::SupportsAzure(BackendType& aBackend)
>  {
> +  aBackend = BACKEND_COREGRAPHICS;

We need to sort something out here. Maybe a pref?
Comment 10 Jeff Muizelaar [:jrmuizel] 2012-01-16 14:01:49 PST
Created attachment 588997 [details] [diff] [review]
Handle failing tests on OS X 10.5
Comment 11 Jeff Muizelaar [:jrmuizel] 2012-01-16 14:25:24 PST
Created attachment 589014 [details] [diff] [review]
Enable azure canvas on OS X
Comment 12 Matt Brubeck (:mbrubeck) 2012-01-17 10:16:16 PST
Landed on inbound but backed out (along with the other patches it landed with):
https://hg.mozilla.org/integration/mozilla-inbound/rev/52f52eb53549

because one of the changes caused reftests to fail at startup with "REFTEST TEST-UNEXPECTED-FAIL | | EXCEPTION: SyntaxError: missing ) in parenthetical":
https://tbpl.mozilla.org/php/getParsedLog.php?id=8612031&tree=Mozilla-Inbound
Comment 15 Julian Viereck 2012-01-23 23:33:07 PST
I see various rendering breakage turning on the new Azure backend on OSX: 

    https://bugzilla.mozilla.org/show_bug.cgi?id=719417

Note You need to log in before you can comment on or make changes to this bug.