Bug 190255 – [GTK][WPE] VDMX vertical font metrics support

• Component: WebKitGTK
• Status: NEW

[Olivier Blin]

Some fonts specify their ascent and descent values for a given size in a table called VDMX.
The specification is available here: https://docs.microsoft.com/en-us/typography/opentype/spec/vdmx

This used to be supported by the Chromium port: https://bugs.webkit.org/show_bug.cgi?id=25116

[Olivier Blin]

Created attachment 351536 [details]
Patch

[EWS Watchlist]

Attachment 351536 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/freetype/VDMXParser.h:31:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WebCore/platform/graphics/freetype/VDMXParser.h:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/freetype/VDMXParser.h:36:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/freetype/VDMXParser.h:37:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/freetype/VDMXParser.h:38:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:31:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:93:  Declaration has space between type name and * in const uint8_t *const  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:120:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:121:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:153:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 12 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Comment on attachment 351536 [details]
Patch

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

Please try to make a test font so you can make a test.

> Source/WebCore/ChangeLog:10
> +        No new tests (OOPS!).

👎

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:142
> +                    ascent = vdmxAscent;
> +                    descent = -vdmxDescent;

Presumably these values should only be used in vertical text?

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:2
> + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.

?

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:36
> +// For htons/ntohs
> +#include <arpa/inet.h>

