Bug 106774 – [EFL] Update freetype in jhbuild to 2.4.11 and activate subpixel layout

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Dominik Röttsches (drott)]

As per https://bugs.webkit.org/show_bug.cgi?id=102374#c40 we now know that in freetype 2.4.6 an issue with ascent/descent/height calculation is fixed/masked. 
We need to update freetype to at least that version . Since there have been a couple of recommended security updates, let's bump it to latest, 2.4.11 while we're at it.

This will probably require (some) rebaselining - let's do that together with enabling subpixel layout for EFL.

[Dominik Röttsches (drott)]

Let's do that in one go in order to avoid extra rebaselining.

[Dominik Röttsches (drott)]

Created attachment 182566 [details]
Patch

[Laszlo Gombos]

Comment on attachment 182566 [details]
Patch

lgtm.

I trust you with landing this patch and rebaselining.

[Dominik Röttsches (drott)]

Created attachment 182580 [details]
Patch

[Dominik Röttsches (drott)]

Comment on attachment 182580 [details]
Patch

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

Martin, Laszlo reviewed the general change - then I updated it to remove rounding in SimpleFontDataFreetype for subpixel layout enabled ports. Would you take a look at this part and possibly r+? This is mainly what we already discussed in the other bug. Thanks.

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:69
> +    m_fontMetrics.setLineSpacing(ascent + descent + lineGap);

Updated patch with removing rounding, as discussed in https://bugs.webkit.org/show_bug.cgi?id=102374#c20 and following.

[Levi Weintraub]

Very exciting to see this nearly done!

[Martin Robinson]

Comment on attachment 182580 [details]
Patch

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

> Source/WTF/wtf/Platform.h:831
> +#if PLATFORM(CHROMIUM) || PLATFORM(EFL)

From what I understand from Behdad's comments, it doesn't make sense to enable subpixel layout until Cairo supports it. Why enable it here?

[Dominik Röttsches (drott)]

