Bug 68734 – Enable LCD text in Skia on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Cary Clark]

Enable LCD text in Skia on Mac

[Cary Clark]

Created attachment 108544 [details]
Patch

[James Robinson]

Comment on attachment 108544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108544&action=review

> Source/WebCore/ChangeLog:11
> +        Duplicate the logic in FontMac.mm to pass settings

That word makes me sad.  Is there any way to share the logic instead of duplicating?

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:109
> +    case Antialiased: {
> +        shouldSmoothFonts = false;
> +        break;
> +    }

{ }s in a case statement is very strange - I think this would be easier to read if they were omitted

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:123
> +    default: 
> +        ASSERT_NOT_REACHED();

just leave out the 'default' case and the compiler will tell us if all cases are listed or not

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:126
> +    if (!shouldUseSmoothing() || PlatformSupport::layoutTestMode())

do you really need to check layoutTestMode() here? I thought we twiddled the shouldSmoothFonts somewhere

[Cary Clark]

Created attachment 108653 [details]
Patch

[Cary Clark]

Comment on attachment 108544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108544&action=review

>> Source/WebCore/ChangeLog:11
>> +        Duplicate the logic in FontMac.mm to pass settings
> 
> That word makes me sad.  Is there any way to share the logic instead of duplicating?

No. The logic is duplicated, but the code is not. The FontMac.mm code performs additional work that does not make sense to duplicate. Eventually, the Linux and Windows Chromium ports will share this logic -- all Skia ports will use FontSkia for common code.

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:109
>> +    }
> 
> { }s in a case statement is very strange - I think this would be easier to read if they were omitted

Fixed.

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:123
>> +        ASSERT_NOT_REACHED();
> 
> just leave out the 'default' case and the compiler will tell us if all cases are listed or not

Fixed.

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:126
>> +    if (!shouldUseSmoothing() || PlatformSupport::layoutTestMode())
> 
> do you really need to check layoutTestMode() here? I thought we twiddled the shouldSmoothFonts somewhere

Yes, it's required. There is an equivalent check in FontCacheChromiumWin.cpp.

[Cary Clark]

Created attachment 108654 [details]
Patch

[James Robinson]

