Bug 103403 - Harfbuzz sal common layout of Graphite fonts over the margin
Summary: Harfbuzz sal common layout of Graphite fonts over the margin
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: graphics stack (show other bugs)
Version:
(earliest affected)
5.3.0.0.alpha0+
Hardware: All Windows (All)
: medium normal
Assignee: ⁨خالد حسني⁩
URL:
Whiteboard: target:5.3.0
Keywords:
Depends on:
Blocks: HarfBuzz
  Show dependency treegraph
 
Reported: 2016-10-22 07:10 UTC by V Stuart Foote
Modified: 2016-11-01 00:22 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
canvas layout with no Hardware acceleration (39.26 KB, image/png)
2016-10-22 15:10 UTC, V Stuart Foote
Details
test document for seeing Graphite fonts on canvas (29.80 KB, application/vnd.oasis.opendocument.text)
2016-10-22 15:16 UTC, V Stuart Foote
Details
test document for seeing Graphite fonts on canvas (29.89 KB, application/vnd.oasis.opendocument.text)
2016-10-22 15:24 UTC, V Stuart Foote
Details
Patch to print some debug information (1.45 KB, patch)
2016-10-30 16:31 UTC, ⁨خالد حسني⁩
Details

Note You need to log in before you can comment on or make changes to this bug.
Description V Stuart Foote 2016-10-22 07:10:03 UTC
Description:
On Windows 10 Pro 64-bit (1607) en-US with 
Version: 5.3.0.0.alpha1+
Build ID: 36bafd3d4ad7fa75649eeab0c9cd1b3d6f53d8e8
CPU Threads: 8; OS Version: Windows 6.19; UI Render: GL; 
TinderBox: Win-x86@39, Branch:master, Time: 2016-10-21_18:49:50
Locale: en-US (en_US); Calc: CL

With SAL common layout enabled, the Harfbuzz handling of Graphite fonts is over the margins.

Steps to Reproduce:
1. set SAL_USE_COMMON_LAYOUT=1
2. launch soffice.exe
3. new writer document
4. three pragraphs of the DummyText
5. change font of first to Linux Libertine Display G, change font of second to Biolinum G, and third to Linux Libertine G
6. use status bar zoom slider to shrink and enlarge view

Actual Results:  
canvas is resized but font text as composed extends outside the margins. And on zoom in, font kerning and misspacing is apparent.

Expected Results:
Clean recompose of document canvas as zoom in and out, and more even spacing of characters and kerned pairs.


Reproducible: Always

User Profile Reset: No

Additional Info:


User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Comment 1 V Stuart Foote 2016-10-22 07:22:11 UTC
It is more noticible with OpenGL rendering enabled, but also affects GPU "default" rendering.

With CPU only rendering, the Graphite text gets badly overwritten--so maybe bug 72940 or bug 102916
Comment 2 ⁨خالد حسني⁩ 2016-10-22 10:59:58 UTC
Can you attach test document and screenshots?
Comment 3 V Stuart Foote 2016-10-22 15:10:42 UTC
Created attachment 128159 [details]
canvas layout with no Hardware acceleration

Example of all Graphite fonts being crushed on canvas with no Hardware acceleration.

Windows 10 Pro 64-bit (1607) en-US with
Version: 5.3.0.0.alpha1+
Build ID: 36bafd3d4ad7fa75649eeab0c9cd1b3d6f53d8e8
CPU Threads: 8; OS Version: Windows 6.19; UI Render: default; 
TinderBox: Win-x86@39, Branch:master, Time: 2016-10-21_18:49:50
Locale: en-US (en_US); Calc: CL

Name	NVIDIA GeForce GTX 750 Ti
PNP Device ID	PCI\VEN_10DE&DEV_1380&SUBSYS_37553842&REV_A2\4&1D0A902F&0&0018
Adapter Type	GeForce GTX 750 Ti, NVIDIA compatible
Adapter Description	NVIDIA GeForce GTX 750 Ti
Adapter RAM	(2,147,483,648) bytes
Installed Drivers	C:\WINDOWS\System32\DriverStore\FileRepository\nv_dispi.inf_amd64_d3851cb7c8216f9e\nvd3dumx,C:\WINDOWS\System32\DriverStore\FileRepository\nv_dispi.inf_amd64_d3851cb7c8216f9e\nvwgf2umx,C:\WINDOWS\System32\DriverStore\FileRepository\nv_dispi.inf_amd64_d3851cb7c8216f9e\nvwgf2umx,C:\WINDOWS\System32\DriverStore\FileRepository\nv_dispi.inf_amd64_d3851cb7c8216f9e\nvwgf2umx,C:\WINDOWS\System32\DriverStore\FileRepository\nv_dispi.inf_amd64_d3851cb7c8216f9e\nvd3dum,C:\WINDOWS\System32\DriverStore\FileRepository\nv_dispi.inf_amd64_d3851cb7c8216f9e\nvwgf2um,C:\WINDOWS\System32\DriverStore\FileRepository\nv_dispi.inf_amd64_d3851cb7c8216f9e\nvwgf2um,C:\WINDOWS\System32\DriverStore\FileRepository\nv_dispi.inf_amd64_d3851cb7c8216f9e\nvwgf2um
Driver Version	21.21.13.7270
INF File	oem10.inf (Section078 section)
Color Planes	Not Available
Color Table Entries	4294967296
Resolution	1920 x 1080 x 60 hertz
Bits/Pixel	32
Comment 4 V Stuart Foote 2016-10-22 15:16:43 UTC
Created attachment 128161 [details]
test document for seeing Graphite fonts on canvas

