Bug 169107 – Migrate font-stretch to use fixed-point values

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Migrate font-stretch to use fixed-point values

[Myles C. Maxfield]

Created attachment 303269 [details]
Patch

[Dean Jackson]

Comment on attachment 303269 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.h:162
> +    uint16_t m_stretch { 0 };

You don't really need the initializer since you always set the value in the constructor.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:903
> +                ASSERT_UNUSED(success, success);

Why is success there twice?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:904
> +                width = FontSelectionValue(ctWidth < 0.5 ? ctWidth * 50 + 100 : ctWidth * 150 + 50);

Why not just use (ctWidth + 1) * 75 + 50 ?

Is there some reason why the lower 75% of the [-1, 1] range takes up 50% of the output space in CSS?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1047
> +        auto threshold = std::max(targetStyle, installedFonts.capabilities.slope.maximum);

Is styleBounds equivalent to capabilities.slope?

> Source/WebCore/platform/text/TextFlags.h:404
> +class FontSelectionValue {

This class doesn't really have anything to do with fonts. Why not call it something like Float16 (in a separate .h file) and then typedef Float16 FontSelectionValue?

> Source/WebCore/platform/text/TextFlags.h:511
> +struct FontSelectionRange {

Same here. Float16Range?

> Source/WebCore/platform/text/TextFlags.h:514
> +        return minimum == maximum || minimum < maximum;

Why not use the operator<= you added above?

> Source/WebCore/platform/text/TextFlags.h:521
> +        if (!isValid())
> +            *this = other;

In what cases would this happen? Should it be a silent write?

[Dean Jackson]

FixedFloat16?

[Myles C. Maxfield]

Comment on attachment 303269 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontCache.h:162
>> +    uint16_t m_stretch { 0 };
> 
> You don't really need the initializer since you always set the value in the constructor.

Yeah, but I think it's safer. And someone told me to add static initializers to this struct some time (but I don't remember who or when).

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:903
>> +                ASSERT_UNUSED(success, success);
> 
> Why is success there twice?

ASSERT_UNUSED(), not UNUSED_PARAM().

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:904
>> +                width = FontSelectionValue(ctWidth < 0.5 ? ctWidth * 50 + 100 : ctWidth * 150 + 50);
> 
> Why not just use (ctWidth + 1) * 75 + 50 ?
> 
> Is there some reason why the lower 75% of the [-1, 1] range takes up 50% of the output space in CSS?

If you use just a regular linear transfer function, you get wrong results. Instead, something more complicated is necessary, such as this piecewise linear transfer function. Even this one isn't perfect, though - we probably need CoreText SPI to do it correctly.

The reason is just that this is roughly how Core Text's widths and CSS widths are related. (CSS's widths came from Microsoft's widths originally, so that's the origin.)

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1047
>> +        auto threshold = std::max(targetStyle, installedFonts.capabilities.slope.maximum);
> 
> Is styleBounds equivalent to capabilities.slope?

Yes.

>> Source/WebCore/platform/text/TextFlags.h:404
>> +class FontSelectionValue {
> 
> This class doesn't really have anything to do with fonts. Why not call it something like Float16 (in a separate .h file) and then typedef Float16 FontSelectionValue?

It is kind of related to fonts - the number of fractional bits, the size of the type, and the fact that it's signed are chosen with font selection in mind. I would agree with you if this type was more general purpose (like, if the number of bits was templated), but I don't want to be in the business of implementing a general-purpose fixed-point class.

>> Source/WebCore/platform/text/TextFlags.h:514
>> +        return minimum == maximum || minimum < maximum;
> 
> Why not use the operator<= you added above?

Good catch.

>> Source/WebCore/platform/text/TextFlags.h:521
>> +            *this = other;
> 
> In what cases would this happen? Should it be a silent write?

This is so that you can say something like

Range r;
r.extend(otherrange1);
r.extend(otherrange2);
r.extend(otherrange3);

At the end of this, r should encompass otherrange1, 2, and 3, but nothing else.

If you make the default constructor initialize to { 0, 0 }, then r will always include a point at 0. Instead, we need to have some state to mean "uninitialized". Extending an uninitialized range will cause it to simply become the target range.

Instead of a separate bool, a cheaper way to do this is to enforce isValid() above.

[Myles C. Maxfield]

Created attachment 303271 [details]
Patch for committing

[Myles C. Maxfield]

Committed r213341: <http://trac.webkit.org/changeset/213341>

[Dean Jackson]

Comment on attachment 303269 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:904
>>> +                width = FontSelectionValue(ctWidth < 0.5 ? ctWidth * 50 + 100 : ctWidth * 150 + 50);
>> 
>> Why not just use (ctWidth + 1) * 75 + 50 ?
>> 
>> Is there some reason why the lower 75% of the [-1, 1] range takes up 50% of the output space in CSS?
> 
> If you use just a regular linear transfer function, you get wrong results. Instead, something more complicated is necessary, such as this piecewise linear transfer function. Even this one isn't perfect, though - we probably need CoreText SPI to do it correctly.
> 
> The reason is just that this is roughly how Core Text's widths and CSS widths are related. (CSS's widths came from Microsoft's widths originally, so that's the origin.)

I think you should describe why this is the case. The FIXME just says it isn't good.

>>> Source/WebCore/platform/text/TextFlags.h:404
>>> +class FontSelectionValue {
>> 
>> This class doesn't really have anything to do with fonts. Why not call it something like Float16 (in a separate .h file) and then typedef Float16 FontSelectionValue?
> 
> It is kind of related to fonts - the number of fractional bits, the size of the type, and the fact that it's signed are chosen with font selection in mind. I would agree with you if this type was more general purpose (like, if the number of bits was templated), but I don't want to be in the business of implementing a general-purpose fixed-point class.

Fair enough, but I would still prefer the class here should be more accurately named for what it does, and then a typedef to FontSelectionValue. But leaving it as-is is fine.

>>> Source/WebCore/platform/text/TextFlags.h:521
>>> +            *this = other;
>> 
>> In what cases would this happen? Should it be a silent write?
> 
> This is so that you can say something like
> 
> Range r;
> r.extend(otherrange1);
> r.extend(otherrange2);
> r.extend(otherrange3);
> 
> At the end of this, r should encompass otherrange1, 2, and 3, but nothing else.
> 
> If you make the default constructor initialize to { 0, 0 }, then r will always include a point at 0. Instead, we need to have some state to mean "uninitialized". Extending an uninitialized range will cause it to simply become the target range.
> 
> Instead of a separate bool, a cheaper way to do this is to enforce isValid() above.

Wouldn't the correct way to write the above be:

Range r(otherrange1); // Or Range r = otherrange1;
r.expand(otherrange2);
r.expand(otherrange3);

I think that a Range created with the default constructor *should* be { 0, 0 }, and you should initialize a Range correctly if you want something else. You'd need to add a copy constructor and one for FontSelectionRange(FontSelectionValue minimum, FontSelectionValue maximum). But then you can never have an invalid range, which seems ok - you only have it here to make sure that the first expand operates as an assignment.

I see why you did it this way: so that the FontSelectionCapabilities::expand doesn't need to care about what is passed in. Anyway, I don't really mind. It does seem a bit weird that to assign a range you use expand, rather than operator= (even though that would work).