|
LacaK
2019-03-20 09:34
developer
|
|
|
|
LacaK
2019-03-20 09:35
developer
|
|
|
|
LacaK
2019-03-20 09:36
developer
|
|
|
|
LacaK
2019-03-21 09:38
developer
|
|
|
|
|
I have attached "test-noembed.pdf" (generated using poNoEmbeddedFonts option) where is NO-embedded font inside file.
Look at Font definition and Font descriptor especialy for /W and /MissingWidth.
IMO extracted values are too high to be right ...
(so probably there is specific problem with "Courier New" font parsing)
|
|
|
|
0001-fppdf-fixes-glyph-width-range-check-error-for-monosp.patch (1,824 bytes)
From 09af9dc3fb6ad4d810ea7226fadc2f80b21f9eca Mon Sep 17 00:00:00 2001
From: Graeme Geldenhuys <graemeg@gmail.com>
Date: Thu, 21 Mar 2019 16:14:09 +0000
Subject: [PATCH 1/2] fppdf: fixes glyph width range check error for monospaced
fonts.
---
packages/fcl-pdf/src/fppdf.pp | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/packages/fcl-pdf/src/fppdf.pp b/packages/fcl-pdf/src/fppdf.pp
index 56be0c008a..125322a61f 100644
--- a/packages/fcl-pdf/src/fppdf.pp
+++ b/packages/fcl-pdf/src/fppdf.pp
@@ -1686,14 +1686,30 @@ procedure TPDFTrueTypeCharWidths.Write(const AStream: TStream);
s: string;
lst: TTextMappingList;
lFont: TTFFileInfo;
+ lWidthIndex: integer;
begin
s := '';
lst := Document.Fonts[EmbeddedFontNum].TextMapping;
lst.Sort;
lFont := Document.Fonts[EmbeddedFontNum].FTrueTypeFile;
- // use decimal values for the output
+
+ {$IFDEF gdebug}
+ System.WriteLn('****** isFixedPitch = ', BoolToStr(lFont.PostScript.isFixedPitch > 0, True));
+ System.WriteLn('****** Head.UnitsPerEm := ', lFont.Head.UnitsPerEm );
+ System.WriteLn('****** HHead.numberOfHMetrics := ', lFont.HHead.numberOfHMetrics );
+ {$ENDIF}
+
+ { NOTE: Monospaced fonts may not have a width for every glyph
+ the last one is for subsequent glyphs. }
for i := 0 to lst.Count-1 do
- s := s + Format(' %d [%d]', [ lst[i].GlyphID, TTTFFriendClass(lFont).ToNatural(lFont.Widths[lst[i].GlyphID].AdvanceWidth)]);
+ begin
+ if lst[i].GlyphID < lFont.HHead.numberOfHMetrics then
+ lWidthIndex := lst[i].GlyphID
+ else
+ lWidthIndex := lFont.HHead.numberOfHMetrics-1;
+ s := s + Format(' %d [%d]', [lst[i].GlyphID, TTTFFriendClass(lFont).ToNatural(lFont.Widths[lWidthIndex].AdvanceWidth)])
+ end;
+
WriteString(s, AStream);
end;
--
2.17.1
|
|
|
|
0002-fcl-pdf-ttfdump-fixes-glyph-width-range-check-error-.patch (2,331 bytes)
From 5cd9586f418467806f8336c679623305ac4acd4d Mon Sep 17 00:00:00 2001
From: Graeme Geldenhuys <graemeg@gmail.com>
Date: Thu, 21 Mar 2019 16:14:55 +0000
Subject: [PATCH 2/2] fcl-pdf ttfdump: fixes glyph width range check error for
monospaced fonts.
---
packages/fcl-pdf/utils/ttfdump.lpr | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/packages/fcl-pdf/utils/ttfdump.lpr b/packages/fcl-pdf/utils/ttfdump.lpr
index 9e564b9773..ce2f8df27b 100644
--- a/packages/fcl-pdf/utils/ttfdump.lpr
+++ b/packages/fcl-pdf/utils/ttfdump.lpr
@@ -37,6 +37,21 @@ TFriendClass = class(TTFFileInfo)
{ TMyApplication }
procedure TMyApplication.DumpGlyphIndex;
+
+ procedure PrintGlyphWidth(const aIndex: UInt32);
+ var
+ lWidthIndex: integer;
+ begin
+ { NOTE: Monospaced fonts may not have a width for every glyph
+ the last one is for subsequent glyphs. }
+ if aIndex < FFontFile.HHead.numberOfHMetrics then
+ lWidthIndex := FFontFile.Chars[aIndex]
+ else
+ lWidthIndex := FFontFile.HHead.numberOfHMetrics-1;
+
+ Writeln(Format(' %3d = %d', [FFontFile.Chars[aIndex], TFriendClass(FFontFile).ToNatural(FFontFile.Widths[lWidthIndex].AdvanceWidth)]));
+ end;
+
begin
Writeln('FHHead.numberOfHMetrics = ', FFontFile.HHead.numberOfHMetrics);
Writeln('Length(Chars[]) = ', Length(FFontFile.Chars));
@@ -47,9 +62,9 @@ procedure TMyApplication.DumpGlyphIndex;
Writeln(' U+0048 (H) = ', Format('%d (%0:4.4x)', [FFontFile.Chars[$0048]]));
writeln;
Writeln('Glyph widths:');
- Writeln(' 3 = ', TFriendClass(FFontFile).ToNatural(FFontFile.Widths[FFontFile.Chars[$0020]].AdvanceWidth));
- Writeln(' 4 = ', TFriendClass(FFontFile).ToNatural(FFontFile.Widths[FFontFile.Chars[$0021]].AdvanceWidth));
- Writeln(' H = ', TFriendClass(FFontFile).ToNatural(FFontFile.Widths[FFontFile.Chars[$0048]].AdvanceWidth));
+ PrintGlyphWidth($0020);
+ PrintGlyphWidth($0021);
+ PrintGlyphWidth($0048);
end;
function TMyApplication.GetGlyphIndices(const AText: UnicodeString): TTextMappingList;
@@ -121,6 +136,7 @@ procedure TMyApplication.DoRun;
end;
FFontFile.LoadFromFile(self.GetOptionValue('f'));
+ Writeln('Postscript.IsFixedPitch = ', BoolToStr(FFontFile.PostScript.isFixedPitch > 0, True));
DumpGlyphIndex;
// test #1
--
2.17.1
|
|
|
|
I've attached two pathes.
* One fixes the range check error in the ttfdump utility app.
* The other one fixes the Glyph AdvanceWidth handling of monospaced fonts, also
preventing a range check error.
This seems to resolve this issue for me.
The patches are against FPC Trunk r41755, but I recommend this gets backported to FPC 3.0.4 (fixes branch) too - if possible. |
|
|
|
Applied Graeme's patched, please test and close if OK |
|
|
|
There is still unresolved spacing issue in case when embedded fonts are used (test-embed2.pdf compare to test-noembed2.pdf). Was reported on ML ...
And /MissingWidth has wrong value (which should not be used so probably has no negative impact on document rendering, but still may signals problem)
|
|
|
LacaK
2019-03-27 07:19
developer
|
|
|
|
LacaK
2019-03-27 07:20
developer
|
|
|
|
|
Can confirm - the problem does not appear with all tested monospace fonts (Courier New, DejaVu Sans Mono, Free Mono)
when no poSubsetFont is used in options. |
|
|
|
I made a small change to TFontSubsetter.buildHmtxTable, linked again to the length of FFontInfo.Widths and the glyph index. Then PDF with CurierNew and poSubsetFont looks good.
Tested with FPC 3.2.0 fixed + joellinn's patch from 38354 and the proposed change.
Ubuntu 20.04 64b, Windows XP (32b, cross), Windows 10 (64b, cross).
fpttfsubsetter1.diff (843 bytes)
Index: packages/fcl-pdf/src/fpttfsubsetter.pp
===================================================================
--- packages/fcl-pdf/src/fpttfsubsetter.pp (revision 48225)
+++ packages/fcl-pdf/src/fpttfsubsetter.pp (working copy)
@@ -940,12 +940,18 @@
function TFontSubsetter.buildHmtxTable: TStream;
var
n: integer;
+ GID: longint;
+ LastGID: longint;
begin
Result := TMemoryStream.Create;
+ LastGID := Length(FFontInfo.Widths)-1;
for n := 0 to FGlyphIDs.Count-1 do
begin
- WriteUInt16(Result, FFontInfo.Widths[FGlyphIDs[n].GID].AdvanceWidth);
- WriteInt16(Result, FFontInfo.Widths[FGlyphIDs[n].GID].LSB);
+ GID := FGlyphIDs[n].GID;
+ if GID > LastGID then
+ GID := LastGID;
+ WriteUInt16(Result, FFontInfo.Widths[GID].AdvanceWidth);
+ WriteInt16(Result, FFontInfo.Widths[GID].LSB);
end;
end;
|
|
|
|
Applied second patch, tested:all seems to be working OK, document looks good.
Thanks to everyone for your help ! |
|
|
|
I think the fix for the subsetter in 48696 has still a bug.
If I understand it correctly, the left side bearing values shouldn't be copied from the last TFontInfo. There is another array that contains the remaining LSBs.
MS hmtx doc:
> If numberOfHMetrics is less than the total number of glyphs, then the hMetrics array is followed by an array for the left side bearing values of the remaining glyphs.
I didn't see the LSB used anywhere except in the subsetter but there should probably be a getter function added for it (analog to the advancewidth) |
|
|
|
If I understand the code correctly, the created PDF file with the poSubsetFont option and the Hmtx table, in particular, contain information on all used glyps, i.e. there are no glyphs left. |
|
|
|
You are right, there is an entry in the hmtx table for every glyph.
What I was saying is, that the LSB is wrong in cases where the original glyph ID was larger or equal than the htmx table lenght. While repeating AdvanceWidths may be ommited, the standard requires LSB values for all glyphs to be available (which our subsetter ignores) |
|
|
|
Аh, I finally understood. But the LSB for the remaining glyphs is missing. This array does not load in ParseHmtx. He needs another patch. |
|
|
|
I suggest that TTFFileInfo.Widths always contain information about all glyphs.
fppdf-widths.diff (5,514 bytes)
Index: packages/fcl-pdf/src/fpparsettf.pp
===================================================================
--- packages/fcl-pdf/src/fpparsettf.pp (revision 48808)
+++ packages/fcl-pdf/src/fpparsettf.pp (working copy)
@@ -501,14 +501,30 @@
procedure TTFFileInfo.ParseHmtx(AStream : TStream);
var
i : Integer;
+ LSB: TSmallintArray;
+ LastAdvanceWidth: Word;
begin
- SetLength(FWidths,FHHead.numberOfHMetrics);
- AStream.ReadBuffer(FWidths[0],SizeOf(TLongHorMetric)*Length(FWidths));
+ SetLength(FWidths,FMaxP.numGlyphs);
+
+ AStream.ReadBuffer(FWidths[0],SizeOf(TLongHorMetric)*FHHead.numberOfHMetrics);
for I:=0 to FHHead.NumberOfHMetrics-1 do
begin
FWidths[I].AdvanceWidth:=BEtoN(FWidths[I].AdvanceWidth);
FWidths[I].LSB:=BEtoN(FWidths[I].LSB);
end;
+
+ if FMaxP.numGlyphs > FHHead.NumberOfHMetrics then
+ begin
+ LastAdvanceWidth := FWidths[FHHead.NumberOfHMetrics-1].AdvanceWidth;
+
+ SetLength(LSB, FMaxP.numGlyphs - FHHead.numberOfHMetrics);
+ AStream.ReadBuffer(LSB[0],SizeOf(Int16)*Length(LSB));
+ for I:= FHHead.NumberOfHMetrics to FMaxP.numGlyphs-1 do
+ begin
+ FWidths[I].AdvanceWidth:= LastAdvanceWidth;
+ FWidths[I].LSB:=BEtoN(LSB[I-FHHead.NumberOfHMetrics]);
+ end;
+ end;
end;
@@ -772,7 +788,31 @@
procedure TTFFileInfo.LoadFromStream(AStream : TStream);
var
i: Integer;
- tt : TTTFTableType;
+
+ procedure ParseTable(TT : TTTFTableType);
+ var
+ j: integer;
+ begin
+ for j:=0 to FTableDir.NumTables-1 do
+ begin
+ if TT = GetTableType(FTables[j].Tag) then
+ begin
+ AStream.Position:=FTables[j].Offset;
+ Case TT of
+ tthead: ParseHead(AStream);
+ ttHhea: ParseHhea(AStream);
+ ttmaxp: ParseMaxp(AStream);
+ tthmtx: ParseHmtx(AStream);
+ ttcmap: ParseCmap(AStream);
+ ttname: ParseName(AStream);
+ ttos2 : ParseOS2(AStream);
+ ttPost: ParsePost(AStream);
+ end;
+ break;
+ end;
+ end;
+ end;
+
begin
FOriginalSize:= AStream.Size;
AStream.ReadBuffer(FTableDir,Sizeof(TTableDirectory));
@@ -795,24 +835,17 @@
offset:=BeToN(offset);
Length:=BeToN(Length);
end;
- for I:=0 to FTableDir.NumTables-1 do
- begin
- TT:=GetTableType(FTables[I].Tag);
- if (TT<>ttUnknown) then
- begin
- AStream.Position:=FTables[i].Offset;
- Case TT of
- tthead: ParseHead(AStream);
- ttHhea: ParseHhea(AStream);
- ttmaxp: ParseMaxp(AStream);
- tthmtx: ParseHmtx(AStream);
- ttcmap: ParseCmap(AStream);
- ttname: ParseName(AStream);
- ttos2 : ParseOS2(AStream);
- ttPost: ParsePost(AStream);
- end;
- end;
- end;
+
+ // Consistency is important
+ ParseTable(tthead);
+ ParseTable(ttHhea);
+ ParseTable(ttmaxp);
+ ParseTable(tthmtx);
+ ParseTable(ttcmap);
+ ParseTable(ttname);
+ ParseTable(ttos2);
+ ParseTable(ttPost);
+
end;
procedure TTFFileInfo.PrepareFontDefinition(const Encoding: string; Embed: Boolean);
@@ -930,19 +963,8 @@
end;
function TTFFileInfo.GetAdvanceWidth(AIndex: word): word;
-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;
+ Result := Widths[AIndex].AdvanceWidth;
end;
function TTFFileInfo.ItalicAngle: single;
Index: packages/fcl-pdf/src/fppdf.pp
===================================================================
--- packages/fcl-pdf/src/fppdf.pp (revision 48808)
+++ packages/fcl-pdf/src/fppdf.pp (working copy)
@@ -1747,7 +1747,6 @@
s: string;
lst: TTextMappingList;
lFont: TTFFileInfo;
- lWidthIndex: integer;
begin
s := '';
lst := Document.Fonts[EmbeddedFontNum].TextMapping;
@@ -1760,15 +1759,9 @@
System.WriteLn('****** HHead.numberOfHMetrics := ', lFont.HHead.numberOfHMetrics );
{$ENDIF}
- { NOTE: Monospaced fonts may not have a width for every glyph
- the last one is for subsequent glyphs. }
for i := 0 to lst.Count-1 do
begin
- if lst[i].GlyphID < lFont.HHead.numberOfHMetrics then
- lWidthIndex := lst[i].GlyphID
- else
- lWidthIndex := lFont.HHead.numberOfHMetrics-1;
- s := s + Format(' %d [%d]', [lst[i].GlyphID, TTTFFriendClass(lFont).ToNatural(lFont.Widths[lWidthIndex].AdvanceWidth)])
+ s := s + Format(' %d [%d]', [lst[i].GlyphID, TTTFFriendClass(lFont).ToNatural(lFont.Widths[lst[i].GlyphID].AdvanceWidth)])
end;
WriteString(s, AStream);
Index: packages/fcl-pdf/src/fpttfsubsetter.pp
===================================================================
--- packages/fcl-pdf/src/fpttfsubsetter.pp (revision 48808)
+++ packages/fcl-pdf/src/fpttfsubsetter.pp (working copy)
@@ -940,18 +940,12 @@
function TFontSubsetter.buildHmtxTable: TStream;
var
n: integer;
- GID: longint;
- LastGID: longint;
begin
Result := TMemoryStream.Create;
- LastGID := Length(FFontInfo.Widths)-1;
for n := 0 to FGlyphIDs.Count-1 do
begin
- GID := FGlyphIDs[n].GID;
- if GID > LastGID then
- GID := LastGID;
- WriteUInt16(Result, FFontInfo.Widths[GID].AdvanceWidth);
- WriteInt16(Result, FFontInfo.Widths[GID].LSB);
+ WriteUInt16(Result, FFontInfo.Widths[FGlyphIDs[n].GID].AdvanceWidth);
+ WriteInt16(Result, FFontInfo.Widths[FGlyphIDs[n].GID].LSB);
end;
end;
|
|
|
|
Yes, that is possible and I already suggested it in https://bugs.freepascal.org/view.php?id=38354#c128314
> That however is a design decision to be made by the maintainers ("how closely should TTFFileInfo mirror the actual structure of the File")
A question I can't or don't want to answer... |
|
|
|
I think the TTFFileInfo should mirror the actual structure of the file. |
|
|
|
I made another patch first. With permanent LSB array. Exactly "mirror". But I didn't send it because
"Be careful when working with Widths[] and glyph Index" remained. |
|
|
|
Here is the other patch.
fppdf-lsb.diff (4,415 bytes)
Index: packages/fcl-pdf/src/fpparsettf.pp
===================================================================
--- packages/fcl-pdf/src/fpparsettf.pp (revision 48840)
+++ packages/fcl-pdf/src/fpparsettf.pp (working copy)
@@ -255,6 +255,7 @@
FOS2Data : TOS2Data;
FPostScript : TPostScript;
FWidths: TLongHorMetricArray; // hmtx data
+ FLSB: TSmallintArray; // Left side bearings for glyph IDs >= numberOfHMetrics
// Needed to create PDF font def.
FOriginalSize : Cardinal;
FMissingWidth: Integer;
@@ -313,6 +314,9 @@
Function CapHeight: SmallInt;
{ Returns the glyph advance width, based on the AIndex (glyph index) value. The result is in font units. }
function GetAdvanceWidth(AIndex: word): word;
+ { Returns the glyph left side bearings, based on the AIndex (glyph index) value. The result is in font units. }
+ function GetLSB(AIndex: word): SmallInt;
+
function ItalicAngle: single;
{ max glyph bounding box values - as space separated values }
function BBox: string;
@@ -509,6 +513,16 @@
FWidths[I].AdvanceWidth:=BEtoN(FWidths[I].AdvanceWidth);
FWidths[I].LSB:=BEtoN(FWidths[I].LSB);
end;
+
+ if FMaxP.numGlyphs > FHHead.numberOfHMetrics then
+ begin
+ SetLength(FLSB, FMaxP.numGlyphs - FHHead.numberOfHMetrics);
+ AStream.ReadBuffer(FLSB[0],SizeOf(Int16)*Length(FLSB));
+ for I:=0 to Length(FLSB)-1 do
+ begin
+ FLSB[I] := BEtoN(FLSB[I]);
+ end;
+ end;
end;
@@ -772,7 +786,31 @@
procedure TTFFileInfo.LoadFromStream(AStream : TStream);
var
i: Integer;
- tt : TTTFTableType;
+
+ procedure ParseTable(TT : TTTFTableType);
+ var
+ j: integer;
+ begin
+ for j:=0 to FTableDir.NumTables-1 do
+ begin
+ if TT = GetTableType(FTables[j].Tag) then
+ begin
+ AStream.Position:=FTables[j].Offset;
+ Case TT of
+ tthead: ParseHead(AStream);
+ ttHhea: ParseHhea(AStream);
+ ttmaxp: ParseMaxp(AStream);
+ tthmtx: ParseHmtx(AStream);
+ ttcmap: ParseCmap(AStream);
+ ttname: ParseName(AStream);
+ ttos2 : ParseOS2(AStream);
+ ttPost: ParsePost(AStream);
+ end;
+ break;
+ end;
+ end;
+ end;
+
begin
FOriginalSize:= AStream.Size;
AStream.ReadBuffer(FTableDir,Sizeof(TTableDirectory));
@@ -795,24 +833,16 @@
offset:=BeToN(offset);
Length:=BeToN(Length);
end;
- for I:=0 to FTableDir.NumTables-1 do
- begin
- TT:=GetTableType(FTables[I].Tag);
- if (TT<>ttUnknown) then
- begin
- AStream.Position:=FTables[i].Offset;
- Case TT of
- tthead: ParseHead(AStream);
- ttHhea: ParseHhea(AStream);
- ttmaxp: ParseMaxp(AStream);
- tthmtx: ParseHmtx(AStream);
- ttcmap: ParseCmap(AStream);
- ttname: ParseName(AStream);
- ttos2 : ParseOS2(AStream);
- ttPost: ParsePost(AStream);
- end;
- end;
- end;
+
+ ParseTable(tthead);
+ ParseTable(ttHhea);
+ ParseTable(ttmaxp);
+ ParseTable(tthmtx);
+ ParseTable(ttcmap);
+ ParseTable(ttname);
+ ParseTable(ttos2);
+ ParseTable(ttPost);
+
end;
procedure TTFFileInfo.PrepareFontDefinition(const Encoding: string; Embed: Boolean);
@@ -945,6 +975,15 @@
Result := Widths[i].AdvanceWidth;
end;
+function TTFFileInfo.GetLSB(AIndex: word): SmallInt;
+begin
+ if AIndex < Length(FWidths) then
+ Result := FWidths[AIndex].LSB
+ else
+ Result := FLSB[AIndex-Length(FWidths)];
+end;
+
+
function TTFFileInfo.ItalicAngle: single;
begin
Result := FPostScript.ItalicAngle / 65536.0;
Index: packages/fcl-pdf/src/fpttfsubsetter.pp
===================================================================
--- packages/fcl-pdf/src/fpttfsubsetter.pp (revision 48840)
+++ packages/fcl-pdf/src/fpttfsubsetter.pp (working copy)
@@ -940,18 +940,12 @@
function TFontSubsetter.buildHmtxTable: TStream;
var
n: integer;
- GID: longint;
- LastGID: longint;
begin
Result := TMemoryStream.Create;
- LastGID := Length(FFontInfo.Widths)-1;
for n := 0 to FGlyphIDs.Count-1 do
begin
- GID := FGlyphIDs[n].GID;
- if GID > LastGID then
- GID := LastGID;
- WriteUInt16(Result, FFontInfo.Widths[GID].AdvanceWidth);
- WriteInt16(Result, FFontInfo.Widths[GID].LSB);
+ WriteUInt16(Result, FFontInfo.GetAdvanceWidth(FGlyphIDs[n].GID));
+ WriteInt16(Result, FFontInfo.GetLSB(FGlyphIDs[n].GID));
end;
end;
|
|
|
|
An extra array for the trailing LSBs seems like a nice idea. Or at least the best way to follow the original file structure. |
|
|
|
I am reopening... According to latest discussion it seems, that another patch is provided ... |
|
|
|
From my tests, the original patch fixed the problem.
So please don't reopen a bug for something new, even if it concerns functionality related to the original bug.
If you want to introduce new functionality or changes to the API, this is OK, but
please open a new bugreport and explain why it is needed, with an example. |
|
|
|
As far as discussion continued in original bug report by other users, I reopened.
I did not know whether the users involved were aware that the provided patches did not have to be taken into account.
So please open another bug report if you think (Rumen Gyurov & joellinn), that there is still room to improve things. |
|
