Add pagx embed command, restructure pagx font, and support external font files. by YyZz-wy · Pull Request #3471 · Tencent/libpag

YyZz-wy · 2026-05-29T08:19:29Z

本 PR 整合 PAGX CLI 的三阶段重构：

新增 pagx embed 命令：统一字形与图片嵌入流程，支持 --skip-fonts / --skip-images / --fallback 选项；ImageEmbedder 与 FontEmbedder 拆分清晰。
重构 pagx font 命令：扁平化为单一查询命令，移除 embed 子命令（保留兼容重定向），新增 --list 列出系统字体。
PAGX Font 节点新增 file 属性：可引用外部字体文件，pagx embed 自动扫描并注册字体到 FontConfig，无需 --font-file 参数；ClearEmbeddedGlyphRuns 在 Font 含 file 时保留节点结构仅清除 Glyph 子节点，支持幂等 re-embed。

附带 SystemFonts 在 macOS / Windows / Linux / FreeType 后端新增 AllFontFamilies 与 FindFont 实现，修复若干内存泄漏与悬空指针问题，并更新 spec、skills 文档及 CLI 测试。

… partial-read detection.

…ip flags.

…phRun nodes from document. Previously only cleared Text::glyphRuns vector but left Font, Glyph, GlyphRun, PathData, and bitmap Image nodes in document->nodes, causing duplicate nodes on re-embed.

…nd idempotency. 9 CLI_TEST cases cover EMBED-01..05, EMBED-07, EMBED-08, EMBED-10, EMBED-11.

…phRun nodes from document. Previously only cleared Text::glyphRuns vector but left Font, Glyph, GlyphRun, PathData, and bitmap Image nodes in document->nodes, causing duplicate nodes on re-embed.

…Runs to follow coding convention.

…elpers.

…tions policy.

…including node removal.

…tEmbedUsage.

…onts.h.

… generic fallback.

…command layer.

…to pagx font.

…ist.

…_embed

… ExporterUtils.

shlzxjp · 2026-06-03T12:55:35Z

+        }
+        if (node->nodeType() == NodeType::Font) {
+          auto* font = static_cast<Font*>(node.get());
+          ResolveRelativePath(basePath, font->file);


Font.file 在导入时被改写为绝对路径，但 PAGXExporter 在写出 Font 节点时直接 xml.addAttribute("file", font->file) 没有反向相对化（参见 src/pagx/PAGXExporter.cpp:1007-1009）。结果：用户写的 <Font file="./fonts/X.otf"/> 经 pagx embed 后会变成 file="/abs/path/.../X.otf"，把整个 PAGX 复制到别的机器或目录后外部字体引用就失效了。文档（attributes.md）声称嵌入后保留 file 是为了"源文件可追溯性"，但绝对路径反而破坏了这一语义。新增测试 Embed_FontFile_ReembedPreservesNode、Embed_FontFile_AutoRegistersAndEmbeds 都没断言路径形式，所以问题没暴露。建议导出时对原本是相对形式的路径做反向相对化，或在 importer 里保留原始字符串、仅在解析阶段使用临时副本。

shlzxjp · 2026-06-03T12:56:04Z

+   * invalidated after this call. Callers must first collect all affected nodes to remove
+   * before calling.
+   */
+  void removeNodes(const std::unordered_set<Node*>& nodesToRemove);


removeNodes / setNodeId / resetLayoutState 三个新方法本质上是为 FontEmbedder::ClearEmbeddedGlyphRuns 服务的内部协作接口，却暴露在公开头文件 include/pagx/PAGXDocument.h 中。第三方调用者一旦使用 removeNodes，就要承担注释里写的"any external pointers… become dangling"风险，没有任何编译期或运行期保护。建议把这三个方法改成 private + friend class FontEmbedder，或者迁移到内部头文件，避免污染公开 API surface。

shlzxjp · 2026-06-03T12:56:31Z

      continue;
    }