(In reply to comment #7)
> (From update of attachment 182580 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182580&action=review
> 
> > Source/WTF/wtf/Platform.h:831
> > +#if PLATFORM(CHROMIUM) || PLATFORM(EFL)
> 
> From what I understand from Behdad's comments, it doesn't make sense to enable subpixel layout until Cairo supports it. Why enable it here?

Behdad's comments are referring to subpixel hinting and subpixel-positioned text drawing - it's true that this specifically shouldn't be attempted until Cairo supports it. However, this is not what the subpixel layout #define controls. Subpixel Layout improves accuracy in positioning when zooming and doing transformation - when it gets to drawing, subpixel layout breaks the calculation results down to device pixels - (http://trac.webkit.org/wiki/LayoutUnit#LayoutUnitSubpixelLayout)

[Martin Robinson]

(In reply to comment #8)

> Behdad's comments are referring to subpixel hinting and subpixel-positioned text drawing - it's true that this specifically shouldn't be attempted until Cairo supports it. However, this is not what the subpixel layout #define controls. Subpixel Layout improves accuracy in positioning when zooming and doing transformation - when it gets to drawing, subpixel layout breaks the calculation results down to device pixels - (http://trac.webkit.org/wiki/LayoutUnit#LayoutUnitSubpixelLayout)

I'm not sure I grasp how you can use subpixel layout and not have subpixel positioned text. Won't it just constrain the text coordinates to pixels? Does that produce proper results? Maybe since Behdad raised the original concern he could comment here.

[Dominik Röttsches (drott)]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > Behdad's comments are referring to subpixel hinting and subpixel-positioned text drawing - it's true that this specifically shouldn't be attempted until Cairo supports it. However, this is not what the subpixel layout #define controls. Subpixel Layout improves accuracy in positioning when zooming and doing transformation - when it gets to drawing, subpixel layout breaks the calculation results down to device pixels - (http://trac.webkit.org/wiki/LayoutUnit#LayoutUnitSubpixelLayout)
> 
> I'm not sure I grasp how you can use subpixel layout and not have subpixel positioned text. Won't it just constrain the text coordinates to pixels? Does that produce proper results? Maybe since Behdad raised the original concern he could comment here.

Basically what's meant by subpixel layout is that you do your layout calculations with higher resolution than device pixels. In all scenarios where you're downscaling, upscaling, calculating percentages, doing transforms and zooming etc. the higher resolution during the layout calculation leads to more accurate results before you break it down to device pixels. Scaling, zooming and transformations become more accurate. If you'd work with device pixels and integer data types in your calculations, you are forced to round prematurely and the rounding errors can accumulate.

If your text backend supports subpixel positioning, then you can use the result of the internal higher resolution calculations to position text on non-device pixel coordinates, but that's just a bonus. If you don't have that, before rendering the text, you snap to the nearest device pixel coord to position and draw the text. Still, what you gain is better accuracy for transformed and zoomed rendering results.

[Martin Robinson]

(In reply to comment #10)

> > > Behdad's comments are referring to subpixel hinting and subpixel-positioned text drawing - it's true that this specifically shouldn't be attempted until Cairo supports it. However, this is not what the subpixel layout #define controls. Subpixel Layout improves accuracy in positioning when zooming and doing transformation - when it gets to drawing, subpixel layout breaks the calculation results down to device pixels - (http://trac.webkit.org/wiki/LayoutUnit#LayoutUnitSubpixelLayout)
> > 
> > I'm not sure I grasp how you can use subpixel layout and not have subpixel positioned text. Won't it just constrain the text coordinates to pixels? Does that produce proper results? Maybe since Behdad raised the original concern he could comment here.

> If your text backend supports subpixel positioning, then you can use the result of the internal higher resolution calculations to position text on non-device pixel coordinates, but that's just a bonus. If you don't have that, before rendering the text, you snap to the nearest device pixel coord to position and draw the text. Still, what you gain is better accuracy for transformed and zoomed rendering results.

I guess I was really curious about the combination of subpixel-drawn contents and non-subpixel rendered text. Are the results okay?

[Dominik Röttsches (drott)]

(In reply to comment #11)
> I guess I was really curious about the combination of subpixel-drawn contents and non-subpixel rendered text. Are the results okay?

Yes, the results are okay.

Quoting from the link I posted earlier: "Even though layout calculations are done using LayoutUnits the values are aligned to integer pixel values at paint time to line up with device pixels. While most modern graphics libraries support painting with subpixel precision, this results in unwanted anti-aliasing. "

[Martin Robinson]

Comment on attachment 182580 [details]
Patch

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

Looks good, but a couple small comments...

> Source/WTF/ChangeLog:8
> +        Let's update FreeType accordinggly, but bump it to 2.4.11 due to the

Spelling nit: accordingly

>> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:69
>> +#if ENABLE(SUBPIXEL_LAYOUT)
>> +    m_fontMetrics.setLineSpacing(ascent + descent + lineGap);
> 
> Updated patch with removing rounding, as discussed in https://bugs.webkit.org/show_bug.cgi?id=102374#c20 and following.

I understand why you are doing this here, but from what I understand SUBPIXEL_LAYOUT isn't really accurate here. It would be more accurate to say PLATFORM(EFL) since this fix is independent of subpixel layout. Or am I misunderstanding this?

[Dominik Röttsches (drott)]

Created attachment 182785 [details]
Patch v3, review comments addressed.

[Dominik Röttsches (drott)]

Created attachment 182789 [details]
Patch v4, reviewer lines edited.

[Dominik Röttsches (drott)]

(In reply to comment #13)

Thanks very much for the r+, Martin - glad we could resolve this.

> (From update of attachment 182580 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182580&action=review
> 
> Looks good, but a couple small comments...
> 
> > Source/WTF/ChangeLog:8
> > +        Let's update FreeType accordinggly, but bump it to 2.4.11 due to the
> 
> Spelling nit: accordingly

Fixed in all ChangeLogs.

> >> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:69
> >> +#if ENABLE(SUBPIXEL_LAYOUT)
> >> +    m_fontMetrics.setLineSpacing(ascent + descent + lineGap);
> > 
> > Updated patch with removing rounding, as discussed in https://bugs.webkit.org/show_bug.cgi?id=102374#c20 and following.
> 
> I understand why you are doing this here, but from what I understand SUBPIXEL_LAYOUT isn't really accurate here. It would be more accurate to say PLATFORM(EFL) since this fix is independent of subpixel layout. Or am I misunderstanding this?

No, it's related but not strictly a consequence of enabling subpixel layout. You're right - I changed that to #if PLATFORM(EFL).

[Dominik Röttsches (drott)]

Committed r140111: <http://trac.webkit.org/changeset/140111>

[Michael Catanzaro]

(In reply to comment #16)
> No, it's related but not strictly a consequence of enabling subpixel layout.
> You're right - I changed that to #if PLATFORM(EFL).

Hi from the future. This looks really suspicious to me:

#if PLATFORM(EFL)
    m_fontMetrics.setLineSpacing(ascent + descent + lineGap);
#else
    // Match CoreGraphics metrics.
    m_fontMetrics.setLineSpacing(lroundf(ascent) + lroundf(descent) + lroundf(lineGap));
#endif

I don't understand all the previous comments in this bug, and I'm very confused why EFL wanted to have different line spacing behavior than other WebKit ports. Regardless, since EFL is the only port using that first codepath, it's going to be removed now. If it's something that other ports should have been doing, please file a new bug.