1268502 - Use a CGContext around a Skia Canvas to draw subpixel AA text on popup menus

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Mason Chang [Inactive] [:mchang]]

To enable subpixel AA text on pop up menus and context menus on OS X, wrap a skia canvas with a CGContext and ask CG to draw the text. Then we get subpixel AA text and can still delete DrawTargetCG.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: subpixel.patch

Comment 2

[Mason Chang [Inactive] [:mchang]]

Couple of things. Since some of this is shared across CG/Skia, I moved some functions related to the subpixel AA fixups from DrawTargetCG to CGPopupHelper. In Skia, if we detect a font smoothing background color, we use CG. We create a CG Context, setup the transforms, visit the clips provided by Skia and apply them to the CG Context, then mostly do the same thing as DrawTargetCG::FillGlyphs in Skia.

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8746552 [details] [diff] [review]
subpixel.patch

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

I don't think LockBits/ReleaseBits is supposed to be used in a way where you keep the bits locked while you execute other drawing commands.
I think you need to Lock/Release for every text drawing call, and then you can compare the new bits pointer to the one you used for your CG context and create a new CG context if it's a different pointer.
You also need to clear the CGContext in PushLayer / PopLayer.

::: gfx/2d/CGPopupDrawer.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-

Please call this file CGTextDrawing.h

::: gfx/2d/DrawTargetSkia.cpp
@@ +134,5 @@
>  
>  DrawTargetSkia::DrawTargetSkia()
>    : mSnapshot(nullptr)
> +#ifdef MOZ_WIDGET_COCOA
> +  , mCG(nullptr)

Please also initialize mCanvasData.

@@ +711,5 @@
> +
> +static CGContextRef
> +CreateCGContext(DrawTargetSkia* aDT,
> +             CGColorSpaceRef aColorSpace,
> +             uint8_t* aData,

Did you want this to be a uint8_t*&? I'd prefer it to be a uint8_t** though.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: subpixel.patch

Updated with feedback from comment 4.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8747791 [details] [diff] [review]
subpixel.patch

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

Getting closer.

::: gfx/2d/CGTextDrawing.h
@@ +86,5 @@
> +  }
> +
> +  uint8_t* bitmapData = (uint8_t*)CGBitmapContextGetData(aContext);
> +  int w = CGBitmapContextGetWidth(aContext);
> +  int h = CGBitmapContextGetHeight(aContext);

Please call these endX and endY.

@@ +92,5 @@
> +  int startY = 0;
> +  int startX = 0;
> +
> +  if (!CGRectIsNull(aBox)) {
> +    startX = aBox.origin.x;

What if aBox extends beyond the edges of our buffer in any direction? I think you need to intersect aBox with our bounds.

I think you should do that using CGRectIntersection, and use CGRectInfinite as the "no bounds" value for aBox. Then you can use CGRectIntersection and compute startX/startY/endX/endY from the intersected rect, both unconditionally.

::: gfx/2d/DrawTargetSkia.cpp
@@ +686,5 @@
> +      } // end switch
> +    } // end while
> +
> +    RefPtr<Path> path = pathBuilder->Finish();
> +    PathCG *cgPath = static_cast<PathCG*>(path.get());

PathCG* cgPath

@@ +690,5 @@
> +    PathCG *cgPath = static_cast<PathCG*>(path.get());
> +    CGContextBeginPath(mCG);
> +
> +    // Weirdly, CoreGraphics clips empty paths as all shown
> +    // but emtpy rects as all clipped.  We detect this situation and

emtpy -> empty