-    if (image->filePath.find("data:") == 0) {
+    if (IsUrlPath(image->filePath)) {


新的 IsUrlPath 仅识别 http://、https://、file://，不再排除 data: URI。代码注释说"data: is handled by PAGXImporter before this point"，对 FromFile 路径成立；但 getExternalFilePaths 是公开 API，第三方完全可以绕过 importer 直接构造 PAGXDocument 并设置 image->filePath = "data:image/png;base64,..."。在新逻辑下：1) getExternalFilePaths 会返回 data:... 字符串；2) ImageEmbedder::embed 调用 ifstream 打开失败；3) pagx embed 报 failed to load image 'data:image/png...'，对调用方非常迷惑。建议在 IsUrlPath 中也匹配 data: 前缀，与旧行为对齐（或改名为 IsNonFileScheme）。

shlzxjp · 2026-06-03T12:56:52Z

+    std::cout << "pagx embed: wrote " << options.outputFile << "\n";
+    return 0;
+  }
+  if (!WriteStringToFile(xml, options.outputFile, "pagx embed")) {


覆盖输入文件路径走的是 tempPath + rename 的原子写入（上面 152-175 行），而显式 -o <other> 这条路径直接走 WriteStringToFile，是直接 ofstream 写入。如果 -o 指向的已存在文件，写到一半失败/崩溃就会得到一个截断的输出文件。两条路径的崩溃安全语义不一致。建议统一用 temp+rename helper，或在 WriteStringToFile 内部统一加上原子写入。

shlzxjp · 2026-06-03T12:57:26Z

+    if ((arg == "-o" || arg == "--output") && i + 1 < argc) {
+      options->outputFile = argv[++i];
+    } else if (arg == "--fallback" && i + 1 < argc) {
+      options->fallbacks.push_back(argv[++i]);


用户传 --fallback 但同时传 --skip-fonts 时，fallbacks 收到了值却完全不会被使用（下面 if (!options.skipFonts) 块被跳过），命令也不报错。脚本中常会无脑加 --fallback，发现某次没生效非常难排查。建议在 skipFonts && !fallbacks.empty() 时报错或打印警告。

shlzxjp · 2026-06-03T12:58:12Z

  int fontIndex = 1;
  if (vectorBuilder.font != nullptr) {
-    vectorBuilder.font->id = "font" + std::to_string(fontIndex++);
+    document->setNodeId(vectorBuilder.font, "__embed_font_" + std::to_string(fontIndex++));


setNodeId 内部调用 registerNode，撞 ID 时只在 document->errors 里追加一条记录，然后直接覆盖 nodeMap[id]（参见 PAGXDocument.cpp:66-76）。如果用户的 PAGX 里恰好存在 id 为 __embed_font_1 的节点（不限于 Font，任何节点类型），它会被静默从 nodeMap 中踢掉，且 pagx embed 仍返回 0。__embed_font_ 前缀降低了概率，但前缀本身没有任何文档化的"保留"声明。建议要么在 spec 中明确声明 __embed_* 为保留前缀，要么在 setNodeId 撞 ID 时让命令行调用方拿到非零退出码。

shlzxjp · 2026-06-03T12:59:59Z

+  if (!style.empty()) {
+    FcPatternAddString(pattern, FC_STYLE, reinterpret_cast<const FcChar8*>(style.c_str()));
+  }
+  FcPatternAddBool(pattern, FC_SCALABLE, FcTrue);


Linux FindFont 强制 FC_SCALABLE=true，与 FallbackTypefaces 行为一致；但当用户的 PAGX 显式声明使用一个仅有位图（embedded bitmap-only）的字体（如某些 emoji 字体）时，FindFont 会找不到。FallbackTypefaces 排除位图是合理的，但用户显式查询时是否要放宽这个限制需要确认产品定位。

…edder friend.

…e FC_SCALABLE filter.

…_embed

…text_box tests.

shlzxjp · 2026-06-10T01:57:47Z

+        if (node->nodeType() == NodeType::Font) {
+          auto* font = static_cast<Font*>(node.get());
+          if (!font->file.empty()) {
+            font->fileOriginal = font->file;


fileOriginal 只在 PAGXImporter::FromFile 这一条加载路径上被设置，而 PAGXImporter::FromXML(content) 这条路径不经过这里——只在 ParseFont 中执行 font->file = GetAttribute(node, "file")，所以从 XML 字符串/字节流加载的文档，Font::fileOriginal 永远为空。

现状下没有功能 bug（FromXML 路径下 font->file 没被改写，导出端 !fileOriginal.empty() ? fileOriginal : font->file 退化到 font->file 仍是原值），但这把 fileOriginal 的语义碎片化到了具体加载路径上，而不是数据结构的不变量。

隐患：未来若新增第三种加载路径（如解压 zip 内嵌 PAGX 后再解析），开发者很容易忘记再补一遍这段备份逻辑。建议把"备份原值"的语义下沉到 ParseFont（解析时立即 font->fileOriginal = font->file），由 FromFile 单独负责解析后的相对→绝对重写。这样 fileOriginal == 原始 XML 中的 file 属性 在数据结构层面就是个稳定的不变量。

shlzxjp · 2026-06-10T01:58:11Z

+  {
+    std::ofstream out(tempPath);
+    if (!out.is_open()) {
+      std::cerr << command << ": failed to write '" << tempPath << "'\n";


错误消息暴露了内部 .tmp 路径细节——tempPath 是 filePath + ".tmp" 这一实现选择，对用户而言完全是黑盒。

用户运行 pagx embed -o /readonly/out.pagx input.pagx 失败时会看到：

pagx embed: failed to write '/readonly/out.pagx.tmp'

用户指定的是 out.pagx，但报错里冒出来一个 out.pagx.tmp，会让人困惑（怀疑是不是自己拼写错了，或文件被别的进程占着）。第 73、79、87、88 行的错误消息都有同样问题。

参考主流 CLI（git、cargo、npm），失败消息都报用户指定的最终路径，而不是内部临时文件名。建议把错误消息中的 tempPath 替换为 filePath，例如：failed to write '<filePath>' (creating temp file failed)。

…_embed

shlzxjp · 2026-06-23T09:12:20Z

+
+bool ImageEmbedder::embed(PAGXDocument* document) {
+  if (document == nullptr) return false;
+  auto paths = document->getExternalFilePaths();


ImageEmbedder 与新合入的外部 composition 特性存在契约冲突，会导致误报失败 + 静默吞数据。

本类写于 main 合入外部 composition 特性（#3475）之前，依赖的 getExternalFilePaths() 契约在合并后变了：现在它不仅返回 Image 文件路径，还会返回未解析的外部 composition .pagx 路径（见 PAGXDocument.cpp 的 AppendExternalFilePaths：layer->composition == nullptr && IsExternalFilePath(layer->compositionFilePath)）。

而本函数对返回的每个路径无差别 ReadFileToData 再交给 loadFileDataMap，于是当 PAGX 含 <Layer composition="scene.pagx"> 时：

误报失败：compositionFilePath 在 PAGXImporter::FromFile 里并不会被解析成绝对路径（那里只处理 Image/Font），所以相对当前工作目录往往打不开 → embed() 返回 false → pagx embed 整体失败，错误信息还把它叫成 failed to load image 'scene.pagx'，把一个本应成功的 embed 弄挂。

静默吞数据：即便能读到，字节被放进 fileDataMap，但 loadFileDataMap 只匹配 NodeType::Image 节点，composition 路径永远匹配不上 → 数据被丢弃，白做一次（最大 256MB）I/O，且外部 composition 并未被嵌入。

修复建议（任选其一）：

首选：新增一个只返回 Image 资源路径的接口（如 PAGXDocument::getExternalImagePaths()），ImageEmbedder 改用它，使图片嵌入只触碰图片资源。

或者：在本函数内按节点类型过滤——遍历 Image 节点自行收集 filePath，不走会混入 composition 路径的 getExternalFilePaths()。

同时请明确 pagx embed 对外部 composition 的预期行为（嵌入 / 跳过 / 提示先 pagx resolve），并补一个含 <Layer composition="..."> 的回归测试覆盖该路径。

注：font 嵌入侧也有类似盲区——未解析的外部 composition 子文档里的文字不会被 CollectAllText 收集（需先 resolve）。建议在明确预期时一并覆盖。

shlzxjp · 2026-06-25T13:18:44Z

+    if (fileDataMap.count(path) > 0) {
+      continue;
+    }
+    auto data = ReadFileToData(path);


pagx embed 对 URL 形式（http/https/file://）的图片路径会误报失败。（与上面 line 53 的 composition 问题同源，建议合并修复）

本 PR 早期版本里 getExternalFilePaths() 用 IsUrlPath 排除了 http/https/file:// 这类 URL。但合并 main 时，commit 6e200f7a "Remove unused PAGX URL helper" 删除了整个 IsUrlPath，改用 main 既有的 IsExternalFilePath（PAGXDocument.cpp:44）——它只排除 data:，不再排除 http/https/file://。而本函数对每个返回路径无差别 ReadFileToData（本地 ifstream），删除 IsUrlPath 后没有补回任何 scheme 过滤。

后果：PAGX 含 <Image source="https://cdn.example/logo.png"/> 时，getExternalFilePaths() 返回该 URL → ReadFileToData 用 ifstream 打开必然失败 → embed() 返回 false → pagx embed 整体失败，报 failed to load image 'https://...'。一个引用了远程图片的合法 PAGX，哪怕用户只想嵌字体/本地图片，也会被整体弄挂。

为何 main 自身没暴露：main 运行时不靠 getExternalFilePaths() 做本地读取——URL 由 host 自行下载后用 loadFileData(path,data) 回填，所以返回 URL 是 main 的有意设计。问题在于纯本地 I/O 的 ImageEmbedder 直接消费了这个面向 host 的接口。

修复建议（与上面 composition 问题合并解决）：

推荐新增 PAGXDocument::getExternalImagePaths()，在其中同时排除：① 非 Image 节点（即上面的 composition .pagx 路径）；② 含 :// 的 URL-scheme 路径（http/https/file/data 等）。ImageEmbedder 改用它，使图片嵌入只触碰"本地图片文件"这一类资源。URL 形式的远程资源本就不应被 inline，应原样保留在 PAGX 中。

请补测试覆盖 http(s):// 与 file:// 形式的 image 源。

YyZz-wy requested review from Hparty, domchen, kevingpqi123 and shlzxjp as code owners May 29, 2026 08:19

YyZz-wy force-pushed the feature/codywwang_cli_embed branch from b8fbace to 74428c8 Compare May 29, 2026 08:31

YyZz-wy added 25 commits May 29, 2026 16:34

Ignore GSD planning directory from version control.

fc2706f

Add ImageEmbedder header declaring embed and lastErrorPath API.

f6723e6

Implement ImageEmbedder with ReadFileToData supporting empty-file and…

5b6a818

… partial-read detection.

Add CommandEmbed header declaring RunEmbed entry point.

4c99b91

Add pagx embed command orchestrating font and image embedding with sk…

849b4a6

…ip flags.

Register pagx embed command and update top-level usage.

1435101

Add embed_sample pagx and png fixtures for pagx embed CLI tests.

c864a08

Fix FontEmbedder::ClearEmbeddedGlyphRuns to remove stale Font and Gly…

0b403b1

…phRun nodes from document. Previously only cleared Text::glyphRuns vector but left Font, Glyph, GlyphRun, PathData, and bitmap Image nodes in document->nodes, causing duplicate nodes on re-embed.

Add EMBED CLI tests covering default embed skip flags missing image a…

b02b1a7

…nd idempotency. 9 CLI_TEST cases cover EMBED-01..05, EMBED-07, EMBED-08, EMBED-10, EMBED-11.

Fix FontEmbedder::ClearEmbeddedGlyphRuns to remove stale Font and Gly…

78de811

…phRun nodes from document. Previously only cleared Text::glyphRuns vector but left Font, Glyph, GlyphRun, PathData, and bitmap Image nodes in document->nodes, causing duplicate nodes on re-embed.

Replace lambda with explicit loop in FontEmbedder::ClearEmbeddedGlyph…

9da0451

…Runs to follow coding convention.

Retire pagx font embed subcommand with redirect error and drop dead h…

2961465

…elpers.

Revise CommandFont header doc comment to query-only scope.

7e7d61e

Add FONT-01 CLI test asserting pagx font embed redirect error.

103b09b

Add std::nothrow to ImageEmbedder buffer allocation to honor no-excep…

097a127

…tions policy.

Document full reset contract of FontEmbedder::ClearEmbeddedGlyphRuns …

f7436e6

…including node removal.

Trim CommandEmbed header doc to a summary and defer flag list to Prin…

56e0165

…tEmbedUsage.

Add FontFamilyEntry struct and AllFontFamilies declaration to SystemF…

c5d04d1

…onts.h.

Implement SystemFonts::AllFontFamilies for macOS, Windows, Linux, and…

ebcebb7

… generic fallback.

Flatten pagx font into a single query command and delete the info sub…

f0794c7

…command layer.

Rename FontInfo tests to Font, add info redirect and help surface tests.

dcfdf36

Add --list flag parsing, validation, and system font list formatting …

15b57ea

…to pagx font.

Add FontList text, JSON, and mutual exclusion tests for pagx font --l…

3b31a25

…ist.

Update CommandFont.h comment to reflect removed subcommands.

a6e759b

Eliminate redundant map lookup in SystemFonts Linux AllFontFamilies.

1bad5eb

YyZz-wy added 3 commits June 3, 2026 14:57

Merge remote-tracking branch 'origin/main' into feature/codywwang_cli…

3c04ca3

…_embed

Remove duplicated MakeStandaloneParams in Text and reuse the one from…

17e370f

… ExporterUtils.

Add missing Font file import round-trip test for Phase 3 plan 03.

6fa0fd7

shlzxjp reviewed Jun 3, 2026

View reviewed changes

YyZz-wy added 10 commits June 3, 2026 21:33

Make removeNodes, setNodeId and resetLayoutState private with FontEmb…

b90fe4b

…edder friend.

Error when --skip-fonts and --fallback are both set in embed command.

890fb04

Fix IsUrlPath to also match data: URI scheme.

1cd6eac

Preserve Font file original path across import/export round-trip.

54f6708

Unify atomic write in WriteStringToFile, fix font ID collision, remov…

9d07c41

…e FC_SCALABLE filter.

Merge remote-tracking branch 'origin/main' into feature/codywwang_cli…

edce376

…_embed

Merge remote-tracking branch 'origin/main' into feature/codywwang_cli…

d31df93

…_embed

Update screenshot baseline for layout_flex_weights test.

e738016

Update screenshot baseline for spec app_icons game_hud glyph_run and …

1d4e909

…text_box tests.

Merge branch 'main' into feature/codywwang_cli_embed

8e56b43

shlzxjp reviewed Jun 10, 2026

View reviewed changes

YyZz-wy added 6 commits June 17, 2026 15:23

Merge main into cli embed branch.

95026df

Preserve font paths and clarify CLI write errors.

e25d3f1

Format CLI write error message.

2138deb

Remove unused PAGX URL helper.

6e200f7

Merge remote-tracking branch 'origin/main' into feature/codywwang_cli…

a75c515

…_embed

Update screenshot baseline for spec pagx features test.

c6b7534

shlzxjp reviewed Jun 23, 2026

View reviewed changes

shlzxjp reviewed Jun 25, 2026

View reviewed changes

Uh oh!

Conversation

YyZz-wy commented May 29, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants