Closed Bug 692879 Opened 13 years ago Closed 12 years ago

Implement CoreGraphics Azure backend

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(4 files, 3 obsolete files)

      No description provided.
Blocks: 525650
Blocks: 379992
Attached patch First implementionation (obsolete) — Splinter Review
Attachment #575052 - Flags: review?(matt.woodrow)
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?
Attachment #575052 - Attachment is obsolete: true
Attachment #575056 - Attachment is obsolete: true
Attachment #575056 - Flags: review?(matt.woodrow)
(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.
Attached patch Done versionSplinter Review
Attachment #585964 - Attachment is obsolete: true
Attachment #587110 - Flags: review?(matt.woodrow)
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+
Attachment #588997 - Flags: review?(matt.woodrow)
Attachment #588997 - Flags: review?(matt.woodrow) → review+
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
I see various rendering breakage turning on the new Azure backend on OSX: 

    https://bugzilla.mozilla.org/show_bug.cgi?id=719417
Depends on: 719417
Depends on: 729145
Depends on: 729850
No longer depends on: 719417
Depends on: 752341
Blocks: 839383
Depends on: 898267
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: