Bug 76520 – [EFL] Bad text rendering since r101343

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

EFL layout test reports about 4,000 test cases are fail after r101343.

= After r101343 =
 Results: 16084/27955 tests passed (57.5%) => Tests to be fixed (11871):
  6 DumpRenderTree crashes ( 0.1%) 1 test timed out ( 0.0%)
  4498 text diff mismatch (37.9%)
  9 image mismatch ( 0.1%)
  7357 skipped (62.0%)

= Before r101343 =
 Results: 20201/27955 tests passed (72.3%) => Tests to be fixed (7754):
  6 DumpRenderTree crashes ( 0.1%)
  382 text diff mismatch ( 4.9%)
  9 image mismatch ( 0.1%)
  7357 skipped (94.9%)

We should fix this issue.

[Martin Robinson]

The patch for GTK+ should have fixed text rendering for EFL, but you may still want to disable metrics hinting for DRT runs.

[Gyuyoung Kim]

(In reply to comment #1)
> The patch for GTK+ should have fixed text rendering for EFL, but you may still want to disable metrics hinting for DRT runs.


Hello Martin, I disable metrics hinting for DRT runs. I add above code to setCairoFontOptionsFromFontConfigPattern() in FontPlatformDataFreeType.cpp. 

+    // Turn off text metrics hinting, which quantizes metrics to pixels in device space. 
+   cairo_font_options_set_hint_metrics(options, CAIRO_HINT_METRICS_OFF); 
 }

However, failed test cases are still failed. To disable metrics hinting doesn't influence on EFL layout test result.

[Gyuyoung Kim]

Hello Martin,

Could you explain why lroundf(lineGap) was added ?

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp#L66

lroundf(lineGap) was added by r101343, then it looks result of EFL layout test was influenced. When I remove lroundf(lineGap), failed test cases are almost passed.

[Martin Robinson]

