Pango fixes #384

PreferLinux · 2018-06-18T08:46:24Z

Some updates to the Pango text backend. The commit messages should be adequate explanation, but if they're not just ask.

filipnavara · 2018-07-06T09:20:33Z

@monojenkins build

filipnavara · 2018-07-06T09:27:50Z

src/font.c

+
+		float sizeInPoints = gdip_unit_conversion (font->unit, UnitPoint, gdip_get_display_dpi(), gtMemoryBitmap, font->emSize);
+
+		pango_font_description_set_size (font->pango, sizeInPoints * PANGO_SCALE);


Unfortunately this won't work on the Pango CoreText backend. There's a bug in Pango where the point size is treated as CoreText point size, which is actually not the typographic point size, but something that HTML calls "px" (device independent pixel). One of them 1/72 inch, the other one is 1/96 inch.

Then again, it was broken before, the bug is in Cairo and not here, and the FreeType backend is currently enforced on all platforms anyway... so just a random note :-)

Would pango_font_description_set_absolute_size() (meant to be the size in device units) solve that? I was in two minds about whether it was better to calculate the point size (which I did in the end), or to use that and the pixel size which is already computed on creating the font...

Incidentally it would because the CoreText backend doesn't differentiate them properly, so it would cancel out the bug. I am not sure what is a better option, but I am fine with either one.

I think I settled on the way I did because of the possibility that the output dpi was different to the display dpi (e.g. for a printer). I realise this does not solve that completely, but it does mean the only unit affected is Pixel. I guess the correct thing to do would be to pass in the dpi to use. That'd require a change in #383 (or moving those changes) to set the resolution on the Graphics object used when drawing to a GraphicsPath.

filipnavara · 2018-07-06T09:28:44Z

src/text-pango.c

+		iter = text;
+		iter2 = g_utf8_next_char (iter);
+		g_string_append_len (res, iter, iter2 - iter);
+		j += iter2 - iter;


Would it be possible to make simple unit test for this? Just so we can compare the behavior on Windows and Linux.

Should be, measure a string with, for example, an em-space (the issue I had). I'll have a go tomorrow.

Hmm, doing that was easy enough, but it uncovered something else – the reported number of characters fitted is incorrect when removing multi-byte code points, or removing immediately after them. This change does not make it wrong, it just alters the (incorrect) behaviour slightly.

Well, I hope I've sorted out counting the characters.

But I did a few checks on Windows (via .Net) as to the behaviour there, and it is weird. To me, it looks like unless there is a line ending, the only whitespace removed from the end are actual spaces. If there is a line ending, all trailing whitespace (considering tab as non-whitespace!) is removed. Furthermore, there is no leaving one space / character behind (gives a width of 0). It'd be good to do some testing without .Net in between, although I doubt it'd change. And I'm sure using Cairo text rendering doesn't follow Windows any more than it does here.

I've just added a test case for this (aded2ce). I have also added fixes for the reported number of characters fitted (in two parts, one issue was counting the characters removed (109fe41), the other was in counting the characters in the layout (5b6db10)).

filipnavara · 2018-07-06T09:30:56Z

src/text-pango.c

 			//g_warning("yrange: %d  %d  clipy2: %f", y0 / PANGO_SCALE, y1 / PANGO_SCALE, clipy2);
 			/* StringFormatFlagsLineLimit */
