Bug 204671 – [GTK] Turn off antialiasing when rendering with Ahem

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Miguel Gomez]

r252701 disabled antialiasing for the Ahem font (which is just rectangles for testing) for apple, and marked as passing all the failing tests, but these are not passing for gtk yet. Maybe we should follow the same approach and disable antialiasing.

  imported/w3c/web-platform-tests/css/css-multicol/multicol-rule-fraction-003.xht [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-multicol/multicol-rule-shorthand-2.xht [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-001.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-002.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-003.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-004.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-005.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-006.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-007.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-008.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-009.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-010.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-014.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-015.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-016.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-end-017.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-justify-001.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-justify-002.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-justify-003.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-justify-004.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-justify-005.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-justify-006.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-001.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-002.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-003.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-004.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-005.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-006.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-007.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-008.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-009.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-010.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-014.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-015.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-016.html [ ImageOnlyFailure ]
  imported/w3c/web-platform-tests/css/css-text/text-align/text-align-start-017.html [ ImageOnlyFailure ]

[Carlos Alberto Lopez Perez]

Created attachment 387726 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 387726 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        the glyps causes small pixel differences with the reference test.

glyps -> glyphs

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:139
> +    FcChar8* fontConfigFamilyName;
> +    FcPatternGetString(m_platformData.fcPattern(), FC_FAMILY, 0, &fontConfigFamilyName);
> +    String familyName = String::fromUTF8(reinterpret_cast<char*>(fontConfigFamilyName));
> +    // Disable antialiasing for the Ahem font because many tests require this.
> +    if (equalIgnoringASCIICase(familyName, "Ahem"))
> +        m_allowsAntialiasing = false;

This doesn't work for web fonts, because FC_FAMILY is not set. You need to set the family name in FontCustomPlatformData::fontPlatformData(). Since we are always going to modify the pattern now, we can modify defaultFontconfigOptions() to return a RefPtr with the static pattern duplicated. Then you can get the family name from the FC face directly (freeTypeFace->family_name) and set it in the pattern with FcPatternAddString

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #2)
> This doesn't work for web fonts, because FC_FAMILY is not set. You need to
> set the family name in FontCustomPlatformData::fontPlatformData(). Since we
> are always going to modify the pattern now, we can modify
> defaultFontconfigOptions() to return a RefPtr with the static pattern
> duplicated. Then you can get the family name from the FC face directly
> (freeTypeFace->family_name) and set it in the pattern with FcPatternAddString

You are totally right! And thanks for the pointers! very useful :)

After testing this I can see how now it works across a bigger range of tests, including fixing the ones from bug 206265 and bug 152821.

[Carlos Alberto Lopez Perez]

*** Bug 152821 has been marked as a duplicate of this bug. ***

[Carlos Alberto Lopez Perez]

Committed r254567: <https://trac.webkit.org/changeset/254567>

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #5)
> Committed r254567: <https://trac.webkit.org/changeset/254567>

It seems I caused a leak here because I didn't call adoptRef() when creating the RefPtr from FcPattern.

Re-opening to fix

[Carlos Alberto Lopez Perez]

Created attachment 387956 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 387956 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:75
> +    return RefPtr<FcPattern>(adoptRef(FcPatternDuplicate(pattern)));

Isn't return adoptRef(FcPatternDuplicate(pattern)) enough?

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #8)
> Comment on attachment 387956 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387956&action=review
> 
> > Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:75
> > +    return RefPtr<FcPattern>(adoptRef(FcPatternDuplicate(pattern)));
> 
> Isn't return adoptRef(FcPatternDuplicate(pattern)) enough?

yes, it is.. will change it before landing

[Carlos Alberto Lopez Perez]

Committed r254741: <https://trac.webkit.org/changeset/254741>