Close the real gaps in font embedding - #26
Merged
Conversation
📝 WalkthroughWalkthroughThe font pipeline now discovers ChangesFont pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FontResolverBase
participant FontMetadataBackend
participant TrueTypeCollection
participant PdfFont
participant PdfDocument
FontResolverBase->>FontMetadataBackend: discover face metadata
FontResolverBase->>TrueTypeCollection: extract requested collection face
FontResolverBase->>PdfFont: provide resolved font bytes
PdfFont->>PdfDocument: embed OpenType or TrueType program
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR closes several functional gaps around font discovery and embedding in PdfSharpCore: widening system font discovery beyond *.ttf, supporting TrueType/OpenType collections by extracting individual faces in the resolver, correctly embedding CFF/OpenType-PS fonts, and ensuring style simulation flags are actually requested and propagated through to rendering.
Changes:
- Extend discovery/resolution to
.ttf,.otf,.ttc, and.otc, including per-face handling for collections. - Embed CFF fonts correctly as
/FontFile3with/Subtype /OpenType(and raise PDF version to 1.6 when needed), while keeping TrueType fonts subsetted into/FontFile2. - Fix resolver style fallback to request bold/italic simulations when a family lacks a real face, and propagate simulation flags into
XGlyphTypeface.
Reviewed changes
Copilot reviewed 26 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents font discovery scope, always-embed behavior, CFF whole-embed, and style simulation caveats. |
| PdfSharpCore/Utils/TrueTypeCollection.cs | Adds collection parsing and face extraction to produce standalone sfnt fonts. |
| PdfSharpCore/Utils/LinuxSystemFontResolver.cs | Uses shared font-extension filtering for Linux fontconfig + fallback results. |
| PdfSharpCore/Utils/FontResolverBase.cs | Refactors discovery to map face-name→(path,index), adds collection metadata hooks, and implements style-simulation-aware fallback. |
| PdfSharpCore/Utils/FontFileTypes.cs | Centralizes supported font extensions and directory enumeration. |
| PdfSharpCore/Pdf.Advanced/PdfTrueTypeFont.cs | Routes embedding through shared EmbedFontProgram helper. |
| PdfSharpCore/Pdf.Advanced/PdfFont.cs | Introduces shared font-program embedding logic for TrueType vs CFF. |
| PdfSharpCore/Internal/FontFamilyModel.cs | Clarifies that stored “FontFiles” values are resolver face names (not paths). |
| PdfSharpCore/Fonts/PlatformFontResolverInfo.cs | Removes dead/unreachable platform-resolver info type. |
| PdfSharpCore/Fonts/PlatformFontResolver.cs | Removes dead/unreachable platform resolver implementation. |
| PdfSharpCore/Fonts/FontResolverInfo.cs | Updates docs: simulation flags are meaningful; collectionNumber is vestigial with resolver-side extraction. |
| PdfSharpCore/Fonts/FontFactory.cs | Removes unreachable platform resolver fallback path; relies on configured GlobalFontSettings.FontResolver. |
| PdfSharpCore/Fonts.OpenType/enums/TableTagNames.cs | Fixes CFF table tag to 4-byte "CFF " form. |
| PdfSharpCore/Drawing/XGlyphTypeface.cs | Plumbs StyleSimulations into glyph typeface construction so renderer can apply them. |
| PdfSharpCore/Drawing/XFontSource.cs | Notes that collections are extracted in the resolver and never reach XFontSource. |
| PdfSharpCore.Test/PdfSharpCore.Test.csproj | Ensures .otf test assets are copied to output. |
| PdfSharpCore.Test/Helpers/TrueTypeCollectionBuilder.cs | Adds a test helper to synthesize .ttc files from existing .ttf assets. |
| PdfSharpCore.Test/Helpers/PinnedFontResolver.cs | Adds a pinned CFF family/face and returns style simulation flags for it. |
| PdfSharpCore.Test/Fonts/TrueTypeCollectionTest.cs | Adds tests for collection face counting, extraction correctness, and backend metadata parity. |
| PdfSharpCore.Test/Fonts/StyleSimulationTest.cs | Adds tests that fallback chooses nearest face and only simulates missing axes; verifies renderer behavior. |
| PdfSharpCore.Test/Fonts/PostscriptOutlineEmbeddingTest.cs | Adds tests asserting correct CFF embedding dictionary structure, bytes, descendant subtype, and PDF version bump. |
| PdfSharpCore.Test/Assets/Fonts/LICENSE.txt | Updates licensing text to cover the newly added .otf asset. |
| PdfSharpCore.Skia/SkiaFontResolver.cs | Implements per-face and batched collection metadata reading for Skia backend. |
| PdfSharpCore.Skia/OpenTypeFontMetadata.cs | Adds face-indexed collection metadata reading and a batched “read all faces” path. |
| PdfSharpCore.ImageSharp/ImageSharpFontResolver.cs | Adds collection-aware metadata reading via ImageSharp’s collection APIs. |
| docs/specs/font-embedding-gaps.md | Adds a detailed design/spec describing the six gaps and the implemented approach. |
Comments suppressed due to low confidence (2)
PdfSharpCore/Utils/TrueTypeCollection.cs:160
- Potential integer overflow in
records + tableCount * TableRecordLengthbounds check. Uselongarithmetic so malformed/crafted font data can't bypass the validation via overflow.
int records = directory + OffsetTableLength;
if (records + tableCount * TableRecordLength > data.Length)
throw new InvalidOperationException("Font collection face declares more tables than the file holds.");
PdfSharpCore/Utils/TrueTypeCollection.cs:175
- Potential integer overflow in
offset + lengthbounds check. If the sum overflows it can bypass validation and then hitBuffer.BlockCopywith out-of-range arguments. Uselongarithmetic for the addition.
int record = records + idx * TableRecordLength;
int offset = (int)U32(data, record + 8);
int length = (int)U32(data, record + 12);
if (offset < 0 || length < 0 || offset + length > data.Length)
throw new InvalidOperationException("Font collection face points at table data outside the file.");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+149
to
+151
| int directory = (int)U32(data, OffsetTableLength + faceIndex * 4); | ||
| if (directory < 0 || directory + OffsetTableLength > data.Length) | ||
| throw new InvalidOperationException("Font collection points at a face outside the file."); |
Comment on lines
+137
to
+140
| // '/Subtype /OpenType' arrives in PDF 1.6. Raising the version is the honest thing | ||
| // to do; lowering it is not this method's business. | ||
| if (Owner._version < 16) | ||
| Owner._version = 16; |
Comment on lines
+308
to
311
| _facePaths = facePaths; | ||
| // Written last: its volatile write publishes _facePaths to EnsureInitialized, | ||
| // which reads that field to decide whether both are ready. | ||
| _installedFonts = installedFonts; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
PdfSharpCore.Test/Fonts/PostscriptOutlineEmbeddingTest.cs (2)
127-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo small cleanups.
Line 127:
renderedholds a nativeMagickImageCollectionand is never disposed — worth ausingalongside the cloned page. Line 208:Resolvenever uses itsdocumentparameter; drop it or use it to resolve the reference through the document's object table.Also applies to: 208-211
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PdfSharpCore.Test/Fonts/PostscriptOutlineEmbeddingTest.cs` around lines 127 - 135, Dispose the native rasterized image collection by declaring rendered from PdfHelper.Rasterize within a using scope alongside the cloned page. In Resolve, remove the unused document parameter and update all callers, or use document’s object table to resolve the reference; keep resolution behavior unchanged.
182-190: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
DrawAndReopenreturns a document whose backing stream is already disposed.
using var streamdisposes theMemoryStreamwhen the method returns, but thePdfDocumentfromPdfReader.Openkeeps a lexer over it. This only works while every object needed later has been parsed eagerly; any lazily-resolved object touched byGetAllObjects()/Stream.UnfilteredValuewould hit a disposed stream. Read the bytes out first so the document never depends on a live stream.♻️ Decouple the document from the stream's lifetime
private static PdfDocument DrawAndReopen(PdfFontEncoding encoding, string familyName = PinnedFontResolver.CffFamilyName) { - using var stream = new MemoryStream(); - Draw(encoding, familyName).Save(stream, false); - stream.Position = 0; - - return Pdf.IO.PdfReader.Open(stream, PdfDocumentOpenMode.Modify); + byte[] bytes; + using (var stream = new MemoryStream()) + { + Draw(encoding, familyName).Save(stream, false); + bytes = stream.ToArray(); + } + + // Not disposed on purpose: the document reads from it for as long as it lives. + return PdfReader.Open(new MemoryStream(bytes), PdfDocumentOpenMode.Modify); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PdfSharpCore.Test/Fonts/PostscriptOutlineEmbeddingTest.cs` around lines 182 - 190, Update DrawAndReopen so it materializes the saved document bytes before opening it, then pass the byte data through a reader or stream whose lifetime is independent of the disposed MemoryStream. Ensure the returned PdfDocument does not retain a lexer backed by the local using stream, while preserving the existing encoding and familyName behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PdfSharpCore.Skia/OpenTypeFontMetadata.cs`:
- Around line 71-91: Replace the duplicated TTC face-selection logic in
OpenTypeFontMetadata.Read with TrueTypeCollection’s existing collection
detection and face-offset/bounds validation helpers. Preserve the non-collection
faceIndex behavior, and ensure malformed or out-of-range face directories
produce the established controlled InvalidOperationException before any U16 or
table-record access.
In `@PdfSharpCore/Fonts/FontResolverInfo.cs`:
- Around line 91-96: Update the MustSimulateBold property summary in
FontResolverInfo to remove the stale statement that bold simulation is not
implemented, and document it consistently with the constructor parameter as
simulated bold rendering via stroking and filling glyphs.
In `@PdfSharpCore/Pdf.Advanced/PdfCIDFont.cs`:
- Around line 100-101: Update the comment immediately preceding
EmbedFontProgram(true) in PdfCIDFont to accurately state that CFF CID fonts
embed the full font program, while only TrueType fonts are automatically
subsetted; leave the embedding call unchanged.
In `@PdfSharpCore/Pdf.Advanced/PdfFont.cs`:
- Around line 117-146: Update EmbedFontProgram and its PdfTrueTypeFont
integration to distinguish non-CID CFF/OpenType fonts from TrueType-outline
fonts. Ensure CFF data without CID operators is emitted through a compatible
Type1 representation, while CID CFF data uses CIDFontType0; do not leave a
simple /TrueType dictionary paired with /FontFile3 /OpenType CFF data. Preserve
the existing TrueType and CID handling for fonts containing the corresponding
outlines.
In `@PdfSharpCore/Utils/FontFileTypes.cs`:
- Around line 43-50: Update the FontFileTypes.In method’s recursive file
enumeration to use EnumerationOptions with RecurseSubdirectories enabled and
IgnoreInaccessible enabled, preserving the existing wildcard and font-file
filtering behavior so inaccessible subdirectories are skipped.
In `@PdfSharpCore/Utils/FontResolverBase.cs`:
- Line 166: Update the font-directory enumeration around FontFileTypes.In to
guard each platform font directory with a directory-exists check before
enumerating it, matching the existing appdata-directory guard pattern. Ensure
missing macOS or Windows font directories return an empty result instead of
throwing, while preserving enumeration for existing directories.
---
Nitpick comments:
In `@PdfSharpCore.Test/Fonts/PostscriptOutlineEmbeddingTest.cs`:
- Around line 127-135: Dispose the native rasterized image collection by
declaring rendered from PdfHelper.Rasterize within a using scope alongside the
cloned page. In Resolve, remove the unused document parameter and update all
callers, or use document’s object table to resolve the reference; keep
resolution behavior unchanged.
- Around line 182-190: Update DrawAndReopen so it materializes the saved
document bytes before opening it, then pass the byte data through a reader or
stream whose lifetime is independent of the disposed MemoryStream. Ensure the
returned PdfDocument does not retain a lexer backed by the local using stream,
while preserving the existing encoding and familyName behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 131d2ec7-0506-4fd4-97b3-df44e887ec84
⛔ Files ignored due to path filters (1)
PdfSharpCore.Test/Assets/Fonts/SourceCodePro-Regular.otfis excluded by!**/*.otf
📒 Files selected for processing (28)
PdfSharpCore.ImageSharp/ImageSharpFontResolver.csPdfSharpCore.Skia/OpenTypeFontMetadata.csPdfSharpCore.Skia/SkiaFontResolver.csPdfSharpCore.Test/Assets/Fonts/LICENSE.txtPdfSharpCore.Test/Fonts/PostscriptOutlineEmbeddingTest.csPdfSharpCore.Test/Fonts/StyleSimulationTest.csPdfSharpCore.Test/Fonts/TrueTypeCollectionTest.csPdfSharpCore.Test/Helpers/PinnedFontResolver.csPdfSharpCore.Test/Helpers/TrueTypeCollectionBuilder.csPdfSharpCore.Test/PdfSharpCore.Test.csprojPdfSharpCore/Drawing/XFontSource.csPdfSharpCore/Drawing/XGlyphTypeface.csPdfSharpCore/Fonts.OpenType/OpenTypeFontface.csPdfSharpCore/Fonts.OpenType/enums/TableTagNames.csPdfSharpCore/Fonts/FontFactory.csPdfSharpCore/Fonts/FontResolverInfo.csPdfSharpCore/Fonts/PlatformFontResolver.csPdfSharpCore/Fonts/PlatformFontResolverInfo.csPdfSharpCore/Internal/FontFamilyModel.csPdfSharpCore/Pdf.Advanced/PdfCIDFont.csPdfSharpCore/Pdf.Advanced/PdfFont.csPdfSharpCore/Pdf.Advanced/PdfTrueTypeFont.csPdfSharpCore/Utils/FontFileTypes.csPdfSharpCore/Utils/FontResolverBase.csPdfSharpCore/Utils/LinuxSystemFontResolver.csPdfSharpCore/Utils/TrueTypeCollection.csREADME.mddocs/specs/font-embedding-gaps.md
💤 Files with no reviewable changes (2)
- PdfSharpCore/Fonts/PlatformFontResolver.cs
- PdfSharpCore/Fonts/PlatformFontResolverInfo.cs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🧹 Nitpick comments (2)
PdfSharpCore.Test/Fonts/PostscriptOutlineEmbeddingTest.cs (2)
127-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo small cleanups.
Line 127:
renderedholds a nativeMagickImageCollectionand is never disposed — worth ausingalongside the cloned page. Line 208:Resolvenever uses itsdocumentparameter; drop it or use it to resolve the reference through the document's object table.Also applies to: 208-211
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PdfSharpCore.Test/Fonts/PostscriptOutlineEmbeddingTest.cs` around lines 127 - 135, Dispose the native rasterized image collection by declaring rendered from PdfHelper.Rasterize within a using scope alongside the cloned page. In Resolve, remove the unused document parameter and update all callers, or use document’s object table to resolve the reference; keep resolution behavior unchanged.
182-190: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
DrawAndReopenreturns a document whose backing stream is already disposed.
using var streamdisposes theMemoryStreamwhen the method returns, but thePdfDocumentfromPdfReader.Openkeeps a lexer over it. This only works while every object needed later has been parsed eagerly; any lazily-resolved object touched byGetAllObjects()/Stream.UnfilteredValuewould hit a disposed stream. Read the bytes out first so the document never depends on a live stream.♻️ Decouple the document from the stream's lifetime
private static PdfDocument DrawAndReopen(PdfFontEncoding encoding, string familyName = PinnedFontResolver.CffFamilyName) { - using var stream = new MemoryStream(); - Draw(encoding, familyName).Save(stream, false); - stream.Position = 0; - - return Pdf.IO.PdfReader.Open(stream, PdfDocumentOpenMode.Modify); + byte[] bytes; + using (var stream = new MemoryStream()) + { + Draw(encoding, familyName).Save(stream, false); + bytes = stream.ToArray(); + } + + // Not disposed on purpose: the document reads from it for as long as it lives. + return PdfReader.Open(new MemoryStream(bytes), PdfDocumentOpenMode.Modify); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PdfSharpCore.Test/Fonts/PostscriptOutlineEmbeddingTest.cs` around lines 182 - 190, Update DrawAndReopen so it materializes the saved document bytes before opening it, then pass the byte data through a reader or stream whose lifetime is independent of the disposed MemoryStream. Ensure the returned PdfDocument does not retain a lexer backed by the local using stream, while preserving the existing encoding and familyName behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PdfSharpCore.Skia/OpenTypeFontMetadata.cs`:
- Around line 71-91: Replace the duplicated TTC face-selection logic in
OpenTypeFontMetadata.Read with TrueTypeCollection’s existing collection
detection and face-offset/bounds validation helpers. Preserve the non-collection
faceIndex behavior, and ensure malformed or out-of-range face directories
produce the established controlled InvalidOperationException before any U16 or
table-record access.
In `@PdfSharpCore/Fonts/FontResolverInfo.cs`:
- Around line 91-96: Update the MustSimulateBold property summary in
FontResolverInfo to remove the stale statement that bold simulation is not
implemented, and document it consistently with the constructor parameter as
simulated bold rendering via stroking and filling glyphs.
In `@PdfSharpCore/Pdf.Advanced/PdfCIDFont.cs`:
- Around line 100-101: Update the comment immediately preceding
EmbedFontProgram(true) in PdfCIDFont to accurately state that CFF CID fonts
embed the full font program, while only TrueType fonts are automatically
subsetted; leave the embedding call unchanged.
In `@PdfSharpCore/Pdf.Advanced/PdfFont.cs`:
- Around line 117-146: Update EmbedFontProgram and its PdfTrueTypeFont
integration to distinguish non-CID CFF/OpenType fonts from TrueType-outline
fonts. Ensure CFF data without CID operators is emitted through a compatible
Type1 representation, while CID CFF data uses CIDFontType0; do not leave a
simple /TrueType dictionary paired with /FontFile3 /OpenType CFF data. Preserve
the existing TrueType and CID handling for fonts containing the corresponding
outlines.
In `@PdfSharpCore/Utils/FontFileTypes.cs`:
- Around line 43-50: Update the FontFileTypes.In method’s recursive file
enumeration to use EnumerationOptions with RecurseSubdirectories enabled and
IgnoreInaccessible enabled, preserving the existing wildcard and font-file
filtering behavior so inaccessible subdirectories are skipped.
In `@PdfSharpCore/Utils/FontResolverBase.cs`:
- Line 166: Update the font-directory enumeration around FontFileTypes.In to
guard each platform font directory with a directory-exists check before
enumerating it, matching the existing appdata-directory guard pattern. Ensure
missing macOS or Windows font directories return an empty result instead of
throwing, while preserving enumeration for existing directories.
---
Nitpick comments:
In `@PdfSharpCore.Test/Fonts/PostscriptOutlineEmbeddingTest.cs`:
- Around line 127-135: Dispose the native rasterized image collection by
declaring rendered from PdfHelper.Rasterize within a using scope alongside the
cloned page. In Resolve, remove the unused document parameter and update all
callers, or use document’s object table to resolve the reference; keep
resolution behavior unchanged.
- Around line 182-190: Update DrawAndReopen so it materializes the saved
document bytes before opening it, then pass the byte data through a reader or
stream whose lifetime is independent of the disposed MemoryStream. Ensure the
returned PdfDocument does not retain a lexer backed by the local using stream,
while preserving the existing encoding and familyName behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 131d2ec7-0506-4fd4-97b3-df44e887ec84
⛔ Files ignored due to path filters (1)
PdfSharpCore.Test/Assets/Fonts/SourceCodePro-Regular.otfis excluded by!**/*.otf
📒 Files selected for processing (28)
PdfSharpCore.ImageSharp/ImageSharpFontResolver.csPdfSharpCore.Skia/OpenTypeFontMetadata.csPdfSharpCore.Skia/SkiaFontResolver.csPdfSharpCore.Test/Assets/Fonts/LICENSE.txtPdfSharpCore.Test/Fonts/PostscriptOutlineEmbeddingTest.csPdfSharpCore.Test/Fonts/StyleSimulationTest.csPdfSharpCore.Test/Fonts/TrueTypeCollectionTest.csPdfSharpCore.Test/Helpers/PinnedFontResolver.csPdfSharpCore.Test/Helpers/TrueTypeCollectionBuilder.csPdfSharpCore.Test/PdfSharpCore.Test.csprojPdfSharpCore/Drawing/XFontSource.csPdfSharpCore/Drawing/XGlyphTypeface.csPdfSharpCore/Fonts.OpenType/OpenTypeFontface.csPdfSharpCore/Fonts.OpenType/enums/TableTagNames.csPdfSharpCore/Fonts/FontFactory.csPdfSharpCore/Fonts/FontResolverInfo.csPdfSharpCore/Fonts/PlatformFontResolver.csPdfSharpCore/Fonts/PlatformFontResolverInfo.csPdfSharpCore/Internal/FontFamilyModel.csPdfSharpCore/Pdf.Advanced/PdfCIDFont.csPdfSharpCore/Pdf.Advanced/PdfFont.csPdfSharpCore/Pdf.Advanced/PdfTrueTypeFont.csPdfSharpCore/Utils/FontFileTypes.csPdfSharpCore/Utils/FontResolverBase.csPdfSharpCore/Utils/LinuxSystemFontResolver.csPdfSharpCore/Utils/TrueTypeCollection.csREADME.mddocs/specs/font-embedding-gaps.md
💤 Files with no reviewable changes (2)
- PdfSharpCore/Fonts/PlatformFontResolver.cs
- PdfSharpCore/Fonts/PlatformFontResolverInfo.cs
🛑 Comments failed to post (1)
PdfSharpCore/Pdf.Advanced/PdfCIDFont.cs (1)
100-101: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the embedding comment.
CFF CID fonts now embed the full program through
EmbedFontProgram(true); only TrueType fonts are subsetted. The current wording conflicts with the CFF behavior documented and implemented by this PR.Proposed fix
-// CID fonts must be always embedded. PDFsharp embeds automatically a subset. +// CID fonts must always be embedded. TrueType fonts are subsetted; CFF fonts embed whole.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// CID fonts must always be embedded. TrueType fonts are subsetted; CFF fonts embed whole. EmbedFontProgram(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PdfSharpCore/Pdf.Advanced/PdfCIDFont.cs` around lines 100 - 101, Update the comment immediately preceding EmbedFontProgram(true) in PdfCIDFont to accurately state that CFF CID fonts embed the full font program, while only TrueType fonts are automatically subsetted; leave the embedding call unchanged.
GetFont took the face name it had handed out, reduced it to a file name, and returned the first discovered path that contained that string. Any path containing the name won, so a file present in both the system and the per-user font directory was decided by array order; the scan copied the whole file array first and ran on every call; and a face that is one member of a collection cannot be named this way at all. The face name is now assigned once, while the files are being read anyway, and kept in a dictionary beside the path it came from. A family records the face names it found rather than paths, so the name a caller is given and the file it is read back from cannot drift apart. A duplicate name is dropped rather than overwriting, and only after its metadata has been read, so that an unparseable file cannot reserve a name a readable one would have used. A face with no file behind it now raises FileNotFoundException naming the face rather than a bare Exception naming both it and an empty string. The dictionaries are also published properly: EnsureInitialized reads them outside the lock that fills them, which on a weaker memory model than x86 could hand out a reference to a dictionary whose contents were not yet visible.
OpenTypeFontface.Read threw on the 'ttcf' signature, so a .ttc or .otc could be discovered but never used -- which on Windows and macOS rules out most of the CJK system faces, since those ship as collections. The index of a face cannot travel the normal route: IFontResolver.GetFont takes a name and nothing else, and it is implemented by every custom resolver in the wild, so widening it is not on offer. FontResolverInfo already carries a collectionNumber, but it would still have to reach GetFont somehow, which leaves the name as the only channel either way. So the name is the channel. A face of a collection is called "msgothic.ttc#1", and the resolver answers with a standalone sfnt built from that member alone -- a fresh offset table, a rewritten directory, and a copy of each table the face points at, including the ones it shared with its siblings. Nothing below the resolver ever meets a collection, so nothing below it had to change. collectionNumber stays zero and now says why rather than reading as unfinished. Reading the faces past the first is a backend's job, as reading metadata always was: ReadFontMetadata gains an overload taking a face index, which Skia answers from the collection header and ImageSharp from LoadFontCollectionDescriptions. A backend that does not override it keeps working and sees first faces only. The tests build a collection out of the Liberation faces already shipped, so they hold on a machine with no collection installed, and check the extracted faces with SixLabors.Fonts -- an independent reader accepting the output is the claim worth making -- plus every table byte for byte, because reading a name back only proves that one table landed where the directory said it would.
A font with CFF outlines has no glyf and no loca table. PdfTrueTypeFont did not check, called CreateFontSubSet, and dereferenced the missing loca -- a NullReferenceException out of nothing worse than asking for an .otf. PdfCIDFont did check, and on finding no loca embedded the whole OTTO file under /FontFile2 with a descendant subtype of /CIDFontType2. Both of those describe a TrueType program, so the output said the stream held one thing and the stream held another. Lenient viewers sniff the signature and cope; strict ones are entitled not to. FontTechnology has recorded which kind of outlines a face has since the parser was written, and nothing had ever read it. It is now the thing that decides: TrueType is subsetted into /FontFile2 as before, CFF goes in whole as /FontFile3 with /Subtype /OpenType, and the descendant becomes /CIDFontType0. Embedding /Subtype /OpenType requires PDF 1.6, so the document version is raised to 16 when one is written. Whole, not subsetted: subsetting CFF means parsing the INDEX and DICT structures, the Type 2 charstrings and both subroutine sets, tracing the calls per glyph and renumbering the lot. That is a project of its own and the part most likely to go quietly wrong, so it is not attempted here. The cost is that an .otf embeds at full size, which for a CJK face is megabytes -- worth saying plainly rather than discovering. Both font classes did their own embedding, near-identically, so the shared part moved to PdfFont.EmbedFontProgram and the branch lives in one place. CreateFontSubSet now refuses a CFF font by name instead of failing on a null table, and TableTagNames.Cff is "CFF " -- a table tag is four bytes, and the three-character version could never have matched anything. The tests check the embedded stream against the font file byte for byte, since the dictionary keys only say what was intended, and separately that Ghostscript -- which rejects malformed font dictionaries rather than guessing -- renders a page carrying one. Source Code Pro Regular is added under Assets/Fonts for this: SIL OFL 1.1, the same licence as the Liberation faces already there, whose text now covers both. Nothing else shipped with the tests has PostScript outlines.
…wing it PdfSharpCore has been able to draw a weight or a slant a font file does not have for as long as this fork has existed. XGraphicsPdfRenderer strokes the glyphs as well as filling them for a simulated bold and skews the text matrix for a simulated italic, and MeasureString widens its answer to match. Nobody ever asked it to. Two things stood in the way, and both are fixed here. The resolver never asked. Its fallback walked BoldItalic, Bold, Italic, Regular and returned whichever it found through the constructor that hardcodes both simulation flags to false, so a family shipping only a regular face rendered bold text as regular text and said nothing. It now picks the nearest face and asks for the axis it could not supply -- and only that one, so a family with a real bold asked for bold italic gets the bold file with a slant drawn on rather than both drawn over the regular. And the answer was thrown away. XGlyphTypeface.GetOrCreateFrom built the typeface with a constructor that takes no simulations, so even a resolver that did ask -- a custom one, since the shipped one never did -- had its answer dropped before the renderer could read it. The flags now travel. Filing a family's faces changed with it. A family shipping a single file was recorded as Regular whatever that file actually was, which made resolution succeed because Regular was looked for last. Now that a missing weight is drawn on rather than ignored, that would stroke a bold-only family's face bolder still, so faces are filed under the style they report. The candidate list reaches all four styles, so resolution still always finds something. Existing golden images are unmoved: the resolver the tests pin ships all four Liberation faces, so nothing in them simulates anything.
PlatformFontResolver.ResolveTypeface opened with a local set to null, returned when it found it null, and then continued into code that could not run -- reading a variable never assigned, and stepping through an if and an else that were both empty. The residue of the platform-specific code this fork removed. Its only caller was the branch FontFactory took when no resolver had been set, and GlobalFontSettings.FontResolver now throws rather than returning null when that is so, which leaves the call site unreachable as well. Both types go, along with the branch and the "is PlatformFontResolverInfo" guard that kept a resolver's result from being cached. PlatformFontResolver was public, so this breaks source for anyone calling it -- but calling it returns null and always has, so nothing that works today stops working. XGlyphTypeface kept the CreateSolitary call the guard used to skip. Its result was already being discarded; the call is made for the entry it leaves in FontFamilyCache, and now says so.
Discovery globbed "*.ttf" on all three platforms, and the Linux resolver filtered .ttf out of the fontconfig result as well -- so even where fontconfig had enumerated the machine correctly, most of a modern font set was discarded on the way out. All four platforms now take .ttf, .otf, .ttc and .otc. Last of the six on purpose: widening this before collections could be read and CFF outlines embedded would have surfaced fonts that resolve and then fail at save time, which is worse than not finding them. Type 1 and the bitmap formats stay out. The parser reads an sfnt and nothing else, so surfacing one would move the failure from discovery to use rather than remove it. Measured on this machine's font directories, 519 .ttf files against 540 files holding 557 faces: 170 ms to discover before, 238 ms after. The added time is not the 21 extra files but their size -- the collections here are the CJK faces, which are megabytes each. Reading them costs what it costs, but reading them repeatedly does not: asking a backend for one face at a time opened a fourteen-face collection fourteen times, which was most of the increase. ReadCollectionMetadata reads a whole collection in one pass, and both backends override it -- 309 ms before that, 238 ms after. A backend that cannot do better inherits the per-face loop, and a collection that throws under the batch falls back to it, so one unreadable face still costs only itself.
The spec now carries the three places the build departed from it: the style simulation flags had nowhere to go until XGlyphTypeface took them, filing a one-file family under Regular had to stop, and widening discovery was not free until a collection could be read in one pass. The README gains the two limits a caller has to know before choosing a font -- that a CFF .otf embeds whole, and that a weight or slant the family has no file for is drawn on rather than typeset.
Review follow-up on the font embedding work. Reading a collection's metadata took the declared face count on trust and never checked that the face's table directory was inside the file, so a malformed .ttc walked off the end of the array. TrueTypeCollection already validates a declared count against the room the file has for it, so the detection and that check are delegated to it, and the directory offset and table count are bounds-checked before anything is read through them. Walking a font directory failed the first font operation of the process if any subdirectory under it was unreadable, and the macOS and Windows font directories were enumerated without the exists check their appdata sibling already had. Two comments described code that had changed underneath them: bold simulation is implemented, by stroking the glyphs as well as filling them, and a CID font with CFF outlines is embedded whole rather than subsetted. The simple-font path pairs a /TrueType dictionary with a /FontFile3 of subtype /OpenType, which reads oddly enough to be worth pinning rather than arguing about: Ghostscript, the strict reader already on hand, renders it. Only the CID path was in front of it before.
jafin
force-pushed
the
feat/font-embedding-gaps
branch
from
July 28, 2026 11:20
da9aa7b to
344d543
Compare
Owner
Author
|
Rebased onto current master ( The diff now reads as font work only. Before the rebase it appeared to delete the number-tree, page-label and text-markup-annotation code, because the branch was cut before those merged; they are now beneath it rather than beside it. Suite: 361 passed / 1 skipped on net8.0 and net10.0. Golden images unmoved. Commit SHAs in the description have been updated to their post-rebase values. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Embedding itself was never the gap. Both font paths have always written a subset unconditionally
through managed OpenType table surgery, identically on every platform — the upstream claim that it
cannot work off Windows is about
GdiGetFontData/CreateFontPackage, Win32 APIs this codebase hasnever called. What was missing is everything around it: which files can be found, which of those can
then be embedded at all, and what happens when the requested style has no file.
Six gaps, one commit each, built in dependency order.
docs/specs/font-embedding-gaps.mdcarries thedesign and the three places the build departed from it.
GetFontresolved a face by substring match over the whole file array96e0faf.ttc,.otc) threw on read8c78b3fc835c471a1d5b9PlatformFontResolverwas unreachable dead code7580374*.ttfwas discovered5f9a5d3G1 is last on purpose: widening discovery before collections could be read and CFF embedded would
have surfaced fonts that resolve and then fail at save time, which is worse than not finding them.
What each one does
G5 — the face name is assigned once, while files are being read anyway, and kept beside the path
it came from. A family records face names rather than paths, so the name a caller is handed and the
file it is read back from cannot drift apart. Also fixes an unsynchronised publish:
EnsureInitializedread the lookup dictionaries outside the lock that filled them.
G4 — the face index cannot travel the normal route, because
IFontResolver.GetFont(string)takesa name and nothing else and is implemented by every custom resolver in the wild. So the name is the
channel: a face is called
msgothic.ttc#1, and the resolver answers with a standalone sfnt builtfrom that member. Nothing below the resolver had to change. Reading faces past the first is a
backend's job — Skia from the collection header, ImageSharp from
LoadFontCollectionDescriptions.G2 —
FontTechnologyhad recorded which outlines a face has since the parser was written andnothing had ever read it. It now decides: TrueType subsets to
/FontFile2, CFF goes in whole as/FontFile3/Subtype /OpenTypeunder a/CIDFontType0descendant, and the version is raised to1.6. Previously the simple-font path raised
NullReferenceExceptionon the missingloca, and theCID path wrote an
OTTOfile into/FontFile2under/CIDFontType2— both describe a TrueTypeprogram, so the file said one thing and held another.
No CFF subsetting. That means parsing the INDEX and DICT structures, the Type 2 charstrings and
both subroutine sets, tracing calls per glyph and renumbering the lot — a project of its own and the
part most likely to go quietly wrong. An
.otftherefore embeds at full size, which for a CJK faceis megabytes. Documented in the README rather than left to be discovered.
G3 — PdfSharpCore has always been able to stroke a simulated bold and skew a simulated italic,
and
MeasureStringcompensates. Nobody asked it to. Two things were in the way and both are fixed:the resolver returned through the constructor that hardcodes both flags false, and
XGlyphTypeface.GetOrCreateFrombuilt the typeface through a constructor that takes no simulations— so even a custom resolver that did ask had its answer dropped. Only the missing axis is simulated:
a family with a real bold asked for bold italic gets the bold file with a slant drawn on.
G6 —
ResolveTypefaceopened with a local set to null, returned on finding it null, and thencontinued into code that could not run. Its only caller was the branch taken when no resolver was
set, and that property now throws instead. Both types are public, so this is source-breaking — but
calling it returns null and always has.
Measurements
Discovery, on this machine's Windows font directories — 519
.ttfagainst 540 files holding 557faces:
.ttfonlyThe 84% jump for 4% more files was not the files but the reading: asking a backend for one face at a
time opens the file once per face, so a fourteen-face collection was opened fourteen times.
ReadCollectionMetadatareads a collection in one pass; the default is still the per-face loop, anda collection that throws under the batch falls back to it, so one unreadable face costs only itself.
The remainder is real work — Windows collections are the multi-megabyte CJK faces.
Two judgement calls worth review
The
.otftest asset.SourceCodePro-Regular.otf, SIL OFL 1.1 — the same licence as theLiberation faces, whose text now covers both. 131 KB, smaller than any of them. Nothing else shipped
with the tests has PostScript outlines.
The Ghostscript test claims less than it first did. Ghostscript substitutes silently for a font
it cannot use, so ink on the page does not prove which glyphs drew it. It asserts only that a reader
which rejects malformed font dictionaries accepts this one and renders — which is what the old
output failed at. Identity is settled separately, comparing the embedded stream to the font file byte
for byte. The collection tests do the same, per table, because reading a name back only proves the
nametable landed where the directory said it would.Tests
280 passing on
net8.0andnet10.0, zero build warnings. Existing golden images are unmoved: theresolver the tests pin ships all four Liberation faces, so nothing in the existing suite falls back
far enough to simulate anything — the new tests reach that behaviour through a family that
deliberately ships one.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Upstream issue
Closes ststeiger#492 — "Bold style is not applied for some fonts", which is G3 seen from the outside.
Cascadia Code and Bahnschrift are variable fonts: Windows ships one file per family, so the family map has no
Boldentry, the lookup fell through toRegular, and the result came back through theFontResolverInfoconstructor that fixes both simulation flags to false. Italic looked fine only because Windows ships a separateCascadiaCodeItalic.ttf; Bahnschrift ignored italic too.Verified against the reporter's own code and the real Windows fonts:
CascadiaCode.ttfCascadiaMono.ttfbahnschrift.ttfarialbd.ttfBold+italic on Cascadia correctly takes
CascadiaCodeItalic.ttfand simulates only the missing weight, rather than drawing both axes over the regular face.