-			if (((fmt->formatFlags & StringFormatFlagsLineLimit) && y1 / PANGO_SCALE >= clipy2) || (y0 / PANGO_SCALE >= clipy2)) {
+			if (((fmt->formatFlags & StringFormatFlagsLineLimit) && y1 / PANGO_SCALE > FrameHeight) || (y0 / PANGO_SCALE >= clipy2)) {


This won't work properly when using the DrawString overloads that take point-only coordinates. After all that's why clipy2 is there in the first place.

This is only hit when FrameHeight is greater than 0 (line 437 / 439), so it shouldn't be a problem. clipy2 only differs from FrameHeight if NoClip is not specified, and in that case is set based on the clip region. However, that specific change relates only to LineLimit, and LineLimit should not clip on the clip region, only on the bounds. Probably something else I should write a unit test for.

Eh, sorry, I missed that. It was me who originally changed it from > to >=. There are some unit tests to cover the general line measurements (0a27652). If I missed something then a unit test would certainly be welcome. I'm generally trying to get a good unit test coverage for the Pango code to have a more convincing case for making it the default (#269).

I guess my intention was that the pixels at FrameHeight are already not part of the rectangle (the general formula that FrameX + FrameHeight - 1 is the last included pixel) and thus y1 >= FrameHeight is already out of the rectangle.

True, however from looking at the Pango source, pango_layout_iter_get_line_yrange is effectively pango_layout_iter_get_line_extents(iter, NULL, logical_rect); ... *y1 = logical_rect->y + logical_rect->height; Therefore y1 is actually not included in the line either, and making > correct. I realise the Pango documentation does not make this clear.

Any reason why not keep the comparison to clipy2 (instead of FrameHeight)? If it was intentional then we could drop the whole clipy2 code, but that would certainly need a test.

Because if NoClip is not set, clipy2 is based on the clip region. The clip region does not affect LineLimit on Windows – it is possible to have LineLimit set, and a clip region that shows a partial line, as long as the bounding box is large enough to fit the whole line.

clipy2 is used later in the line to eliminate drawing lines that are not visible, irrespective of LineLimit.

That still sounds wrong. Imagine this example:

The clip region would cut off last two lines

The bounding box has space for both of the lines

You say that StringFormatFlagsLineLimit doesn't take the clipping region into account, but this loop would still break on the second condition and report wrong results.

Since I was trying to get LineLimit working correctly I never really thought about the second condition except to figure out roughly what it was meant to do. So I never thought about the implications of it (i.e. altering bounds, number of lines / characters fitted). But you're absolutely right. That too should use FrameHeight, and we could indeed get rid of the clipy2 code.

filipnavara

Aside from the clipy2 -> FrameHeight change the rest looks good.

PreferLinux · 2018-07-20T00:26:07Z

src/text-pango.c

+			//g_warning("yrange: %d  %d  FrameHeight: %f", y0 / PANGO_SCALE, y1 / PANGO_SCALE, FrameHeight);
 			/* StringFormatFlagsLineLimit */
-			if (((fmt->formatFlags & StringFormatFlagsLineLimit) && y1 / PANGO_SCALE >= clipy2) || (y0 / PANGO_SCALE >= clipy2)) {
+			if (((fmt->formatFlags & StringFormatFlagsLineLimit) && y1 / PANGO_SCALE > FrameHeight) || (y0 / PANGO_SCALE >= FrameHeight)) {


OK, changed the second contition to use FrameHeight too, and eliminated clipy2 and co.

Looking further, clipy2 and co were of very limited reliability for MeasureString because when used from managed code, the layout rectangle will have either the location or the size, not both. This code only does anything when the layout rectangle has size information, therefore there is a good chance that basing anything on the clip rectangle is meaningless. Drawing would probably not be affected.

filipnavara · 2018-07-25T13:02:31Z

Can you rebase please to resolve the conflict?

Pass pango_font_description_set_size the size in points, not whatever unit font->emSize happens to be using.

Measure the actual space passed in, not just a hard-coded normal space char. The actual whitespace might be something else (e.g. em space).

Check the line fits in with > instead of >= This is actually not as obvious as it might seem, because clipy2 is declared as double. It would be reasonable to expect the comparison to never be equal. However, in a variety of situations clipy2 is set to FrameHeight, which is an integer. It is being compared to y1 divided by PANGO_SCALE, both of which are integers, resulting in the possibility of comparing an integer to a double containing an exact representation of an integer, possibly of the same value.

GdipGetFontCollectionList did not set GpFontFamily.collection for the returned families, resulting in a segmentation fault when getting font metrics with the Pango text rendering backend. fontfamily_init did not initialise the family's collection either. This made the actual problem harder to find, as collection was pointing to invalid memory (collection->config was pointing at a font family).

Unless NoClip is set, clipy2 is the lesser of the clip-region-based height, and the specified height. On Windows, LineLimit is not affected by the clip region irrespective of NoClip. Changed to always compare the bottom of the line with FrameHeight.

The behaviour tested for (measure the first / only character despite trimming) is the expected behaviour throughout libgdiplus and Mono. I do not guarantee it matches Windows.

Looks like the previous code relied on the number of characters removed being equal to the difference in length between the input and output. This does not work correctly with multi-byte characters, as the length difference is in bytes. Instead count the characters being removed.

Previously the maximum length to measure was tested with lastIndex + 1. The problem was that if lastIndex happens to be at a multi-byte character, the + 1 will only allow g_utf8_strlen to measure to part-way through that character. This means the last character is not counted. Also do similarly when moving back when lastIndex >= strlen(layoutText). I'm not sure if this is really needed or not, but there are multi-byte line endings / new lines that may require it.

Forgot to decrement the number of characters removed after adding back the first one on a whitespace-only string. I had it there, but accidentally removed it when excluding something else.

Limiting the layout to the clip region is incorrect, and upsets the reported bounds as well as lines and characters fitted.

PreferLinux · 2018-07-25T21:35:34Z

Done.

PreferLinux · 2018-07-25T22:44:24Z

I'm working on the crash on Mac (involved in counting the number of characters fitted). At the same time, I've spotted a related minor error that I'm fixing as well since I think it makes the fix for the crash cleaner.

I hadn't bothered to check the correct behaviour and had accepted the existing. But if LineLimit prevents fitting any lines, the number of lines fitted should be zero.

When no characters were fitted, g_utf8_prev_char would go off the start of layoutText. This also made lastIndex negative, which was then used to access charsRemoved. Both reads are incorrect. Fixed by handling the case of nothing fitted separately. In that situation charsRemoved is not relevant.

When layoutText is of zero length, after my last fix lastIndex was being left at 0. This meant checking for a line ending (with layoutText[lastIndex + 1]) was off the end of the string.

PreferLinux · 2018-07-26T02:13:45Z

OK, looks like that is sorted now.

filipnavara · 2018-07-27T11:47:02Z

@monojenkins build

filipnavara suggested changes Jul 6, 2018

View reviewed changes

filipnavara suggested changes Jul 17, 2018

View reviewed changes

PreferLinux commented Jul 20, 2018

View reviewed changes

filipnavara approved these changes Jul 25, 2018

View reviewed changes

PreferLinux added 11 commits July 26, 2018 09:30

Pango font description needs the size in points.

64742e3

Pass pango_font_description_set_size the size in points, not whatever unit font->emSize happens to be using.

Pango MeasureString measuring whitespace

18f7fb0

Measure the actual space passed in, not just a hard-coded normal space char. The actual whitespace might be something else (e.g. em space).

Test MeasureString on whitespace-only lines.

47a0216

The behaviour tested for (measure the first / only character despite trimming) is the expected behaviour throughout libgdiplus and Mono. I do not guarantee it matches Windows.

Test MeasureString LineLimit behaviour.

55012d3

Fix mistake in couning characters removed.

bcddd2a

Forgot to decrement the number of characters removed after adding back the first one on a whitespace-only string. I had it there, but accidentally removed it when excluding something else.

Only limit Pango layout to the bounding box.

7eac989

Limiting the layout to the clip region is incorrect, and upsets the reported bounds as well as lines and characters fitted.

PreferLinux force-pushed the pango-fixes branch from 0d3bea2 to 7eac989 Compare July 25, 2018 21:35

PreferLinux added 5 commits July 26, 2018 10:49

Fix MeasureSring expected line count when no lines fitted.

cbf8a50

I hadn't bothered to check the correct behaviour and had accepted the existing. But if LineLimit prevents fitting any lines, the number of lines fitted should be zero.

In MeasureString, don't count lines eliminated by LineLimit.

e2e5e86

Fix another out-of-bounds read in pango_MeasureString.

393311c

When layoutText is of zero length, after my last fix lastIndex was being left at 0. This meant checking for a line ending (with layoutText[lastIndex + 1]) was off the end of the string.

Tweak test parameters to suit a range of fonts.

d4320b5

filipnavara approved these changes Jul 27, 2018

View reviewed changes

marek-safar merged commit 696bbd8 into mono:master Aug 8, 2018

PreferLinux deleted the pango-fixes branch August 8, 2018 08:27


		float sizeInPoints = gdip_unit_conversion (font->unit, UnitPoint, gdip_get_display_dpi(), gtMemoryBitmap, font->emSize);

		pango_font_description_set_size (font->pango, sizeInPoints * PANGO_SCALE);

Pango fixes #384

Pango fixes #384

Uh oh!

Conversation

PreferLinux commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filipnavara commented Jul 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PreferLinux Jul 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipnavara Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipnavara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipnavara commented Jul 25, 2018

Uh oh!

PreferLinux commented Jul 25, 2018

Uh oh!

PreferLinux commented Jul 25, 2018

Uh oh!

PreferLinux commented Jul 26, 2018

Uh oh!

filipnavara commented Jul 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PreferLinux commented Jun 18, 2018 •

edited

Loading

PreferLinux Jul 7, 2018 •

edited

Loading

filipnavara Jul 18, 2018 •

edited

Loading