Surely there's a better way to flip endianness rather than including a header for network protocols

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:43
> +class Buffer {

It's kind of unfortunate to introduce a new class for this. We have like a hundred different Buffer types already. Are you sure we can't reuse one of those? Or maybe this should be collapsed with the later function to create a "VDMXParsingContext" class

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:75
> +    bool skip(size_t numBytes)
> +    {
> +        if (m_offset + numBytes > m_length)
> +            return false;
> +        m_offset += numBytes;
> +        return true;
> +    }
> +
> +    bool readU8(uint8_t* value)
> +    {
> +        if (m_offset + sizeof(uint8_t) > m_length)
> +            return false;
> +        *value = m_buffer[m_offset];
> +        m_offset += sizeof(uint8_t);
> +        return true;
> +    }
> +
> +    bool readU16(uint16_t* value)
> +    {
> +        if (m_offset + sizeof(uint16_t) > m_length)
> +            return false;
> +        memcpy(value, m_buffer + m_offset, sizeof(uint16_t));
> +        *value = ntohs(*value);
> +        m_offset += sizeof(uint16_t);
> +        return true;
> +    }

Returning optional<>s would be more elegant.

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:106
> +namespace WebCore {

Why is Buffer in the global namespace? That is almost surely wrong.

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:121
> +bool parseVDMX(int* yMax, int* yMin,
> +               const uint8_t* vdmx, size_t vdmxLength,
> +               unsigned targetPixelSize)

Instead of out params, I prefer returning structs. That way you can return an optional of the struct.

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:131
> +    // Now we have two tables. Firstly we have @numRatios Ratio records, then a

stray @

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:132
> +    // matching array of @numRatios offsets. We save the offset of the beginning

stray @

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:139
> +    unsigned desiredRatio = 0xffffffff;

Optional<unsigned>

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:147
> +            || !buf.readU8(&yRatio1)

The spec names this yStartRatio, I suggest matching it.

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:148
> +            || !buf.readU8(&yRatio2))

The spec names this yEndRatio, I suggest matching it.

> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:186
> +        if (pixelSize == targetPixelSize) {

Font sizes should be floats (the fact that they aren't right now is a bug). Seems unfortunate to match based on ==

[Olivier Blin]

Thanks Myles for your review.

As mentioned in the ChangeLog, the VDMXParser.cpp file is a plain import of the Chromium code which was in WebKit back in 2012, hence the Google copyright and coding style mismatches.
This file hasn't evolved much in Blink, besides the endianness check.
I will update the file according to your review.

Regarding the test, I purposely kept the OOPS line, and did not add one yet because the only test fonts I have with VDMX tables are proprietary.
Frederic suggested to create a web font for this.
Is there any font authoring tool that is commonly used for tests?

[Myles C. Maxfield]

(In reply to Olivier Blin from comment #4)
> Thanks Myles for your review.
> 
> As mentioned in the ChangeLog, the VDMXParser.cpp file is a plain import of
> the Chromium code which was in WebKit back in 2012, hence the Google
> copyright and coding style mismatches.
> This file hasn't evolved much in Blink, besides the endianness check.
> I will update the file according to your review.
> 
> Regarding the test, I purposely kept the OOPS line, and did not add one yet
> because the only test fonts I have with VDMX tables are proprietary.
> Frederic suggested to create a web font for this.
> Is there any font authoring tool that is commonly used for tests?

I made my own tool - feel free to use it https://github.com/litherum/AhemMaker Or feel free to use a better tool if you can find one.

[Frédéric Wang (:fredw)]

(In reply to Olivier Blin from comment #4)
> Regarding the test, I purposely kept the OOPS line, and did not add one yet
> because the only test fonts I have with VDMX tables are proprietary.
> Frederic suggested to create a web font for this.
> Is there any font authoring tool that is commonly used for tests?

Fontforge or TTX/FontTools can help generating fonts using python scripts.

[Michael Catanzaro]

Comment on attachment 351536 [details]
Patch

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

> Source/cmake/OptionsGTK.cmake:87
> +WEBKIT_OPTION_DEFINE(USE_VDMX_FONT_METRICS "Whether to enable support for VDMX font metrics." PRIVATE OFF)

I don't want to add a build flag that's off by default on every port.

I don't think there should be a build flag for this. I would use an experimental features runtime flag. And if the feature is working well already, then hopefully that flag can be removed sooner rather than later.

[Olivier Blin]

Comment on attachment 351536 [details]
Patch

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

>> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:142
>> +                    descent = -vdmxDescent;
> 
> Presumably these values should only be used in vertical text?

VDMX tables are not for vertical text, but to modify the vertical alignment offsets depending on the font size.

>> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:43
>> +class Buffer {
> 
> It's kind of unfortunate to introduce a new class for this. We have like a hundred different Buffer types already. Are you sure we can't reuse one of those? Or maybe this should be collapsed with the later function to create a "VDMXParsingContext" class

Thanks for challenging this.
I will rework it using OpenTypeTypes helpers, which are exactly made for this.
If this works, then no Google copyright anymore.

>> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:121
>> +               unsigned targetPixelSize)
> 
> Instead of out params, I prefer returning structs. That way you can return an optional of the struct.

I will do this, thanks for the suggestion.

>> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:147
>> +            || !buf.readU8(&yRatio1)
> 
> The spec names this yStartRatio, I suggest matching it.

Right

>> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:186
>> +        if (pixelSize == targetPixelSize) {
> 
> Font sizes should be floats (the fact that they aren't right now is a bug). Seems unfortunate to match based on ==

They are uint16 in the VDMX records, and used to be passed as int in this "old" Google code.
What would be a better way to match font sizes?

>> Source/cmake/OptionsGTK.cmake:87
>> +WEBKIT_OPTION_DEFINE(USE_VDMX_FONT_METRICS "Whether to enable support for VDMX font metrics." PRIVATE OFF)
> 
> I don't want to add a build flag that's off by default on every port.
> 
> I don't think there should be a build flag for this. I would use an experimental features runtime flag. And if the feature is working well already, then hopefully that flag can be removed sooner rather than later.

I agree with this, and realized it too when I thought about enabling tests for VDMX tables.
Thanks

[Olivier Blin]

Created attachment 351673 [details]
Patch

[Olivier Blin]

(In reply to Olivier Blin from comment #9)
> Created attachment 351673 [details]
> Patch

This is posted for intermediate review, with the VDMX parser rewritten using OpenType::TableBase.
There are still some issues to address, and a test to write.

[EWS Watchlist]

Attachment 351673 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 351673 [details]
Patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.

[Olivier Blin]

Created attachment 443806 [details]
Patch

[Olivier Blin]

This is still applicable, I have rebased and updated the patch.
I still need to write a proper test, this requires authoring a test font.

[Carlos Alberto Lopez Perez]

(In reply to Olivier Blin from comment #13)
> Created attachment 443806 [details]
> Patch

This patch causes two new unexpected results for the layout tests on WebKitGTK (likely on WPE as well)

Regressions: Unexpected text-only failures (2)
  imported/w3c/web-platform-tests/svg/import/text-intro-06-t-manual.svg [ Failure ]
  imported/w3c/web-platform-tests/svg/import/text-intro-12-t-manual.svg [ Failure ]


Diffs:

--- a/layout-test-results/imported/w3c/web-platform-tests/svg/import/text-intro-12-t-manual-expected.txt
+++ b/layout-test-results/imported/w3c/web-platform-tests/svg/import/text-intro-12-t-manual-actual.txt
@@ -3,15 +3,15 @@
 layer at (0,0) size 800x600
   RenderSVGRoot {svg} at (0,0) size 800x600
     RenderSVGHiddenContainer {defs} at (0,0) size 0x0
-    RenderSVGContainer {g} at (16,0) size 768x509
-      RenderSVGContainer {g} at (16,0) size 768x509
+    RenderSVGContainer {g} at (16,0) size 768x510
+      RenderSVGContainer {g} at (16,0) size 768x510
         RenderSVGPath {path} at (16,199) size 768x2 [stroke={[type=SOLID] [color=#BBBBBB]}] [fill={[type=SOLID] [color=#000000]}] [data="M 10 120 L 470 120"]
         RenderSVGPath {path} at (16,399) size 768x2 [stroke={[type=SOLID] [color=#BBBBBB]}] [fill={[type=SOLID] [color=#000000]}] [data="M 10 240 L 470 240"]
-        RenderSVGText {text} at (152,-1) size 176x186 contains 1 chunk(s)
-          RenderSVGInlineText {#text} at (0,0) size 176x186
+        RenderSVGText {text} at (152,-2) size 176x188 contains 1 chunk(s)
+          RenderSVGInlineText {#text} at (0,0) size 176x187
             chunk 1 (middle anchor) text run 1 at (152.44,120.00) startOffset 0 endOffset 4 width 175.12 RTL: "\x{622}\x{644}\x{627}\x{62A}"
-        RenderSVGText {text} at (161,119) size 158x186 contains 1 chunk(s)
-          RenderSVGInlineText {#text} at (0,0) size 157x186
+        RenderSVGText {text} at (161,118) size 158x188 contains 1 chunk(s)
+          RenderSVGInlineText {#text} at (0,0) size 157x187
             chunk 1 (middle anchor) text run 1 at (161.73,240.00) startOffset 0 endOffset 5 width 156.53 RTL: "\x{62B}\x{644}\x{627}\x{62B}\x{629}"
     RenderSVGContainer {g} at (16,518) size 380x60
       RenderSVGText {text} at (10,311) size 228x36 contains 1 chunk(s)


--- a/layout-test-results/imported/w3c/web-platform-tests/svg/import/text-intro-06-t-manual-expected.txt
+++ b/layout-test-results/imported/w3c/web-platform-tests/svg/import/text-intro-06-t-manual-actual.txt
@@ -3,15 +3,15 @@
 layer at (0,0) size 800x600
   RenderSVGRoot {svg} at (0,0) size 800x600
     RenderSVGHiddenContainer {defs} at (0,0) size 0x0
-    RenderSVGContainer {g} at (16,0) size 768x509
-      RenderSVGContainer {g} at (16,0) size 768x509
+    RenderSVGContainer {g} at (16,0) size 768x510
+      RenderSVGContainer {g} at (16,0) size 768x510
         RenderSVGPath {path} at (16,199) size 768x2 [stroke={[type=SOLID] [color=#BBBBBB]}] [fill={[type=SOLID] [color=#000000]}] [data="M 10 120 L 470 120"]
         RenderSVGPath {path} at (16,399) size 768x2 [stroke={[type=SOLID] [color=#BBBBBB]}] [fill={[type=SOLID] [color=#000000]}] [data="M 10 240 L 470 240"]
-        RenderSVGText {text} at (152,-1) size 176x186 contains 1 chunk(s)
-          RenderSVGInlineText {#text} at (0,0) size 176x186
+        RenderSVGText {text} at (152,-2) size 176x188 contains 1 chunk(s)
+          RenderSVGInlineText {#text} at (0,0) size 176x187
             chunk 1 (middle anchor) text run 1 at (152.44,120.00) startOffset 0 endOffset 4 width 175.12 RTL: "\x{622}\x{644}\x{627}\x{62A}"
-        RenderSVGText {text} at (161,119) size 158x186 contains 1 chunk(s)
-          RenderSVGInlineText {#text} at (0,0) size 157x186
+        RenderSVGText {text} at (161,118) size 158x188 contains 1 chunk(s)
+          RenderSVGInlineText {#text} at (0,0) size 157x187
             chunk 1 (middle anchor) text run 1 at (161.73,240.00) startOffset 0 endOffset 5 width 156.53 RTL: "\x{62B}\x{644}\x{627}\x{62B}\x{629}"
     RenderSVGContainer {g} at (16,518) size 409x60
       RenderSVGText {text} at (10,311) size 245x36 contains 1 chunk(s)


Updating this expected results (A rebaseline) seems appropriate.