(In reply to comment #4)
> >> Source/WebCore/platform/graphics/skia/FontSkia.cpp:126
> >> +    if (!shouldUseSmoothing() || PlatformSupport::layoutTestMode())
> > 
> > do you really need to check layoutTestMode() here? I thought we twiddled the shouldSmoothFonts somewhere
> 
> Yes, it's required. There is an equivalent check in FontCacheChromiumWin.cpp.

But there aren't any such checks in the old mac codepath, and it seems to work fine.  Why would the skia font code be the only rendering system that needs to check this bit explicitly?

[Cary Clark]

Skia sets the font smoothing policy differently from CoreText. This may explain the difference. I have a separate change in for Skia to address this. In the meantime, would you consider approving this as is?

[James Robinson]

(In reply to comment #7)
> Skia sets the font smoothing policy differently from CoreText. This may explain the difference. I have a separate change in for Skia to address this. In the meantime, would you consider approving this as is?

I'm confused about how this addresses my question.  Do we actually know where the difference is?

[Cary Clark]

The Skia change is: http://codereview.appspot.com/5131046/ -- but I'd prefer not to make this change dependent on that one.

[James Robinson]

I can't tell how that skia patch relates to this.  At a quick glance I'm not sure how the chromium mac DRT port enables grayscale text and my worry is that if we're missing that configuration path, we might be missing other important text configuration paths that exist.  I'd feel a lot more comfortable with this change if I knew how the old path worked.  Just saying "it is different" doesn't help me understand how this is the right way to address this.

[Stephen White]

(In reply to comment #10)
> I can't tell how that skia patch relates to this.  At a quick glance I'm not sure how the chromium mac DRT port enables grayscale text and my worry is that if we're missing that configuration path, we might be missing other important text configuration paths that exist.  I'd feel a lot more comfortable with this change if I knew how the old path worked.  Just saying "it is different" doesn't help me understand how this is the right way to address this.

I share James's concern here.  There's no call to layoutTestMode() in platform/graphics/mac or /cg, so I'm confused as to where it was being done in the old Chrome/Mac.  The Safari layout test results look like they don't have LCD AA (although that could be blamed on presbyopia...).

[Stephen White]

Comment on attachment 108654 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108654&action=review

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:116
> +        // For the AutoSmooth case, don't do anything! Keep the default settings.

Although the comment is the same as FontMac.mm, it looks like the behaviour is slightly different here:  it will set antialiasing and smoothFonts to true in the SkPaint, whereas the FontMac code avoids making a font smoothing call at all (via the changeFontSmoothing bool).  Are we diverging from the old Chrome-Mac behaviour here?

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:121
> +    if (!shouldUseSmoothing() || PlatformSupport::layoutTestMode())
> +        shouldSmoothFonts = false;

It looks like the layoutTestMode() check is already being done on the Windows side in FontCustomPlatformData::fontPlatformData().  Could we now remove it from there, since it's centralized here?

Also, how was it that Chrome-Linux was doing non-LCD text for layout tests before?  (just want to make sure we're not changing that either)

[Cary Clark]

Comment on attachment 108654 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108654&action=review

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:116
>> +        // For the AutoSmooth case, don't do anything! Keep the default settings.
> 
> Although the comment is the same as FontMac.mm, it looks like the behaviour is slightly different here:  it will set antialiasing and smoothFonts to true in the SkPaint, whereas the FontMac code avoids making a font smoothing call at all (via the changeFontSmoothing bool).  Are we diverging from the old Chrome-Mac behaviour here?

This preserves old Chrome-Mac behavior. CoreGraphics, by default, enables antialiasing, and smoothFonts. Skia, by default, has both disabled in a newly-created SkPaint object. I interpreted 'AutoSmoothing' to mean 'default CoreGraphics behavior' -- I'll replace the comment with this assumption. I've tested this on Apple and non-Apple monitors to make sure we match when smoothing is turned on and when it is not.

>> Source/WebCore/platform/graphics/skia/FontSkia.cpp:121
>> +        shouldSmoothFonts = false;
> 
> It looks like the layoutTestMode() check is already being done on the Windows side in FontCustomPlatformData::fontPlatformData().  Could we now remove it from there, since it's centralized here?
> 
> Also, how was it that Chrome-Linux was doing non-LCD text for layout tests before?  (just want to make sure we're not changing that either)

Today, FontSkia is only used by the Mac port. I'll add a FIXME here that notes that this should be shared, and create a bug to track that.

I don't know how linux works, but this change doesn't affect it.

[Cary Clark]

(In reply to comment #11)
> (In reply to comment #10)
> > I can't tell how that skia patch relates to this.  At a quick glance I'm not sure how the chromium mac DRT port enables grayscale text and my worry is that if we're missing that configuration path, we might be missing other important text configuration paths that exist.  I'd feel a lot more comfortable with this change if I knew how the old path worked.  Just saying "it is different" doesn't help me understand how this is the right way to address this.
> 
> I share James's concern here.  There's no call to layoutTestMode() in platform/graphics/mac or /cg, so I'm confused as to where it was being done in the old Chrome/Mac.  The Safari layout test results look like they don't have LCD AA (although that could be blamed on presbyopia...).

CoreGraphics assumes that offscreen drawing cannot know the characteristics of the onscreen device, and always draws with font smoothing disabled. Skia assumes that the code author knows what they are doing, and allows font smoothing to be enabled for all devices, both onscreen and offscreen.

To match Apple's behavior, this change disables font smoothing when creating layout tests. I have a separate change to Skia to disable font smoothing when Apple has chosen not to allow it on a class of monitor (typically non-Apple branded ones).

[Stephen White]

Comment on attachment 108654 [details]
Patch

Thanks for the explanations.  It's all clear now.  :)  r=me

[WebKit Review Bot]

Comment on attachment 108654 [details]
Patch

Clearing flags on attachment: 108654

Committed r96366: <http://trac.webkit.org/changeset/96366>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.