View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0035251 | FPC | FCL | public | 2019-03-20 09:34 | 2021-02-26 14:02 |
Reporter | LacaK | Assigned To | Michael Van Canneyt | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
OS | Windows | ||||
Product Version | 3.0.4 | ||||
Fixed in Version | 3.3.1 | ||||
Summary | 0035251: fcl-pdf: Writing text in "Courier New" TTF font generates wrong PDF document | ||||
Description | Try run attached program an look on outputted PDF file. There are 4 lines in : - Courier New, Arial, Verdana and Consolas fonts - 1st line in Courier New font is wrong, other three lines in another fonts are okay. | ||||
Additional Information | Attached test program, generated PDF file and screen shot of content of PDF file | ||||
Tags | fcl-pdf, fpPDF | ||||
Fixed in Revision | 48696 | ||||
FPCOldBugId | 0 | ||||
FPCTarget | 3.2.2 | ||||
Attached Files |
|
related to | 0038354 | resolved | Michael Van Canneyt | TFPFontCacheItem.TextWidth() reports wrong value for (some?) monospace fonts |
has duplicate | 0036881 | closed | Michael Van Canneyt | fcl-pdf: For some monospace TTF cyrillic text not rendered properly |
|
|
|
|
|
|
|
|
|
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) |
|
|
|
|
|
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. |
Date Modified | Username | Field | Change |
---|---|---|---|
2019-03-20 09:34 | LacaK | New Issue | |
2019-03-20 09:34 | LacaK | File Added: test_fpPDF.lpr | |
2019-03-20 09:35 | LacaK | File Added: test.pdf | |
2019-03-20 09:36 | LacaK | File Added: CourierNew.PNG | |
2019-03-20 09:45 | LacaK | Summary | Writing text in "Courier New" TTF font generates wrong PDF document => fcl-pdf: Writing text in "Courier New" TTF font generates wrong PDF document |
2019-03-20 09:45 | LacaK | Tag Attached: fcl-pdf | |
2019-03-20 09:45 | LacaK | Tag Attached: fpPDF | |
2019-03-21 09:38 | LacaK | File Added: test-noembed.pdf | |
2019-03-21 09:40 | LacaK | Note Added: 0114956 | |
2019-03-21 09:47 | LacaK | Note Edited: 0114956 | View Revisions |
2019-03-21 11:04 | LacaK | Note Edited: 0114956 | View Revisions |
2019-03-21 17:18 | Graeme Geldenhuys | File Added: 0001-fppdf-fixes-glyph-width-range-check-error-for-monosp.patch | |
2019-03-21 17:19 | Graeme Geldenhuys | File Added: 0002-fcl-pdf-ttfdump-fixes-glyph-width-range-check-error-.patch | |
2019-03-21 17:22 | Graeme Geldenhuys | Note Added: 0114965 | |
2019-03-22 11:28 | Michael Van Canneyt | Assigned To | => Michael Van Canneyt |
2019-03-22 11:28 | Michael Van Canneyt | Status | new => assigned |
2019-03-26 22:37 | Michael Van Canneyt | Fixed in Revision | => 41800 |
2019-03-26 22:37 | Michael Van Canneyt | Note Added: 0115069 | |
2019-03-26 22:37 | Michael Van Canneyt | Status | assigned => resolved |
2019-03-26 22:37 | Michael Van Canneyt | Fixed in Version | => 3.3.1 |
2019-03-26 22:37 | Michael Van Canneyt | Resolution | open => fixed |
2019-03-26 22:37 | Michael Van Canneyt | Target Version | => 3.2.0 |
2019-03-27 07:19 | LacaK | Note Added: 0115075 | |
2019-03-27 07:19 | LacaK | Status | resolved => feedback |
2019-03-27 07:19 | LacaK | Resolution | fixed => reopened |
2019-03-27 07:19 | LacaK | Status | feedback => acknowledged |
2019-03-27 07:19 | LacaK | File Added: test-embed2.pdf | |
2019-03-27 07:20 | LacaK | File Added: test-noembed2.pdf | |
2019-03-27 07:22 | LacaK | Note Edited: 0115075 | View Revisions |
2019-03-27 07:23 | LacaK | Note Edited: 0115075 | View Revisions |
2019-12-05 16:08 | Michael Van Canneyt | Target Version | 3.2.0 => |
2019-12-05 16:08 | Michael Van Canneyt | FPCTarget | => - |
2020-04-07 07:17 | LacaK | Relationship added | related to 0036881 |
2020-04-09 16:18 | Anton Kavalenka | Note Added: 0122045 | |
2020-04-09 23:39 | Michael Van Canneyt | Relationship replaced | has duplicate 0036881 |
2021-01-20 15:21 | Rumen Gyurov | Note Added: 0128451 | |
2021-01-20 15:21 | Rumen Gyurov | File Added: fpttfsubsetter1.diff | |
2021-01-20 17:13 | Michael Van Canneyt | Relationship added | related to 0038354 |
2021-02-17 14:32 | Michael Van Canneyt | Status | acknowledged => resolved |
2021-02-17 14:32 | Michael Van Canneyt | Fixed in Revision | 41800 => 48696 |
2021-02-17 14:32 | Michael Van Canneyt | FPCTarget | - => 3.2.2 |
2021-02-17 14:32 | Michael Van Canneyt | Note Added: 0128974 | |
2021-02-17 14:33 | Michael Van Canneyt | Resolution | reopened => fixed |
2021-02-21 15:25 | joellinn | Note Added: 0129062 | |
2021-02-22 18:21 | Rumen Gyurov | Note Added: 0129104 | |
2021-02-24 10:08 | joellinn | Note Added: 0129139 | |
2021-02-25 08:58 | Rumen Gyurov | Note Added: 0129154 | |
2021-02-26 10:04 | Rumen Gyurov | Note Added: 0129169 | |
2021-02-26 10:04 | Rumen Gyurov | File Added: fppdf-widths.diff | |
2021-02-26 13:59 | joellinn | Note Added: 0129171 | |
2021-02-26 14:02 | Michael Van Canneyt | Note Added: 0129172 |