sample document; four styles using our Graphite fonts.

Open and cycle through the rending options from Tools -> Options -> View

Close and reopen soffice as needed to actually change the layout engine, and scale canvas larger and smaller using the status bar's zoom slider.
Comment 5 V Stuart Foote 2016-10-22 15:24:51 UTC
Created attachment 128162 [details]
test document for seeing Graphite fonts on canvas

added a DT sample in default Liberation Serif to the end
Comment 6 ⁨خالد حسني⁩ 2016-10-22 17:17:55 UTC
I can reproduce this with the attached document, but not for ones I create myself nor when the document is opened read-only. Something weird is going on.
Comment 7 ⁨خالد حسني⁩ 2016-10-30 16:27:24 UTC
This seems to be either a bug in Graphite or its HarfBuzz integration. It seems that on Windows we sometimes get the same advance widths despite the current font scale. This patch should print the advance widths we are getting from HarfBuzz:

diff --git a/vcl/source/gdi/CommonSalLayout.cxx b/vcl/source/gdi/CommonSalLayout.cxx
index 239486e..03d08eb 100644
--- a/vcl/source/gdi/CommonSalLayout.cxx
+++ b/vcl/source/gdi/CommonSalLayout.cxx
@@ -496,6 +496,22 @@ bool CommonSalLayout::LayoutText(ImplLayoutArgs& rArgs)
             hb_glyph_info_t *pHbGlyphInfos = hb_buffer_get_glyph_infos(pHbBuffer, nullptr);
             hb_glyph_position_t *pHbPositions = hb_buffer_get_glyph_positions(pHbBuffer, nullptr);
 
+            if (strcmp(hb_shape_plan_get_shaper(pHbPlan), "graphite2") == 0)
+            {
+#define NUM 200
+                char buf[NUM];
+                hb_buffer_serialize_glyphs(pHbBuffer, 0, std::min(2, nRunGlyphCount),
+                                           buf, NUM, nullptr,
+                                           mpHbFont,
+                                           HB_BUFFER_SERIALIZE_FORMAT_JSON,
+                                           hb_buffer_serialize_flags_t(HB_BUFFER_SERIALIZE_FLAG_DEFAULT | HB_BUFFER_SERIALIZE_FLAG_NO_CLUSTERS)
+                                           );
+                int nX, nY;
+                hb_font_get_scale(mpHbFont, &nX, &nY);
+                SAL_DEBUG(mrFontSelData.GetFamilyName() << "@" << nX << "x" << nY << " : " << buf);
+#undef NUM
+            }
+
             for (int i = 0; i < nRunGlyphCount; ++i) {
                 int32_t nGlyphIndex = pHbGlyphInfos[i].codepoint;
                 int32_t nCharPos = pHbGlyphInfos[i].cluster;


There is a difference in the values we get on Windows and on Linux.

@Martin: Do you have any idea what is going on here, would it be related to the fact that we cache the HarfBuzz font and reuse it with different scales?
Comment 8 ⁨خالد حسني⁩ 2016-10-30 16:31:13 UTC
Created attachment 128362 [details]
Patch to print some debug information

Attaching the patch in the previous commit.
Comment 9 ⁨خالد حسني⁩ 2016-10-30 18:52:33 UTC
Patch sent upstream https://github.com/behdad/harfbuzz/pull/357.
Comment 10 Commit Notification 2016-10-31 03:22:35 UTC
Khaled Hosny committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=3d83c42008ab51202c0577f493e8ed3fde0310b7

tdf#103403: Wrong glyph advances with Graphite

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 11 martin_hosken 2016-10-31 09:45:23 UTC
I suspect that this line is redundant and would cause problems when x_scale and y_scale are not the same:

++ yscale *= yscale / xscale;

Now that you are multiplying everything by its scale, there is no need for a ratio there.

But I doubt libo is using different scales, so it shouldn't be a problem. Anyway a low priority fix.
Comment 12 ⁨خالد حسني⁩ 2016-10-31 11:25:11 UTC
(In reply to martin_hosken from comment #11)
> I suspect that this line is redundant and would cause problems when x_scale
> and y_scale are not the same:
> 
> ++ yscale *= yscale / xscale;
> 
> Now that you are multiplying everything by its scale, there is no need for a
> ratio there.

But xscale and yscale now are the difference between the scale of the HarfBuzz font when the Graphite fonts was created vs when it is used, so if it was the same then both value will be 1. Without this line yscale will always be 1 even if the font has different x and y scales.

> But I doubt libo is using different scales, so it shouldn't be a problem.
> Anyway a low priority fix.

We do actually use different scales for stretched / condensed text.
Comment 13 Commit Notification 2016-11-01 00:22:24 UTC
Khaled Hosny committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=86abe3cb3d4b06ffecced691829c15ba90d00937

Revert "tdf#103403: Wrong glyph advances with Graphite"

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 14 Commit Notification 2016-11-01 00:22:30 UTC
Khaled Hosny committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=e31f7f4c87d5501599daa8d11d08b6e4a8725356

tdf#103403: Wrong glyph advances with Graphite

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.