Implement CoreGraphics Azure backend

RESOLVED FIXED in mozilla12

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla12
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

6 years ago
Blocks: 525650
(Assignee)

Updated

6 years ago
Blocks: 379992
(Assignee)

Comment 1

6 years ago
Created attachment 575052 [details] [diff] [review]
First implementionation
Attachment #575052 - Flags: review?(matt.woodrow)
(Assignee)

Comment 2

6 years ago
Created attachment 575056 [details] [diff] [review]
Add a initializer that takes an existing CGContextRef
Attachment #575052 - Attachment is obsolete: true
Attachment #575052 - Flags: review?(matt.woodrow)
Attachment #575056 - Flags: review?(matt.woodrow)
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 :)
Attachment #575052 - Attachment is obsolete: false
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?
(Assignee)

Comment 5

6 years ago
Created attachment 585964 [details] [diff] [review]
Mostly finished CG backend. This passes tests on try.
Attachment #575052 - Attachment is obsolete: true
Attachment #575056 - Attachment is obsolete: true
Attachment #575056 - Flags: review?(matt.woodrow)
(Assignee)

Comment 6

6 years ago
(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.
(Assignee)

Comment 7

6 years ago
Created attachment 587110 [details] [diff] [review]
Done version
Attachment #585964 - Attachment is obsolete: true
Attachment #587110 - Flags: review?(matt.woodrow)
(Assignee)

Comment 8

6 years ago
Created attachment 587116 [details] [diff] [review]
Adjust the reftests so this passes
Attachment #587116 - Flags: review?(matt.woodrow)
Attachment #587116 - Flags: review?(matt.woodrow) → review+
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?
Attachment #587110 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 10

6 years ago
Created attachment 588997 [details] [diff] [review]
Handle failing tests on OS X 10.5
Attachment #588997 - Flags: review?(matt.woodrow)
Attachment #588997 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 589014 [details] [diff] [review]
Enable azure canvas on OS X
Attachment #589014 - Flags: review?(matt.woodrow)
Attachment #589014 - Flags: review?(matt.woodrow) → review+
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
Assignee: nobody → jmuizelaar
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
Target Milestone: --- → mozilla12
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 15

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

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

Updated

6 years ago
Depends on: 719417
Blocks: 726764
Depends on: 729145
Depends on: 729850
(Assignee)

Updated

5 years ago
No longer depends on: 719417
Depends on: 752341
Blocks: 839383

Updated

4 years ago
Depends on: 898267
You need to log in before you can comment on or make changes to this bug.