@@ +693,5 @@
> +    // Weirdly, CoreGraphics clips empty paths as all shown
> +    // but emtpy rects as all clipped.  We detect this situation and
> +    // workaround it appropriately
> +    if (CGPathIsEmpty(cgPath->GetPath())) {
> +      // XXX: should we return here?

Yes, and we should do this before calling CGContextBeginPath.

@@ +723,5 @@
> +    NS_WARNING("Could not lock skia bits to wrap CG around");
> +    return nullptr;
> +  }
> +
> +  if ((aSurfaceData == mCanvasData) && mCG && (mCGSize == size)) {

Do you also need to compare against aOptions.mAlpha?

@@ +772,5 @@
> +  CGContextTranslateCTM(aCGContext, 0, aDT->GetSize().height);
> +  CGContextScaleCTM(aCGContext, 1, -1);
> +
> +  // Want to apply clips BEFORE the transform since the transform
> +  // will apply to the clips we apply.

This comment also needs to mention that the clips that CGClipApply applies are in device space, so it would be a mistake to transform those clips.

@@ +778,5 @@
> +  aCanvas->replayClips(&clipApply);
> +
> +  CGContextConcatCTM(aCGContext, GfxMatrixToCGAffineTransform(aDT->GetTransform()));
> +  // Flip again so we draw text in the right spot.
> +  CGContextScaleCTM(aCGContext, 1, -1);

Let's do this flip where we actually draw the text, and not here.

@@ +789,5 @@
> + * CG to draw the text. Even then, we have to use a private API,
> + * CGContextSetFontSmoothingBackgroundColor, which sets the expected
> + * background color the text will draw onto so that CG can render the text
> + * properly. After that, we have to go back and fixup the pixels
> + * such that their alpha values are correct.

Please rewrite this comment to explicitly list the two scenarios that we need it for: text in DrawTargets that are not opaque, and text over vibrant backgrounds. And mention that the CGContextSetFontSmoothingBackgroundColor API is only used for the vibrancy case.

@@ +824,5 @@
> +
> +  // This code can execute millions of times in short periods, so we want to
> +  // avoid heap allocation whenever possible. So we use an inline vector
> +  // capacity of 64 elements, which is enough to typically avoid heap
> +  // allocation in ~99% of cases.

This comment is unnecessary here, I think, because we don't draw millions of lines of text in popups. I think you should just remove it.

@@ +859,5 @@
> +  extents = CGRectMake(rect.x, rect.y, rect.width, rect.height);
> +
> +  EnsureValidPremultipliedData(mCG, extents);
> +
> +  ReleaseBits(mCanvasData);

I don't like how the contract for SetupCGContext is "the caller must call ReleaseBits". Can you rename SetupCGContext to BorrowCGContext and add a ReturnCGContext function (which calls ReleaseBits), and only touch mCG and mCanvasData inside those functions? DrawTargetSkia::FillGlyphsWithCG can have a local CGContextRef cg; variable that it gets from SetupCGContext and passes to ReturnCGContext.

@@ +897,5 @@
>    MarkChanged();
>  
> +#ifdef MOZ_WIDGET_COCOA
> +  if (HasFontSmoothingBackgroundColor(aRenderingOptions)) {
> +    if(FillGlyphsWithCG(aFont, aBuffer, aPattern, aOptions, aRenderingOptions)) {

missing space after "if"

@@ +1519,5 @@
>  
>    SetPermitSubpixelAA(aOpaque);
> +
> +#ifdef MMOZ_WIDGET_COCOA
> +  CGContextSaveGState(mCG);

No, I think you need to CGContextRelease(mCG); mCG = nullptr;, in both cases.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: subpixel.patch

Updated with feedback from comment 6.

> @@ +693,5 @@
> > +    // Weirdly, CoreGraphics clips empty paths as all shown
> > +    // but emtpy rects as all clipped.  We detect this situation and
> > +    // workaround it appropriately
> > +    if (CGPathIsEmpty(cgPath->GetPath())) {
> > +      // XXX: should we return here?
> 
> Yes, and we should do this before calling CGContextBeginPath.

Also fixed in DrawTargetCG.

 
> @@ +723,5 @@
> > +    NS_WARNING("Could not lock skia bits to wrap CG around");
> > +    return nullptr;
> > +  }
> > +
> > +  if ((aSurfaceData == mCanvasData) && mCG && (mCGSize == size)) {
> 
> Do you also need to compare against aOptions.mAlpha?

Fixed to always set the alpha.

> @@ +778,5 @@
> > +  aCanvas->replayClips(&clipApply);
> > +
> > +  CGContextConcatCTM(aCGContext, GfxMatrixToCGAffineTransform(aDT->GetTransform()));
> > +  // Flip again so we draw text in the right spot.
> > +  CGContextScaleCTM(aCGContext, 1, -1);
> 
> Let's do this flip where we actually draw the text, and not here.

I really don't like doing that. It was really confusing in DrawTargetCCC that we had the random flip so I really like that it's in one area now. Can we keep this?

Comment 8

[Markus Stange [:mstange]]

(In reply to Mason Chang [:mchang] from comment #7)
> > @@ +778,5 @@
> > > +  aCanvas->replayClips(&clipApply);
> > > +
> > > +  CGContextConcatCTM(aCGContext, GfxMatrixToCGAffineTransform(aDT->GetTransform()));
> > > +  // Flip again so we draw text in the right spot.
> > > +  CGContextScaleCTM(aCGContext, 1, -1);
> > 
> > Let's do this flip where we actually draw the text, and not here.
> 
> I really don't like doing that. It was really confusing in DrawTargetCCC
> that we had the random flip so I really like that it's in one area now. Can
> we keep this?

There are three reasons I requested this:
 (1) If we want to use SetupCGContext for widget drawing, we don't need this flip. The flip is only for the purpose of drawing text right-side up.
 (2) Having just a scale(1, -1) on the context is not really a transform anybody expects: You either expect the origin at the top left and the y axis pointing down, or you expect the origin at the bottom left with the y axis pointing up. The flip here doesn't move the origin, it only flips the axis, so this "special" transform should be limited to the code that requires it.
 (3) There are two flips here that need to cancel each other out: The flip transform on the CGContext and the "-aBuffer.mGlyphs[i].mPosition.y" flip on the glyph positions. These two flips should be close together in the code so that the reader can see that they match up.

I see now that addressing my comment didn't really get us closer with respect to (3). Ideally we'd be able to factor out a function DrawCGText() that both does the flip on the context and the flip on the positions, and draws to the context. But I see how such a function would be hard to use from DrawTargetCG::FillGlyphs, because DrawTargetCG::FillGlyphs has so many different ways it draws text.
Do you have any ideas for how we could address this?

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: subpixel.patch

Extracted out the flips and inversions for glyph positions into 1 place with a wall of text.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8749337 [details] [diff] [review]
subpixel.patch

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

Almost there now.

::: gfx/2d/DrawTargetCG.cpp
@@ +1504,4 @@
>      return;
>    }
>  
> +  for (unsigned int i = 0; i < aBuffer.mNumGlyphs; i++) {

Why are you moving this loop above the CGContextScaleCTM?

@@ +1939,2 @@
>      CGContextClipToRect(mCg, CGRectZero);
> +    return;

You need to move CGContextBeginPath below this return.

::: gfx/2d/DrawTargetSkia.cpp
@@ +712,5 @@
> +  CGContextRef mCG;
> +};
> +
> +void
> +DrawTargetSkia::ReturnCGContext(CGContextRef aCGContext)

Let's swap the order here, so that we have BorrowCGContext first and then ReturnCGContext.

@@ +847,5 @@
> +  // to transform these clips.
> +  CGClipApply clipApply(aCGContext);
> +  aCanvas->replayClips(&clipApply);
> +
> +  CGContextConcatCTM(aCGContext, GfxMatrixToCGAffineTransform(aDT->GetTransform()));

Sorry for making you move around this stuff again, but I'd really prefer the stuff up to (and including) this line to be in a separate function that's always called by BorrowCGContext before it returns the context.
The transforms and clips up to here are what users of the context expect - clipped properly and with the origin in the top left, and the DT matrix applied. This is the state that for example native widget drawing would want to get the context in.

The stuff that comes after this line is just for text drawing. It can stay in this function.

Comment 11

[Mason Chang [Inactive] [:mchang]]

Attachment: subpixel.patch

Trying again, using the BorrowedCGContext interface was pretty ugly within Skia.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8751837 [details] [diff] [review]
subpixel.patch

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +718,5 @@
> + * while every other DrawTarget assumes the top left is the origin.
> + * This means we have to transform the CGContext to have rects
> + * actually be applied in top left fashion. We do this by:
> + *
> + * 1) Translating the context up by the size of the canvas

s/size/height/

@@ +719,5 @@
> + * This means we have to transform the CGContext to have rects
> + * actually be applied in top left fashion. We do this by:
> + *
> + * 1) Translating the context up by the size of the canvas
> + * 2) Flipping the contex by the Y axis so it's upside down.

s/contex/context/

@@ +721,5 @@
> + *
> + * 1) Translating the context up by the size of the canvas
> + * 2) Flipping the contex by the Y axis so it's upside down.
> + *
> + * These two transforms put the bitmap in the correct order.