(In reply to comment #3)
> Hello Martin,
> 
> Could you explain why lroundf(lineGap) was added ?
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp#L66
> 
> lroundf(lineGap) was added by r101343, then it looks result of EFL layout test was influenced. When I remove lroundf(lineGap), failed test cases are almost passed.

WildFox will be able to give a better explaination, but I believe this was added to make text rendering more similar to the Mac port.

[Gyuyoung Kim]

Hello Nikolas,

CC'ing Nikolas, could you give an explanation why lineGap is added by r101343 ?

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp#L67

It seems to me that to add lineGap makes many test cases fail on EFL layout test.

[Nikolas Zimmermann]

(In reply to comment #5)
> Hello Nikolas,
> 
> CC'ing Nikolas, could you give an explanation why lineGap is added by r101343 ?
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp#L67
> 
> It seems to me that to add lineGap makes many test cases fail on EFL layout test.

It was an experiment to align the Mac/Gtk ways of text rendering. This made the DRT resuts as compatible as possible, compared to the Mac port. When using the right set of fonts (eg. copied from a Mac) that now leads to the same metrics, unfortunately it's not true when using the Liberation fonts - so if this causes problems feel free to revert it!

[Nikolas Zimmermann]

(In reply to comment #6)
> (In reply to comment #5)
> > Hello Nikolas,
> > 
> > CC'ing Nikolas, could you give an explanation why lineGap is added by r101343 ?
> > 
> > http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp#L67
> > 
> > It seems to me that to add lineGap makes many test cases fail on EFL layout test.
> 
> It was an experiment to align the Mac/Gtk ways of text rendering. This made the DRT resuts as compatible as possible, compared to the Mac port. When using the right set of fonts (eg. copied from a Mac) that now leads to the same metrics, unfortunately it's not true when using the Liberation fonts - so if this causes problems feel free to revert it!

Forgot to say: compare current trunk version with SimpleFontDataMac.mm, and you'll find the code almost matches now.

[Gyuyoung Kim]

(In reply to comment #6)

> It was an experiment to align the Mac/Gtk ways of text rendering. This made the DRT resuts as compatible as possible, compared to the Mac port. When using the right set of fonts (eg. copied from a Mac) that now leads to the same metrics, unfortunately it's not true when using the Liberation fonts - so if this causes problems feel free to revert it!

When I remove "+ lroundf(lineGap)" from #67 line, result of gtk layout test is very different from original result. About 5,000 test cases are more failed. Can I remove "+ lroundf(lineGap)" regardless of the GTK layout test result ?

It seems to me that it is better to do rebaseline the EFL result set for layout test. Because, I think EFL port also wants the DRT result as compatible as possible, compared to Mac, GTK port.

[Nikolas Zimmermann]

(In reply to comment #8)
> When I remove "+ lroundf(lineGap)" from #67 line, result of gtk layout test is very different from original result. About 5,000 test cases are more failed. Can I remove "+ lroundf(lineGap)" regardless of the GTK layout test result ?
> 
> It seems to me that it is better to do rebaseline the EFL result set for layout test. Because, I think EFL port also wants the DRT result as compatible as possible, compared to Mac, GTK port.

Ok, I've discussed this online with gyuyoung. The idea behind this lroundf(lineGap) was to align Mac/Gtk more - and IIRC mrobinson confirmed that it was an visual improvement as well. The current set of EFL results was generated before the lroundf(..) patch went in - so they naturally need a rebaseline - as EFL uses the same FreeType renderer.

Though I think you should judge based on visual experiences. Generate a baseline with eg. the lroundf() patch in, and then remove the patch, rerun tests, and compare the pixel test results, to see what changed visually. If you decide for the EFL port that the old way looks better, you can always add a #if PLATFORM(EFL) exception to the freetype renderer, which is shared between at least gtk/efl.

Another idea might be to make the EFL DRT fallback to platform/gtk results if they're not available in platform/efl, and make sure to leave the lroundf(lineGap) in - this way results in theory could be shared between EFL/GTK, as text rendering is what makes up the most differences in render tree dumps. You should definitely consider this as well.

[Gyuyoung Kim]

Created attachment 124311 [details]
Case of keep lroundf(lineGap)

[Gyuyoung Kim]

Created attachment 124312 [details]
Case of removing lroundf(lineGap)

[Gyuyoung Kim]

Created attachment 124314 [details]
Patch

[Gyuyoung Kim]

Though I compare result of pixel test in some test cases, I couldn't find any visual differences on pixel test result of both. So, I use "#if PLATFORM(EFL)" macro instead of doing rebaseline.

Almost 4,000 test cases are passed.

= Before this patch =

 => Results: 16141/28109 tests passed (57.4%)
 => Tests to be fixed (11968):
      3 tests timed out          ( 0.0%)
     82 no expected results found ( 0.7%)
   4432 text diff mismatch       (37.0%)
     10 image mismatch           ( 0.1%)
   7441 skipped                  (62.2%)

= After this patch =

 => Results: 20151/28109 tests passed (71.7%)
 => Tests to be fixed (7958):
    507 text diff mismatch       ( 6.4%)
     10 image mismatch           ( 0.1%)
   7441 skipped                  (93.5%)

[Raphael Kubo da Costa (:rakuco)]

I tend do prefer less #ifdef cluttering. Looking at the two screenshots, one can't say one looks more correct than the other. How about just rebaselining the images and be done with it?

[Nikolas Zimmermann]

I also think it's not worth diverging, just for avoiding a rebaseline.

[Gyuyoung Kim]

(In reply to comment #15)
> I also think it's not worth diverging, just for avoiding a rebaseline.

I agree with you guys opinion as well. I'm gonna rebaselining the EFL expected result set which are influenced by lroundf(lineGap).

[Gyuyoung Kim]

I just complete rebaseline for EFL port.

Committed r106175: <http://trac.webkit.org/changeset/106175>
Committed r106176: <http://trac.webkit.org/changeset/106176>
Committed r106177: <http://trac.webkit.org/changeset/106177>
Committed r106178: <http://trac.webkit.org/changeset/106178
Committed r106180: <http://trac.webkit.org/changeset/106180>
Committed r106181: <http://trac.webkit.org/changeset/106181>
Committed r106182: <http://trac.webkit.org/changeset/106182>

This is new layout test result after this rebaseline. But, I don't add no expected result set because those need to be added by new bug after correct investigating.

=> Results: 20148/28120 tests passed (71.7%)

=> Tests to be fixed (7972):
     82 no expected results found ( 1.0%)
    432 text diff mismatch       ( 5.4%)
     10 image mismatch           ( 0.1%)
   7448 skipped                  (93.4%)