View Issue Details

IDProjectCategoryView StatusLast Update
0038354FPCPackagespublic2021-02-17 14:26
Reporterjoellinn Assigned ToMichael Van Canneyt  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformAll 
Fixed in Version3.3.1 
Summary0038354: TFPFontCacheItem.TextWidth() reports wrong value for (some?) monospace fonts
DescriptionSize calculation is broken for monospace fonts like 'UbuntuMono-Regular' or 'CourierNewPSMT'.

For example ('UbuntuMono-Regular', fontsize=10):
'w' = 437.35
'e' = 0.00
Steps To Reproduce```
const
  pt = 10.0;
var
  s: string;
  w1, w2: single;
begin
  s := THETEXT;
  with gTTFontCache.Find('Ubuntu') do begin
    w1 := TextWidth(s, pt);
  end;
  with gTTFontCache.Find('UbuntuMono-Regular') do begin
    w2 := TextWidth(s, pt);
  end;

  WriteLn(Format('%f %f', [w1, w2]));
```
TagsNo tags attached.
Fixed in Revision48694
FPCOldBugId
FPCTarget3.2.2
Attached Files

Relationships

related to 0035251 resolvedMichael Van Canneyt fcl-pdf: Writing text in "Courier New" TTF font generates wrong PDF document 

Activities

joellinn

2021-01-14 12:02

reporter   ~0128312

Proposed PATCH is attached.
There is an out of bounds array read in the character width table (htmx), which especially effects monospace fonts because they usually have just one entry in that table.
0001-fcl-pdf-fix-for-monospace-fonts.-fixes-38354.patch (2,454 bytes)   
From be62c011961bc3d2dfb981cbf7cebdd4a7e43e01 Mon Sep 17 00:00:00 2001
From: Joel Linn <jl@conductive.de>
Date: Thu, 14 Jan 2021 11:57:08 +0100
Subject: [PATCH] fcl-pdf: fix for monospace fonts. fixes #38354

---
 packages/fcl-pdf/src/fpparsettf.pp | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/packages/fcl-pdf/src/fpparsettf.pp b/packages/fcl-pdf/src/fpparsettf.pp
index fc7c243b5c..9dc055fca4 100644
--- a/packages/fcl-pdf/src/fpparsettf.pp
+++ b/packages/fcl-pdf/src/fpparsettf.pp
@@ -822,13 +822,13 @@ begin
   if embed and not Embeddable then
     raise ETTF.Create(rsFontEmbeddingNotAllowed);
   PrepareEncoding(Encoding);
-//  MissingWidth:=ToNatural(Widths[Chars[CharCodes^[32]]].AdvanceWidth);  // Char(32) - Space character
-  FMissingWidth := Widths[Chars[CharCodes^[32]]].AdvanceWidth;  // Char(32) - Space character
+//  MissingWidth:=ToNatural(GetAdvanceWidth(Chars[CharCodes^[32]]));  // Char(32) - Space character
+  FMissingWidth := GetAdvanceWidth(Chars[CharCodes^[32]]);  // Char(32) - Space character
   for I:=0 to 255 do
   begin
     if (CharCodes^[i]>=0) and (CharCodes^[i]<=High(Chars))
-    and (Widths[Chars[CharCodes^[i]]].AdvanceWidth> 0) and (CharNames^[i]<> '.notdef') then
-      CharWidth[I]:= ToNatural(Widths[Chars[CharCodes^[I]]].AdvanceWidth)
+    and (GetAdvanceWidth(Chars[CharCodes^[i]])> 0) and (CharNames^[i]<> '.notdef') then
+      CharWidth[I]:= ToNatural(GetAdvanceWidth(Chars[CharCodes^[I]]))
     else
       CharWidth[I]:= FMissingWidth;
   end;
@@ -930,8 +930,19 @@ begin
 end;
 
 function TTFFileInfo.GetAdvanceWidth(AIndex: word): word;
-begin
-  Result := Widths[AIndex].AdvanceWidth;
+var
+  i: SizeInt;
+begin
+  // There may be more glyphs than elements in the array, in which
+  // case the last entry is to be used.
+  // https://docs.microsoft.com/en-us/typography/opentype/spec/hmtx
+  i := Length(Widths);
+  if AIndex >= i then
+    Dec(i)
+  else
+    i := AIndex;
+
+  Result := Widths[i].AdvanceWidth;
 end;
 
 function TTFFileInfo.ItalicAngle: single;
@@ -972,7 +983,7 @@ function TTFFileInfo.GetMissingWidth: integer;
 begin
   if FMissingWidth = 0 then
   begin
-    FMissingWidth := Widths[Chars[CharCodes^[32]]].AdvanceWidth;  // 32 is in reference to the Space character
+    FMissingWidth := GetAdvanceWidth(Chars[CharCodes^[32]]);  // 32 is in reference to the Space character
   end;
   Result := FMissingWidth;
 end;
-- 
2.27.0

joellinn

2021-01-14 12:35

reporter   ~0128314

There is a reference to the width array in the ttfsubsetter that needs to be fixed as well. It also reads the LSB values which should be handled differently according to // https://docs.microsoft.com/en-us/typography/opentype/spec/hmtx . They are not truncated.

Maybe it's a better idea to always do SetLength(FWidths, GlyphCount) inside ParseHmtx, fill it like now until numberOfHMetrics. After that, read the packed LSB array into FWidths by adding the last advanceWidth to every entry. It wastes a bit of RAM but removes the complexity from accessing the values (and automatically fixes all software that uses Widths[i] instead of GetAdcanceWidth()). That however is a design decision to be made by the maintainers ("how closely should TTFFileInfo mirror the actual structure of the File")

Michael Van Canneyt

2021-02-17 14:26

administrator   ~0128971

Applied, thank you very much !

Issue History

Date Modified Username Field Change
2021-01-14 01:41 joellinn New Issue
2021-01-14 12:02 joellinn Note Added: 0128312
2021-01-14 12:02 joellinn File Added: 0001-fcl-pdf-fix-for-monospace-fonts.-fixes-38354.patch
2021-01-14 12:35 joellinn Note Added: 0128314
2021-01-14 16:54 Michael Van Canneyt Assigned To => Michael Van Canneyt
2021-01-14 16:54 Michael Van Canneyt Status new => assigned
2021-01-20 17:13 Michael Van Canneyt Relationship added related to 0035251
2021-02-17 14:26 Michael Van Canneyt Status assigned => resolved
2021-02-17 14:26 Michael Van Canneyt Resolution open => fixed
2021-02-17 14:26 Michael Van Canneyt Fixed in Version => 3.3.1
2021-02-17 14:26 Michael Van Canneyt Fixed in Revision => 48694
2021-02-17 14:26 Michael Van Canneyt FPCTarget => 3.2.2
2021-02-17 14:26 Michael Van Canneyt Note Added: 0128971