I don't know what this means. The bits don't change.

@@ +725,5 @@
> + * These two transforms put the bitmap in the correct order.
> + * Transforms are better understood thinking about them from right to left order (mathematically).
> + *
> + * Consider a point we want to draw at (0, 10) in normal cartesian planes with
> + * a box of (100, 100). in CG terms, this would be at (0, 10).

s/box/canvas size/

@@ +736,5 @@
> + * of (0, 10). The first flip of the Y axis puts the point now at (0, -10);
> + * Next, we translate the context up by the size of the canvas (Positive Y values go up in CG
> + * coordinates but down in our draw target coordinates). Since our canvas size is (100, 100),
> + * the resulting coordinate becomes (0, 90), which is what we expect from our DrawTarget code.
> + * These two transforms put the CG context equal to what every other DrawTarget expects.

Well, this section is quite basic, but if you think it helps readers then leave it in.

@@ +761,5 @@
> + * 2) The letter P upside down (b) at (0, 20) due to the second flip
> + * 3) The letter P right side up at (0, -20) due to the first flip
> + * 4) The letter P right side up at (0, 80) due to the translation
> + *
> + * tl;dr - CGRects assume origin is bottom left, DrawTarget rects assume top left. Transforms.

"top left. Transforms." ?

@@ +794,5 @@
> +              const GlyphBuffer& aBuffer,
> +              Vector<CGGlyph,32>& aGlyphs,
> +              Vector<CGPoint,32>& aPositions)
> +{
> +  // Flip again so we draw text in the right spot. Transform (3) from the top

"in the right spot" is wrong here. You want "right-side up".

@@ +807,5 @@
> +  for (unsigned int i = 0; i < aBuffer.mNumGlyphs; i++) {
> +    aGlyphs[i] = aBuffer.mGlyphs[i].mIndex;
> +
> +    // Negative Y axis since glyph positions assume top left is at (0, 0)
> +    // whereas CG is bottom left.

Replace this with // Flip the y coordinates so that text ends up in the right spot after the (3) flip.

Comment 13

[Mason Chang [Inactive] [:mchang]]

Try looks good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c1489886e6a

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cafbc58f64d

Comment 15

[Lee Salzman [:lsalzman]]

(In reply to Pulsebot from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9cafbc58f64d

In DrawTargetSkia::DrawTargetSkia(), you always initialize mColorSpace, but in DrawTargetSkia::~DrawTargetSkia(), you only conditionally release it if mCG is not null.

This means mColorSpace is going to leak, as mCG can be null when mColorSpace is not.

It would be best if mColorSpace is initialized to null in DrawTargetSkia's constructor, and only lazily created if mColorSpace is still null when mCG is finally created. This way, we only pay the cost of constructing mColorSpace if we actually need to use it, whereas currently we always pay the construction cost regardless, plus the leak.

Then in DrawTargetSkia's destructor, the CGColorSpaceRelease call should not be conditional on mCG at all.

Comment 16

[Mason Chang [Inactive] [:mchang]]

Attachment: Only alloc a color space if needed

Comment 17

[Markus Stange [:mstange]]

(In reply to Lee Salzman [:lsalzman] from comment #15)
> This means mColorSpace is going to leak, as mCG can be null when mColorSpace
> is not.

Good catch, sorry for not noticing that.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9cafbc58f64d

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 8751837 [details] [diff] [review]
subpixel.patch

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +923,5 @@
> +
> +  Vector<CGGlyph,32> glyphs;
> +  Vector<CGPoint,32> positions;
> +  if (!SetupCGGlyphs(cgContext, aBuffer, glyphs, positions)) {
> +    return false;

You need to call ReturnCGContext here. Sorry for missing that too.

Comment 20

[Mason Chang [Inactive] [:mchang]]

Attachment: Only alloc a color space if needed

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49ebea2ca37a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49ebea2ca37a