Closed Bug 1055622 Opened 6 years ago Closed 6 years ago

Add support for specifying the font smoothing background color to Moz2D

Categories

(Core :: Graphics, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(2 files, 3 obsolete files)

CoreGraphics has a private API since 10.9 called CGContextSetFontSmoothingBackgroundColor which lets you have subpixel anti-aliased fonts on transparent backgrounds if you roughly know what the final background color is going to be.

We'll need to call this if we want to have nice text rendering on top of "vibrant" widgets on 10.10, like the browser sidebar and context menus. And the cleanest way to support this is probably to start by adding support for it to Moz2D.

Proposed API:

class DrawTarget {
...
  bool SupportsFontSmoothingBackgroundColor() const;
  void SetFontSmoothingBackgroundColor(const Color& aColor);
...
}
Blocks: 1055634
Attached patch wip (obsolete) — Splinter Review
Attached patch v1 (obsolete) — Splinter Review
r?Bas on the 2d.h changes

I'm not really thrilled to add another bit of state to DrawTarget, but putting the color into GlyphRenderingOptions looks quite tricky to me, especially from the Gecko side.
Assignee: nobody → mstange
Attachment #8499989 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8503136 - Flags: review?(bas)
Comment on attachment 8503136 [details] [diff] [review]
v1

r?jrmuizel on the DrawTargetCG changes, especially on the EnsureValidPremultipliedData ugliness
Attachment #8503136 - Flags: review?(jmuizelaar)
This sheds some light on what CG is doing.
Comment on attachment 8503136 [details] [diff] [review]
v1

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

This should probably be added to the recording backends as well.

::: gfx/2d/DrawTargetCG.cpp
@@ +1505,5 @@
> +    // context into a context that has a different color space throws up on
> +    // invalid premultiplied data and creates completely wrong colors.
> +    // Sanitizing the data means that we lose some of the fake component alpha
> +    // behavior that font rendering tries to give us, but the result still
> +    // looks good enough to prefer it over grayscale font anti-aliasing.

When do we draw into a context with a different color space? We should probably not ever be doing that.

@@ +1698,5 @@
> +  if (!lookedUpFunc) {
> +    void* coreGraphicsFramework = dlopen(COREGRAPHICS_FRAMEWORK_PATH, RTLD_LAZY | RTLD_LOCAL);
> +    if (coreGraphicsFramework) {
> +      func = (CGContextSetFontSmoothingBackgroundColorFunc)dlsym(
> +        coreGraphicsFramework, "CGContextSetFontSmoothingBackgroundColor");

I think you can just use RTLD_DEFAULT instead of coreGraphicsFramework here. That's what ScaledFontMac::ScaledFontMac does.

@@ +1712,5 @@
> +{
> +  DrawTarget::SetFontSmoothingBackgroundColor(aColor);
> +
> +  CGContextSetFontSmoothingBackgroundColorFunc func =
> +    GetCGContextSetFontSmoothingBackgroundColorFunc();

Use a better name than func.

Do you have some idea what this function is actually doing? If you do a description in a comment someplace would be great.
Attachment #8503136 - Flags: review?(jmuizelaar) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> Comment on attachment 8503136 [details] [diff] [review]
> v1
> 
> Review of attachment 8503136 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should probably be added to the recording backends as well.

right, thanks

> ::: gfx/2d/DrawTargetCG.cpp
> @@ +1505,5 @@
> > +    // context into a context that has a different color space throws up on
> > +    // invalid premultiplied data and creates completely wrong colors.
> > +    // Sanitizing the data means that we lose some of the fake component alpha
> > +    // behavior that font rendering tries to give us, but the result still
> > +    // looks good enough to prefer it over grayscale font anti-aliasing.
> 
> When do we draw into a context with a different color space? We should
> probably not ever be doing that.

When drawing into the context we get from drawRect, at least. I saw the bad colors in context menus, which don't use hardware acceleration.
You can also see it happening in the test app I attached.

> @@ +1698,5 @@
> > +  if (!lookedUpFunc) {
> > +    void* coreGraphicsFramework = dlopen(COREGRAPHICS_FRAMEWORK_PATH, RTLD_LAZY | RTLD_LOCAL);
> > +    if (coreGraphicsFramework) {
> > +      func = (CGContextSetFontSmoothingBackgroundColorFunc)dlsym(
> > +        coreGraphicsFramework, "CGContextSetFontSmoothingBackgroundColor");
> 
> I think you can just use RTLD_DEFAULT instead of coreGraphicsFramework here.
> That's what ScaledFontMac::ScaledFontMac does.

Oh good. "man dlsym" says about RTLD_DEFAULT "This can be a costly search and should be avoided.", but if we're already doing it then I'll just ignore that warning.

> 
> @@ +1712,5 @@
> > +{
> > +  DrawTarget::SetFontSmoothingBackgroundColor(aColor);
> > +
> > +  CGContextSetFontSmoothingBackgroundColorFunc func =
> > +    GetCGContextSetFontSmoothingBackgroundColorFunc();
> 
> Use a better name than func.

ok

> Do you have some idea what this function is actually doing?

As in, do I know the actual calculation? No, I don't know that. I only know what I wrote in the first paragraph of comment 0.

> If you do a
> description in a comment someplace would be great.

ok
Attachment #8503244 - Flags: review?(bas)
Comment on attachment 8503136 [details] [diff] [review]
v1

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

This is so very specific to a certain backend, I feel it should go into a GlyphRenderingOptions object rather than through its own API. These sort of things will really clutter the API.
Attachment #8503136 - Flags: review?(bas) → review-
Attachment #8503244 - Flags: review?(bas)
Attached patch v2Splinter Review
Attachment #8503136 - Attachment is obsolete: true
Attachment #8503244 - Attachment is obsolete: true
Attachment #8505725 - Flags: review?(bas)
Attachment #8505725 - Flags: review?(jmuizelaar)
Attachment #8505725 - Flags: review?(bas)
Attachment #8505725 - Flags: review+
Attachment #8505725 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/1d593636d86d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
See Also: → 1258752
You need to log in before you can comment on or make changes to this bug.