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.
Let's do that in one go in order to avoid extra rebaselining.
Created attachment 182566 [details] Patch
Comment on attachment 182566 [details] Patch lgtm. I trust you with landing this patch and rebaselining.
Created attachment 182580 [details] Patch
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.
Very exciting to see this nearly done!
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?
(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)
(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.
(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.
(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?
(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. "
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?
Created attachment 182785 [details] Patch v3, review comments addressed.
Created attachment 182789 [details] Patch v4, reviewer lines edited.
(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).
Committed r140111: <http://trac.webkit.org/changeset/140111>
(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.