692879 - Implement CoreGraphics Azure backend

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: First implementionation

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add a initializer that takes an existing CGContextRef

Comment 3

[Matt Woodrow (:mattwoodrow)]

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

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

Attachment: Mostly finished CG backend. This passes tests on try.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

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

Attachment: Done version

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: Adjust the reftests so this passes

Comment 9

[Matt Woodrow (:mattwoodrow)]

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

Attachment: Handle failing tests on OS X 10.5

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Attachment: Enable azure canvas on OS X

Comment 12

[Matt Brubeck (:mbrubeck)]

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 13

[Matt Brubeck (:mbrubeck)]

Re-landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c43d0ddbc1c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/68c24abeab95
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f62e9293d8

Comment 14

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/c43d0ddbc1c7
https://hg.mozilla.org/mozilla-central/rev/68c24abeab95
https://hg.mozilla.org/mozilla-central/rev/f9f62e9293d8
https://hg.mozilla.org/mozilla-central/rev/8b4aae361875

Comment 15

[Julian Viereck]

I see various rendering breakage turning on the new Azure backend on OSX: 

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