Repository navigation
Conversation
… from OpenGL and prepare for Metal / Vulkan / D3D12 / WebGPU backends.
…ligned with tgfx conventions.
…t> semaphore ABI so users can hand libpag externally created Metal resources with correct format and cross-queue timeline sync.
… so Apple apps can drive libpag from CAMetalLayer / MTKView / MTLDevice-backed CVPixelBuffer.
…x external-texture device inference on backends without a thread-local current context.
…t cases mirroring the GL BottomLeft blend paths on Metal.
…pu_backend # Conflicts: # DEPS
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3717 +/- ##
==========================================
- Coverage 83.51% 83.47% -0.05%
==========================================
Files 731 735 +4
Lines 105532 105791 +259
Branches 29218 29259 +41
==========================================
+ Hits 88138 88308 +170
- Misses 10803 10878 +75
- Partials 6591 6605 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
代码评审报告 — Metal 后端支持从架构设计、对外 API、Metal 实现正确性、健壮性、性能、构建测试 CI 六个维度做了深度评审。所有结论均已读实际代码验证；凡涉及 tgfx 行为的判断，一律以 DEPS 锁定版 总体评价方向正确、工程量扎实。 但当前状态建议先不合入，有三个层面的问题： 1. 对外 API 破坏。 好消息是修复模式在同一个文件里已有先例： 2. 质量门禁失效，CI 绿灯不构成渲染无回归的证据。
3. Metal 后端功能完整性不足。 跨设备安全校验被无条件绕过、 关于文档自述
两条高性价比建议① 删掉 ② 删掉 PAGView 里的 评审严谨性说明为避免误报，评审过程中排除了 53 项曾产生怀疑但经核实不成立的项，其中包括几处我们自己的错误结论。列举几条请不要据此修改的项：
另有 3 条依赖运行时行为、未能实机验证的，行级评论中已用「请确认」口径标注，不作断言。 合入建议必须修：上述 1、2 两类（公开头宏门禁、baseline key、截图门禁），以及跨设备校验、 强烈建议同批： 必须进 release notes： |
shlzxjp
left a comment
shlzxjp
left a comment
There was a problem hiding this comment.
以下是行级评审意见，共 23 条（Blocker 4 处 / Major 19 处）。Minor 与 Nit 另有 40 条，已合并在总评与后续汇总评论中。
所有结论均已读实际代码验证，涉及 tgfx 行为的一律以 DEPS 锁定版 5cf4fcce 源码核对。其中 3 条依赖运行时行为、未能实机验证的已标注「请确认」。
|
|
||
| PAG_API @interface PAGSurface : NSObject | ||
|
|
||
| #if defined(TGFX_USE_OPENGL) |
There was a problem hiding this comment.
[Blocker] 公开头用构建期宏做条件声明，下游 App 所有工厂方法会消失
TGFX_USE_OPENGL / TGFX_USE_METAL 只是 libpag 自身的编译宏，不随头文件分发：
CMakeLists.txt:537GLOBsrc/platform/ios/*.h→:882PUBLIC_HEADER，本文件原样进入libpag.framework/Headers/；:867的target_compile_definitions(pag PUBLIC ...)只对通过 CMake 链接 pag target 的消费者生效，预编译 framework / CocoaPods 的 Xcode App 工程拿不到这两个宏。
于是下游编译时两个 #if 双双为假，后果是双向的：
- 存量集成方的
[PAGSurface FromLayer:]、FromCVPixelBuffer:context:编译报 "no known class method"； - 本 PR 新增的
FromMetalLayer:/FromMTKView:/FromCVPixelBuffer:mtlDevice:对 framework 用户永久不可见，新功能等于未交付。
demo 能跑是因为 PAGView.mm 与 libpag 在同一 target 内，恰好掩盖了该问题。另：PAGView.h:23-28 里作者自己写明「公开 API 必须无条件声明」，两处标准不一致。
修复模式本文件已有先例：:55-59 的 +FromCVPixelBuffer: 完全没有被宏包住，实现是在 .mm 内按后端分派（PAGSurfaceImpl.mm:110-112 GL / :127-129 Metal）。照抄即可，无需新机制。
连带项：公开头改回无条件声明后，
PAGSurfaceImpl.h/.mm里对应的#if也要一起拆掉，否则PAGSurface.m会调到当前后端下不存在的 Impl 方法。可参照PAGSurfaceImpl.mm:88-101现有的模拟器分支写法（方法始终存在、内部判断后返回 nil）。
There was a problem hiding this comment.
已在 commit fed0c27 中修复：公开头文件（src/platform/ios/PAGSurface.h、src/platform/mac/PAGSurface.h 及 C 头）现在无条件声明全部工厂方法，不再引用 TGFX_USE_OPENGL / TGFX_USE_METAL 等构建期宏。不可用后端的工厂方法在实现侧返回 nil/NULL 并打 LOGE，头文件对 framework 用户保持完整，下游 App 不会再出现工厂方法消失的问题。
|
|
||
| PAG_API @interface PAGSurface : NSObject | ||
|
|
||
| #if defined(TGFX_USE_OPENGL) |
There was a problem hiding this comment.
[Blocker] 同 iOS 侧：+FromView: 被 TGFX_USE_OPENGL 包裹后对 framework 用户不可见
与 src/platform/ios/PAGSurface.h:33 是同一问题，此处 +FromView: 是已发布的既有 API，包裹后 macOS 存量集成方升级即编译失败。修复方式同上：公开头无条件声明，后端判断下沉到 .mm。
There was a problem hiding this comment.
同 iOS 侧，已在 commit fed0c27 中一并修复：+FromView: 等全部工厂方法无条件声明，后端不支持时由实现返回 nil 并打日志，公开 API 对 framework 用户保持完整。
| pagPlayer->setMatrix(Matrix::I()); | ||
| pagPlayer->setProgress(0.5); | ||
| pagPlayer->flush(); | ||
| EXPECT_TRUE(Baseline::Compare(pagSurface, "PAGBlendMetalTest/CopyDstTexture")); |
There was a problem hiding this comment.
[Blocker] 新增的 7 个 Metal 截图用例基线 key 未提交，PAGFullTest_Metal 按构造必然失败
test/baseline/version.json 本 PR 未修改，全文 Metal key 数为 0（grep -c Metal → 0）。而 test/src/utils/Baseline.cpp:305 的首条判定是：
if (baselineVersion.empty() || ...) { ...; return false; }key 不存在即 baselineVersion.empty() 为真 → 直接 return false → EXPECT_TRUE 失败。
受影响的 7 个断言：本文件 :248,281,320（PAGBlendMetalTest/*）、PAGSurfaceTest.cpp:304,333（PAGSurfaceMetalTest/*）、PAGXRuntimeTest.cpp:483,545（PAGXRuntimeTest/Metal*）。
推测是本地跑过 accept-baseline 但漏提交 test/baseline/version.json。这也与 PR 描述「Metal 测试覆盖」形成事实矛盾。因为目前没有任何 CI job 开 PAG_USE_METAL=ON，这个必然失败至今没被发现。
There was a problem hiding this comment.
Metal 截图用例与 per-backend 基线隔离已在 commit e1923d1 中整体移出本 PR。原因：update_baseline.sh 会切到 main 分支构建并运行 UpdateBaseline 生成基线缓存，而 main 分支目前没有 Metal 相关代码（PAGFullTest_Metal target 与 Metal 基线生成逻辑都不存在），本 PR 内补 key 也无法让脚本在 main 上生成 Metal 基线。因此计划等本 PR 合入 main 后，再单独提 PR 合入 Metal 测试代码与基线。Metal 渲染结果已在本机通过与 GL 后端逐用例手动对比确认一致。
| echo "~~~~~~~~~~~~~~~~~~~Update Baseline ($BACKEND_NAME) Failed~~~~~~~~~~~~~~~~~~" | ||
| COMPLIE_RESULT=false | ||
| fi | ||
| else |
There was a problem hiding this comment.
[Blocker] 降级分支使本 PR 整个 CI 期间的截图比对静默通过
此 else 分支只有 4 行 echo、没有 exit 1，脚本以成功语义继续。配合 Baseline.cpp:47 把 cache 根目录改为 .cache/{backend}/，链路是：
CI 从 main 生成 cache → main 上只有旧的通用 UpdateBaseline target → :111 的 grep 不命中 → 走此分支不生成 cache → cacheVersion 恒空 → Baseline.cpp:306 的 baselineVersion == cacheVersion 为假 → 整个 if 为假 → 无条件 return true。
仓库内没有任何基线图（git ls-files test/baseline 只有 version.json），门禁 100% 依赖 md5 cache，cache 缺失即门禁归零。而 :104-109 的注释原文写明了意图：「skip the baseline generation step gracefully ... so the downstream PAGFullTest does not fail」。
本 PR 恰好改了 13 个 filter 的 uniform 绑定，并把 tgfx 推进 9 个 commit（含 Unify uniform buffer bindings、Fix GLSL varying location mismatch、Keep BGRA hardware buffers as BGRA，均直接影响像素输出）——这次 CI 绿灯不构成任何渲染无回归的证据。
修复方案评估（「保留通用 UpdateBaseline 别名」这条不可行：脚本先 git switch main，grep 的是 main 的 target 列表，别名加在 PR 分支上无效）：
推荐在此 else 分支内用 main 的旧 target 生成 cache 再映射到新路径，约 6 行：
cmake --build . --target UpdateBaseline -- -j 12 || COMPLIE_RESULT=false
./UpdateBaseline || COMPLIE_RESULT=false
mkdir -p "../test/baseline/.cache/${BACKEND_NAME}"
cp ../test/baseline/.cache/{version.json,md5.json} "../test/baseline/.cache/${BACKEND_NAME}/"main 上这次构建同样带 $CMAKE_BACKEND_ARGS，产出的 md5 正是要对比的基准；HEAD 不用搬（CMakeLists.txt:1029 在 configure 阶段自写，且 currentVersion 不参与门禁判定）。合并后此段即成死码，可 follow-up 删除。
另建议顺带贴一次本地完整 GL 截图零 diff 的记录到 PR，作为本次改动的渲染正确性证据。
根因级 follow-up（不阻塞本 PR）：
Baseline.cpp:305-311把「baseline 有 key 而 cache 无 key」当作「跳过并通过」，失败方向是反的。只要这个不对称还在，未来任何一次 cache 路径/schema 变更都会重新打开同一个洞。建议区分baselineVersion != cacheVersion（真在接受变更 → 跳过）与cacheVersion.empty()（cache 没建起来 → 失败并提示）。
There was a problem hiding this comment.
感谢详细的评审！关于 Metal 基线与测试的后续安排说明一下：update_baseline.sh 会 git switch main 并在 main 分支上构建运行 UpdateBaseline 来生成基线缓存，而 main 分支目前还没有 Metal 的相关代码（包括 PAGFullTest_Metal target 与 Metal 基线生成逻辑），即使在本 PR 中补齐 Metal 的基线 key，脚本切到 main 后也无法生成 Metal 基线。因此 Metal 相关的测试代码（含 per-backend 基线隔离）已移出本 PR，等本 PR 合入 main 之后再单独提 PR 合入，届时脚本即可在 main 上正常生成 Metal 基线。Metal 后端的单测结果已在本机通过与 GL 后端逐用例手动对比确认一致。
| return false; | ||
| } | ||
|
|
||
| bool Devices::CanSampleFrom(tgfx::Context* /*context*/, const ExternalDeviceRef* /*deviceRef*/) { |
There was a problem hiding this comment.
[Major] CanSampleFrom() 无条件 return true，GL 原有的跨设备保护在 Metal 上完全消失
改动前 Picture.cpp 的 checkContext() 执行 glDevice->sharableWith(sharedContext)，不满足则 LOGE + 拒绝绘制。现在两个参数都被注释掉、无条件 return true；且 :99-105 的 CaptureCurrent() 直接 return nullptr，deviceRef 恒为空。
注释里的两个论断都不成立：
- 「MakeForTexture already forced the render Device ... safe by construction」——
MakeForTexture只作用于PAGSurfaceFactory.cpp:47,78、pagx/PAGSurface.cpp:51,62、pagx/PAGImage.cpp:72这些创建路径。而PAGImage::FromTexture→StillImage.cpp:69→Picture::MakeFrom(BackendTexture)→checkContext()这条渲染期校验路径根本不经过它（已逐跳验证），目标 PAGSurface 的 device 是独立创建的。 - 「once tgfx exposes that」——
tgfx::MetalDevice::metalDevice()早已公开，本 PR 自己的test/src/utils/TestUtils_Metal.mm:51-55就在用它。校验完全可以实现，与Devices_GL.cpp:100-102同构。
失效场景：macOS 多 GPU 或用户自带 MTLDevice 时，纹理属 device A、PAGSurface 属 device B，被静默接受。Metal 禁止跨 MTLDevice 使用资源：开 API Validation 直接断言中止，关闭时为未定义行为。tgfx 侧也不校验（MetalGPU::importBackendTexture / MetalTexture::MakeFrom 从不比较 metalTexture.device）。
建议：用 MetalDevice::metalDevice() 实现真实比较；若确定本期不做，至少改为 LOGE 一次说明校验不可用并在文档写明限制，不要静默返回 true。
There was a problem hiding this comment.
已在 commit 3dd83c1 中修复：Devices::MakeForTexture() 现在从外部纹理回溯其所属的 MTLDevice 并记录到 MetalExternalDeviceRef，CanSampleFrom() 按设备身份严格比对（Devices_Metal.mm:137-153）：deviceRef 为空才走"trust the caller"，context 与纹理设备不一致时直接拒绝绘制，恢复了 GL 侧 sharableWith() 同级别的跨设备保护（Metal 语义为 MTLDevice 指针相等）。
| return [CAMetalLayer layer]; | ||
| } | ||
|
|
||
| - (void)updateLayerDrawableSize { |
There was a problem hiding this comment.
[Major] macOS 缺 viewDidChangeBackingProperties 与 setFrameSize:，尺寸/缩放变化后 drawableSize 陈旧
Metal 路径下 layer.contentsScale 与 layer.drawableSize 由 libpag 手工维护（:65-66 的注释也承认 NSView 不会自动跟踪 backingScaleFactor），但 updateLayerDrawableSize 的调用点只有 :118（setBounds:）、:133（setFrame:）、:196（initPAGSurface），缺两个关键入口：
viewDidChangeBackingProperties：全仓库 grep 零命中。窗口在不同 backing scale 的显示器间移动时 bounds 不变，setBounds:/setFrame:都不触发 → 缩放停留在旧屏 → 内容明显模糊或被放大裁切。setFrameSize:：AppKit 的 superview autoresizing 走-setFrameSize:/-setFrameOrigin:，不经过-setFrame:→ 窗口拉伸时 drawableSize 不更新。
iOS 侧是完整的：ios/PAGView.mm:144-151 覆写了 setContentScaleFactor:。macOS 无对等实现——这个不对称正是两平台 Metal 逻辑复制粘贴导致的（initPAGSurface 的 Metal 分支 22 行几乎逐字相同，连 NSLog 字符串都一样；updateLayerDrawableSize 亦然）。本 PR 已建立 src/platform/cocoa/private/ 用于共享（MetalGPUDrawable 就放在那里），建议把「创建 device + 配置 CAMetalLayer + 计算 drawableSize」也收敛过去，否则相关问题都要修两遍。
建议：
- (void)viewDidChangeBackingProperties { [super viewDidChangeBackingProperties]; if (pagSurface != nil) { [self updateLayerDrawableSize]; [pagSurface updateSize]; } }
- (void)setFrameSize:(NSSize)newSize { [super setFrameSize:newSize]; /* 同上 */ }There was a problem hiding this comment.
已在 commit 73be597 中补齐：mac/PAGView.mm 实现了 setFrameSize:（处理 autoresizing 布局路径，143-146 行）和 viewDidChangeBackingProperties（处理 Retina 缩放变化，165 行起），两者都会同步刷新 layer.drawableSize 与 contentsScale，尺寸/缩放变化后 drawable 不再陈旧。
| _metalInitRetries = 0; | ||
| layer.device = device; | ||
| layer.pixelFormat = MTLPixelFormatBGRA8Unorm; | ||
| layer.framebufferOnly = YES; |
There was a problem hiding this comment.
[Major] framebufferOnly = YES 叠加 present 后释放 drawable，Metal 下截图/回读可能整体不可用（请确认）
两个叠加问题：
layer.framebufferOnly = YES使 drawable 纹理不能作为 blit 源，而 tgfx 回读走 blit encoder →makeSnapshot/copyPixelsTo:/readPixels失效。- tgfx
MetalDrawableProxy（已在锁定版5cf4fcce确认原文）：getRenderTarget()惰性调[_metalLayer nextDrawable]并缓存_renderTarget；而MetalWindow::onPresent()→proxy->releaseDrawable()→_metalDrawable = nil; _renderTarget = nullptr;→ present 之后再取 render target 会获得一块全新的空白 drawable，于是readPixels读到的不是刚显示的那一帧。
合起来的后果是：Metal 后端下对屏幕 PAGSurface 截图返回空白或失败。GL 后端不受影响（GL framebuffer 可直接 glReadPixels）。
机制已在 tgfx 源码层面确证，但未实机运行验证，请确认。 若成立，建议需要回读时 framebufferOnly = NO（或按需切换），并确认回读时序在 present 之前；同时明确 PAGSurfaceImpl.mm 的 makeSnapshot / copyPixelsTo: 在 Metal 窗口 surface 上的可用性。
There was a problem hiding this comment.
确认存在该限制，已按文档化处理（commit 64f470a）：makeSnapshot 在 Metal 后端不可靠的限制已写入文档——framebufferOnly = YES 保留在 PAGMetalLayerHelper（on-screen 渲染性能最优，回读需求不是 PAGView 的主路径），需要可靠像素回读的场景引导使用 PAGSurface 的离屏路径（FromCVPixelBuffer: / MakeOffscreen:），该路径的 readPixels 已验证可用。Metal 下 makeSnapshot 的具体表现是返回 nil 而非崩溃。
There was a problem hiding this comment.
补充确认与后续方案：评审所述两个叠加问题（framebufferOnly = YES 禁止 drawable 纹理作为 blit 源；present 后 releaseDrawable() 使再取 render target 得到全新空白 drawable）机制成立，已在 tgfx 源码层面复核实锤。
解决方案已提交 tgfx PR：Tencent/tgfx#1586（Expose per-frame drawables on all backends for manual presentation and pixel readback）。它向所有后端暴露公共 Drawable：window->nextDrawable(context) 显式取单帧 drawable，Surface::MakeFrom(context, drawable) 创建的 surface 在 submit 时不会自动 present，drawable->readPixels() 在 submit 之后、present 之前 结果确定（此时帧缓冲仍归调用方所有）。该 PR 已通过 Metal 625/625、OpenGL 621/621 全量测试。
tgfx #1586 合入后，libpag 侧按以下方式落地（依赖 DEPS 升级到包含该 PR 的 tgfx 版本）：
PAGMetalLayerHelper将layer.framebufferOnly置为NO——drawable 纹理可作 blit 源是回读的前提；makeSnapshot/copyPixelsTo:的 Metal 实现不再走 window surface 的readPixels（自动 present 路径下内容未定义），改为手动 Drawable 路径：window->nextDrawable(context)→Surface::MakeFrom(context, drawable)→ 渲染 +flushAndSubmit→drawable->readPixels()→drawable->present()，回读严格落在 submit 与 present 之间的窗口内；- 正常显示路径继续使用现有自动 present 的
Surface::MakeFrom(context, window)，行为与性能不受影响（是否可以预期特效的播放内存占用？ #1586 中已验证自动路径 present 排定后立即释放 drawable，不会占用轮转池导致掉帧）。
在本 PR 内维持已合入的文档化处理（64f470a），上述集成与 DEPS 升级在 tgfx #1586 合入后跟进。
| CAMetalLayer* layer = (CAMetalLayer*)[self layer]; | ||
| CGSize size = self.bounds.size; | ||
| CGFloat scale = self.contentScaleFactor; | ||
| layer.drawableSize = CGSizeMake(size.width * scale, size.height * scale); |
There was a problem hiding this comment.
[Major] 主线程写 layer.drawableSize 与渲染线程读之间存在数据竞争（请确认）
iOS PAGView 默认 async 渲染，此处 updateLayerDrawableSize 在主线程写 layer.drawableSize，而后台渲染线程读 metalLayer.drawableSize 并调 [metalLayer nextDrawable]（tgfx MetalWindow.mm:121-126 + MetalDrawableProxy.mm:65-67），CALayer 属性无同步保护。
另外，GL 路径原有针对 issue #1870 的「非主线程创建 surface」规避（:77-82 的 AsyncSurfacePreparedNotification，被 #if TGFX_USE_OPENGL 包裹）在 Metal 分支没有对等机制。这一条措辞上我们做了下调：CAMetalLayer 的 nextDrawable 在渲染线程调用是 Apple 认可的用法，Metal 下未必必崩，所以定性为能力回退（主动放弃了 GL 时代的保护且未说明理由），而非确定性崩溃。
建议：drawableSize 的读写加同步，或统一在主线程更新完再交给渲染线程；并说明 Metal 下是否仍需等价于 #1870 的规避。
范围说明（避免误查）：
+FromMTKView:那条路另有两个主线程约束（MetalGPUDrawable.mm:39读view.layer；首帧getDevice()懒加载再读一次），且 tgfx 的检查只 LOGE 不拦截——MetalWindow.mm:113-118打完日志后第 119 行照样执行metalLayer.drawableSize = metalView.drawableSize;，libpag 不能指望它兜底。但PAGView 走的是FromMetalLayer:，不受 MTKView 那条影响；反过来本条的数据竞争也与 MTKView 无关。两者请勿互相顶替。
There was a problem hiding this comment.
已在 commit 7e013eb 中修复：主线程的 layer.drawableSize 写入改为与渲染 flush 串行化——更新持有了与 onAnimationFlush 相同的锁（ios/PAGView.mm:134 注释说明了该不变式），渲染线程不会再读到撕裂的 size；文档同步说明了该约束。
| CODE_SIGN_STYLE = Automatic; | ||
| COMPRESS_PNG_FILES = YES; | ||
| DEVELOPMENT_TEAM = 36588GHG2N; | ||
| "EXCLUDED_ARCHS[sdk=iphonesimulator*]" = x86_64; |
There was a problem hiding this comment.
[Major] EXCLUDED_ARCHS 无条件排除 x86_64，Intel Mac 连默认 GL demo 都跑不起来
此处（以及 :651）无条件加了 EXCLUDED_ARCHS[sdk=iphonesimulator*] = x86_64，但 demo 默认后端仍是 OpenGL（ios/README.md:46）→ Intel Mac 上连默认 GL 配置的模拟器都无法运行。min iOS 12→13 也是无条件提升。
建议：把 EXCLUDED_ARCHS 与 min iOS 版本改为仅在 Metal 配置下生效，不要污染默认 GL 配置。
这条容易在长报告里被滑过去，但对贡献者的实际影响很直接——拿到仓库跑不起来 demo。
There was a problem hiding this comment.
已在 commit 653e2cb 中修复：demo 恢复 OpenGL 默认配置，EXCLUDED_ARCHS[sdk=iphonesimulator*] = x86_64 改为仅在 if (PAG_USE_METAL AND IOS) 时设置（CMakeLists.txt:150-152），Intel Mac 上默认 GL demo 恢复可构建运行；Metal 的 arm64-only 限制保留在 Metal 分支内并由 README 说明。
| # Clean stale build directory from previous interrupted runs. | ||
| rm -rf "$BUILD_DIR" | ||
|
|
||
| cmake -G Ninja $CMAKE_ARGS -DPAG_BUILD_TESTS=ON -DPAG_SKIP_BASELINE_CHECK=ON \ |
There was a problem hiding this comment.
[Major] 多后端化引入「跨后端盲接受」，Metal 像素正确性从此永久无人校验
Baseline.cpp:300-303 在 #ifdef UPDATE_BASELINE 下第一行就是 setJSONValue(outputMD5, key, md5); return true;——无条件把当前渲染结果写成新基线。而本脚本现在一次串行接受两个后端。
失效场景：开发者执行 /accept-baseline 时通常只肉眼看了 GL 的图，但 .cache/metal/md5.json 同时被锁定成「Metal 此刻的输出」。推演到底：Metal 若存在渲染 bug，此后所有 PAGFullTest_Metal 对共享 key（如 PAGBlendTest/Blend_*）——baselineVersion 非空、.cache/metal/version.json 是 baseline 副本故两者相等 → 转而比对刚被设成 Metal 自身输出的 md5 → 一律通过。Metal 像素正确性从此永久无人校验。
建议：要求显式指定接受哪个后端，或对每个后端分别产图并要求人工确认，不要一次性把未经目检的后端输出写成基线。
另两条相关 Minor：①
test/out/未像.cache/那样按后端隔离，第二轮的remove_all(test/out)会删掉第一轮的_base.webp（但这些图从不入库也不参与门禁，仅影响人工目检，故只算 Minor）；② 本脚本不可重跑——入口:21-25要求test/out/version.json存在，而:96-97未传-DPAG_SKIP_GENERATE_BASELINE_IMAGES，saveData()在UPDATE_BASELINE分支只写 cache、从不写outVersionPath，第一轮跑完该文件即被删且不重建，再次执行直接撞 "Error: test/out/version.json not found"。
There was a problem hiding this comment.
同意"跨后端盲接受"不可接受。per-backend 基线隔离（含 Baseline.cpp 按 backend 校验、accept 流程的后端匹配）已在 commit e1923d1 中随 Metal 测试基建整体移出本 PR——原因同基线生成：update_baseline.sh / accept_baseline.sh 的工作流依赖 main 上存在对应 target，main 尚无 Metal 代码。本 PR 合入后，后续 PR 会带着按后端隔离的 accept/compare 逻辑与 Metal 基线一起合入，届时 Metal 像素正确性由 CI 门禁持续校验。过渡期内 Metal 与 GL 的逐用例手动对比已全部通过。
Minor / Nit 汇总（40 条）这些不阻塞合入，按主题合并在此，避免逐条行级评论淹没 PR。每条都已读代码验证。 一、对外 API 形态与文档（13 条）
ObjC 平台头
二、注释 / 文档与实现脱节（9 条）
三、代码结构 / 命名（5 条）
四、其他（13 条）
补充：编码规范穷尽检查结果对 3281 行新增代码逐条正则扫描，全部通过：
感谢这次扎实的工作——解耦方向是对的， |
…lt device while it is already held no longer deadlocks on the deduplicated shared device.
…der so framework consumers compile clean.
…so IDE indexing stays clean.
…rs for consumers without the build define.
…me lifecycle check passes.
… for the Xcode 27 lifecycle check.
…s the scene delegate with the Main storyboard.
…fix the iOS 27 simulator black screen
|
感谢 @shlzxjp 非常详尽的评审，23 条 Blocker/Major 已逐条回复（见各行内评论），整体处理情况汇总： 已按建议修复（20 条）：
文档化/确认（1 条）：
移出本 PR、合入后跟进（3 条）：
|
|
回复 Minor / Nit 汇总（40 条），逐条处理状态如下（编号对应原评论）： 一、对外 API 形态与文档（1-13）
二、注释 / 文档与实现脱节（14-22）
三、代码结构 / 命名（23-27）
四、其他（28-40）
另外感谢"评审严谨性说明"里那些防止误改的提示（ 编码规范穷尽检查通过的结果也收到，后续新代码会保持该标准。 |
shlzxjp
left a comment
shlzxjp
left a comment
There was a problem hiding this comment.
第二轮审查（head 8871f97f）的行级意见共 6 条：Blocker 1 / Major 1 / Minor 4。每条都经过独立二次确认，涉及 tgfx 的结论均以 DEPS 锁定的 200dfff 源码核对，死锁和 AppKit 行为已实测。
| (oldBounds.size.width != bounds.size.width || oldBounds.size.height != bounds.size.height)) { | ||
| // Hold the same lock as onAnimationFlush so the main-thread write to layer.drawableSize can | ||
| // never overlap with the render thread reading it inside flush (tgfx MetalWindow). | ||
| std::lock_guard<std::mutex> autoLock(lock); |
There was a problem hiding this comment.
[Blocker] sync 模式下主线程死锁（7e013eb 引入）
这里持有 lock 期间调用了 [animator update]（:140）。PAGAnimator::update() 在 _isSync 为 true 时会同线程直接 doUpdate → onFlush → onAnimationFlush:（:268），而 :268 再次 lock_guard 同一把非递归 std::mutex → 同线程重入，主线程永久挂起（已实测）。setFrame:（:151/:155）同理。
触发条件：调用公开 API setSync:YES，且 view 从 0 尺寸布局到非 0（如 CGRectZero 创建后再 layout，属常见路径）。这把锁没有用 TGFX_USE_METAL 隔离，默认 GL 构建同样会中；main 上没有这把锁。
建议：
- 把
[animator update]移到锁的作用域外，锁只包住updateLayerDrawableSize+[pagSurface updateSize]； - 这三处新加的锁（:136/:151/:165）用
#if defined(TGFX_USE_METAL)包起来，GL 不需要它（GL 下updateSize已由 rootLocker 与 flush 串行）。
There was a problem hiding this comment.
两条建议都已采纳（302942382）：
[animator update]已移到锁作用域外（setBounds:/setFrame:两处均调整）。真正的 flush 发生在onAnimationFlush:自己的锁内，layer.drawableSize与渲染线程的串行化保护不受影响。- 三处新加的锁（
setBounds:/setFrame:/setContentScaleFactor:）已用#if defined(TGFX_USE_METAL)包起来，GL 路径回到与 main 完全一致。
另外在 iPhone 16 Pro 模拟器上做了运行时对照验证，触发路径与本条意见一致（CGRectZero 创建 → addSubview 挂 window 创建 PAGSurface → setSync:YES → setFrame: 到 300×300），使用 ios/PAG 工程默认配置（PAG_USE_METAL=ON，即锁存在的 Metal 路径）：
- 修复前（8871f97fc）：主线程在 resize 处永久挂死，watchdog 线程 15 秒后强制退出进程
- 修复后（302942382）：resize 约 4ms 内正常返回，sync flush 正常执行
两条路径互为对照，也确认了本条 Blocker 的触发条件分析完全准确。
| // Nested pairs reuse the already-locked context (pag::DeviceLockScope) instead of re-entering | ||
| // tgfx's non-recursive device mutex, which deadlocks once devices are deduplicated per native | ||
| // GPU object (tgfx #1581). | ||
| auto scope = std::make_unique<pag::DeviceLockScope>(); |
There was a problem hiding this comment.
[Major] GPUContext 不再缓存 device，GL 下 SVG/PPT 导出每次栅格化都新建并销毁 GL 上下文（b6ec8e89f 引入）
main 和第一轮版本里 GPUContext 都缓存了 _device，同一个导出器只建一次。现在每次 lockContext() 都新起一个最外层 DeviceLockScope → Devices::MakeDefault() → GL 下 GLDevice::MakeWithFallback() → GLDevice::Make() 新建 GL 上下文（GL 不参与 tgfx #1581 去重），unlock() 时随 scope 一起释放。
SVG/PPT 导出路径上没有外层 scope（SVGExporter.cpp:803/3023、PPTExporter.cpp:710-712、PPTStyleEmitter.cpp:284 → 本文件 RenderTiledPattern/RenderMaskedLayer 等），所以每个图案填充/遮罩层都要重建上下文并重编 shader，导出明显变慢。Metal 因去重不受影响；测试因外层 scope 和 DevicePool 常驻设备而掩盖了这个问题。RasterUtils.h:36-37 的类注释 "avoid repeated GL context creation" 现在与实现相反。
建议：恢复 std::shared_ptr<tgfx::Device> _device 的懒创建缓存，在 GPUContext 内自行维护嵌套深度（只有最外层才 _device->lockContext()/unlock()）；或者给 DeviceLockScope 增加一个对指定 device 加锁的构造函数，供这里复用。
There was a problem hiding this comment.
已修复（302942382），但采用了你给的备选方案（给 DeviceLockScope 增加对指定 device 加锁的构造函数），没有采用主方案（GPUContext 内部维护嵌套深度 + 裸调 _device->lockContext()），原因是主方案有死锁回归风险：
当外层已有 DeviceLockScope 持有默认设备、而 GPUContext 缓存的 device 恰好是同一实例时（Metal 下 MakeDefault() 经 #1581 去重必然返回同一设备），裸调 _device->lockContext() 会同线程重入 tgfx 的非递归互斥锁——这正是 b6ec8e8 修掉的问题。目前持有外层 scope 的 HTML 渲染器恰好不经过 RasterUtils 所以不会触发，但这个组合一旦出现就是死锁，所以没有走这条路。
具体改动：
DeviceLockScope新增explicit DeviceLockScope(std::shared_ptr<tgfx::Device>)，默认构造转发到它；tls 重入语义完全保留（外层 scope 存在时复用其 context、传入的 device 被忽略并已注释说明）GPUContext恢复std::shared_ptr<tgfx::Device> _device懒创建缓存，首次MakeDefault()后常驻复用，SVG/PPT 导出全程单一 GL 上下文，不再逐 bake 重建RasterUtils.h:38-41的注释已改回"缓存复用"并补充了原因
顺带说明：这个新构造函数也被 PAGXRuntimeTest 的清理修复（下面第 5 条）直接复用了。
| private: | ||
| // The initialized state is derived from _backend plus the stored handle instead of a dedicated | ||
| // flag, which keeps sizeof(BackendSemaphore) and member offsets identical to earlier releases | ||
| // (binary compatibility). |
There was a problem hiding this comment.
[Minor] 「binary compatibility」的承诺不成立：isInitialized() 由 inline 改为 out-of-line 后，旧二进制会读到错误字节
main 上 isInitialized() 是头文件内联的 return _isInitialized;，按旧头文件编译的 App（release 构建会内联）固定读偏移 16 的 bool。新布局里偏移 16 是 _mtlInfo.value，GL 路径的构造函数和 initGL() 都不写它 → 旧 App 链接新库后读到的是未初始化字节（实测返回非法 bool 0x14；initGL 传入有效 sync 后反而返回 false）。sizeof 和 _backend/union 偏移虽然没变，但被内联的旧读取方式已经失效。
说明：第一轮评审我们建议删除
_isInitialized时只核对了 sizeof 和偏移，遗漏了这一点，这里一并更正。
修改建议：
- 推荐：如实声明为二进制不兼容变更。 删掉 :362-364 注释里「keeps sizeof and member offsets identical ... (binary compatibility)」的承诺，改为说明初始化状态由
_backend+ handle 推导，isInitialized()改为 out-of-line 是为了今后布局可以演进。在 release notes/CHANGELOG 中标注BackendSemaphore与旧版头文件二进制不兼容，使用方需用新头文件重新编译。保留 out-of-line 实现，它本身是对的：以后再改布局，就不会再出现调用方内联读取旧偏移的问题。 - 不推荐：坚持兼容旧二进制。 这样只能在偏移 16 继续维护一个
bool _isInitialized（initGL/initMetal/拷贝时同步写入），并把MtlEventInfo::value移出 union、追加到末尾。代价是 sizeof 回到 32，旧 App 栈上分配 24 字节的对象会被越界写入，兼容性反而更差。在 24 字节内无法同时容纳偏移 16 的 bool 和 Metal 的 event+value，所以完全兼容做不到。
实际受影响的场景较窄：旧头文件编译的 App 不重新编译、直接替换新的动态库。所以定为 Minor，但注释里的承诺应当修正。
There was a problem hiding this comment.
同意，24 字节内确实无法兼容（偏移 16 无法同时容纳旧 bool 和 MtlEventInfo::value，而旧二进制内联读取偏移 16 的行为也无法从新库侧修复）。已按推荐处理（302942382）：删除了 :362-364 的兼容性承诺注释，替换为如实声明——布局为二进制不兼容变更、按旧头编译的二进制会读到未初始化字节、集成方必须用新头重新编译。release notes 会同步标注 BackendSemaphore 的这一变更。
| pagPlayer->setMatrix(Matrix::I()); | ||
| pagPlayer->setProgress(0.5); | ||
| pagPlayer->flush(); | ||
| EXPECT_TRUE(Baseline::Compare(pagSurface, "PAGBlendMetalTest/CopyDstTexture")); |
There was a problem hiding this comment.
[Minor] 残留的 Metal 截图用例缺基线 key，Metal 构建下必然失败
e1923d1 移除了 per-backend 测试基建，但用例本身没有删：本文件 :250/:283/:322、PAGSurfaceTest.cpp:304/333、PAGXRuntimeTest.cpp:500/565 共 7 处 Baseline::Compare 的 key（PAGBlendMetalTest/*、PAGSurfaceMetalTest/*、PAGXRuntimeTest/Metal*）都不在 test/baseline/version.json 中。按 Baseline.cpp 的逻辑，baselineVersion.empty() 会直接返回 false，所以 PAG_USE_METAL=ON 下 PAGFullTest 这 7 条一定失败。TestUtils_Metal.mm 和 TestUtils.h 里的 Metal 辅助函数现在也只被这些用例使用。
建议与作者回复中「Metal 测试随后续 PR 合入」的计划保持一致：把这 8 个 Metal 用例连同 Metal 测试辅助代码一起移到后续 PR，和基线 key 一起提交。
There was a problem hiding this comment.
已按建议全部移出本 PR（302942382）：8 个 Metal 用例（PAGBlendMetalTest 4 个、PAGSurfaceMetalTest 2 个、PAGXRuntimeTest 的 Metal 2 个）连同 TestUtils_Metal.mm、TestUtils.h 中的 Metal 辅助声明一起删除，相关注释已注明"后续 PR 与基线 key 一起回来"。用例代码保留在本分支历史（8871f97fc 及之前）中，可直接恢复。
验证：PAG_USE_METAL=ON 构建下受影响套件全绿，不再有缺失 key 的必失败用例。
| { | ||
| pag::DeviceLockScope lock; | ||
| ASSERT_TRUE(static_cast<bool>(lock)); | ||
| glDeleteTextures(1, &textureInfo.id); |
There was a problem hiding this comment.
[Minor] GL 下清理代码作用在另一个 GL 上下文上，纹理/FBO 实际没有被删除（第二轮引入）
纹理是在用例开头那个 DeviceLockScope 的 device A 上创建的。这里新起了一个最外层 DeviceLockScope，GL 下 MakeDefault() → GLDevice::Make() 会新建一个不共享的上下文 B（GL 不参与去重）。glDeleteTextures 在 B 上执行，名字在 B 中不存在，相当于 no-op，A 上的纹理泄漏。:429-430 的 glDeleteFramebuffers/glDeleteTextures 同理。main 和第一轮版本都是在同一个 fixture device 上清理的。
建议：清理时锁定创建纹理的同一个 device，例如在用例开头持有该 device 的 shared_ptr，创建和清理两个阶段都对它 lockContext()/unlock()，而不是再新起一个 DeviceLockScope。
There was a problem hiding this comment.
已修复（302942382）。两个用例统一改为：开头 pag::Devices::MakeDefault() 取一次 device shared_ptr，创建与清理两个阶段都用 DeviceLockScope(device)（即上面第 2 条意见中新增的构造函数）锁定同一设备，glDelete* 落在创建纹理/FBO 的上下文上。
| } | ||
| } | ||
|
|
||
| - (void)setFrameSize:(NSSize)newSize { |
There was a problem hiding this comment.
[Minor] setFrame: 内部会调用 setFrameSize:，一次 resize 会把尺寸更新处理执行两次（73be597）
AppKit 中 -[NSView setFrame:] 内部会调用 -setFrameSize:（已用最小 AppKit 程序实测：直接 setFrame:、autoresizing、Auto Layout 三条路径都会走到 setFrameSize:），所以 :146-147 注释里「does not pass through setFrame:」的前提不成立。结果是一次 resize 中 updateLayerDrawableSize + [pagSurface updateSize]（会 PurgeCaches + releaseAll + freeSurface）执行两次；从 0 尺寸变为非 0 时还会连续做两次同步整帧渲染，中间的缓存被清空。GL 构建同样受影响。
建议：尺寸变化的处理只保留在 setFrameSize: 中（它覆盖了所有路径），去掉 setFrame:（:128-141）中重复的处理逻辑。
There was a problem hiding this comment.
已修复（302942382）：删除了 mac 的 setFrame: override，尺寸处理只保留在 setFrameSize:（setFrame:、autoresizing、Auto Layout 三条路径都汇聚到这里），并修正了原来方向写反的注释（"does not pass through setFrame:" → "setFrame: calls it internally"）。
顺带说明：iOS 侧 setFrame: 内部调 setBounds: 导致的双重执行是 main 已有的历史行为（非本 PR 引入），未在本 PR 处理，后续单独跟进。
…ed export device and defer Metal tests
|
感谢第二轮评审！6 条意见全部成立，已在 3029423 中修复并推送（1 Blocker / 1 Major / 4 Minor），逐条回复见行级评论。要点：
验证情况：GL 与 Metal（ 另有一条已知但未在本 PR 处理的事项：iOS 侧 |
合入前提：Metal 后端需要补上自动化验证
为什么不能延后
本 PR 合入前需要补齐1. Metal 编译 job
2. Metal 行为测试 job（ 用读回像素直接断言颜色值的方式，代替 md5 截图比对，这样完全不依赖基线。至少覆盖两轮修复的关键点：
合入后紧接着的 PR
另外请注意，过渡期不要用「cache 缺失就跳过比对」的方式让 CI 变绿。第一轮评审中，截图门禁正是这样失效的。 上面第 1、2 项补齐、CI 通过之后，就具备合入条件了。 |
# Conflicts: # DEPS # test/baseline/version.json
|
|
||
| #pragma once | ||
|
|
||
| #if defined(TGFX_USE_OPENGL) |
There was a problem hiding this comment.
已按 tgfx 的目录方式拆分（3d4133f17、3fa7d4024）：
- iOS GL 实现移到
ios/private/opengl/EAGLDrawable，mac GL 实现移到mac/private/opengl/CGLDrawable - Metal 实现移到
cocoa/private/metal/MetalLayerDrawable（原MetalGPUDrawable）和cocoa/private/metal/MetalLayerHelper（原PAGMetalLayerHelper） - CMake 只编译当前后端的目录，做法和 tgfx 只在对应
TGFX_USE_*下 GLOBsrc/gpu/opengl或src/gpu/metal一致；后端目录里的文件因此不再用#if TGFX_USE_*整体包裹
已确认 GL 构建里没有任何 private/metal 的编译单元和 include 路径，Metal 构建反之。
| + (PAGSurfaceImpl*)FromLayer:(CAEAGLLayer*)layer; | ||
|
|
||
| + (PAGSurfaceImpl*)FromCVPixelBuffer:(CVPixelBufferRef)pixelBuffer; | ||
| + (PAGSurfaceImpl*)FromMetalLayer:(CAMetalLayer*)layer; |
There was a problem hiding this comment.
这个也是，是不是应该单独拆出来，写到后端的头文件里。对外提供也应该是两个文件。毕竟metal和opengl不共存
There was a problem hiding this comment.
已拆（3d4133f17）：PAGSurfaceImpl.h 只保留后端无关的部分（MakeOffscreen、读回、单参 FromCVPixelBuffer: 的分发等），GL / Metal 的工厂分别放到 private/opengl/PAGSurfaceImpl+OpenGL.h 和 private/metal/PAGSurfaceImpl+Metal.h 两个分类里。对外也是两个文件：PAGSurface+OpenGL.h 和 PAGSurface+Metal.h。mac 同样处理。
| * never actually renders. When the size changes, update layer.drawableSize and call updateSize | ||
| * on the PAGSurface. | ||
| */ | ||
| + (PAGSurface*)FromMetalLayer:(CAMetalLayer*)metalLayer; |
There was a problem hiding this comment.
这里不用叫MetalLayer，我建议是写到两个分类里，都是FromLayer
There was a problem hiding this comment.
已改（3d4133f17）：删掉了 FromMetalLayer:，两个分类都叫 FromLayer:——PAGSurface (OpenGL) 接收 CAEAGLLayer*，PAGSurface (Metal) 接收 CAMetalLayer*。PAGView 内部也改成调用对应分类的 FromLayer:，两个 PAGView.h 注释里残留的旧名一并更新了。
| * never actually renders. When the size changes, update layer.drawableSize and call updateSize | ||
| * on the PAGSurface. | ||
| */ | ||
| + (PAGSurface*)FromMetalLayer:(CAMetalLayer*)metalLayer; |
There was a problem hiding this comment.
mac 这里也和 iOS 一起拆吧，后端相关的工厂放到各自分类里，不要都塞在 PAGSurface 主接口里，再靠运行时返回 nil 区分支持。iOS 的 FromCVPixelBuffer:context: 和 FromCVPixelBuffer:mtlDevice: 也一起归到对应分类。
另外拆文件时，CMake 的公开头列表和 umbrella header 也要一起按后端选择，不能继续把两套分类无差别打包。两边都叫 FromLayer: 时，参数类型不影响 ObjC selector，必须保证只编译、导出对应那一套。公开头不要用 TGFX_USE_* 判断，framework 使用方没有这些构建宏。
There was a problem hiding this comment.
已按建议处理（3d4133f17、3fa7d4024）：
- mac 也拆成
PAGSurface (OpenGL)（FromView:）和PAGSurface (Metal)（FromLayer:）。iOS 的FromCVPixelBuffer:context:归到 OpenGL 分类，FromCVPixelBuffer:mtlDevice:归到 Metal 分类。主接口只留后端无关的入口，不再靠运行时返回 nil 区分。 - CMake 按
PAG_USE_METAL选一套分类，另一套的源文件和公开头都不进 target，只编译、只导出对应那一套。 - umbrella header 改成
libpag.h.in模板，配置时只 import 当前后端的分类头；生成后的头作为 target 源文件拷进 framework。 - 公开头里没有任何
TGFX_USE_*。
验证：
- 对 mac 的 GL / Metal framework 各写了一个
@import libpag的使用方小程序：调用本后端的工厂能编译通过，调用另一个后端的工厂报class method not found。 - iOS 模拟器的 Metal framework 只导出
PAGSurface+Metal.h，二进制里也只有(Metal)分类的符号。 - PAGViewer demo 用到的 import 对 iOS GL / Metal 两个 framework 都能编译。
| } | ||
|
|
||
| #else | ||
| + (PAGSurfaceImpl*)FromCVPixelBuffer:(CVPixelBufferRef)pixelBuffer mtlDevice:(id<MTLDevice>)device { |
There was a problem hiding this comment.
这里把 GL 的模拟器限制套到 Metal 上了。tgfx 的 EAGLGPU 在模拟器上关闭硬件缓冲，但 MetalGPU 的 HardwareBufferAvailable() 返回 true，走的是 CVMetalTextureCache，不受这个限制。
现在 arm64 模拟器的 Metal 构建里，即使传入可用的 IOSurface-backed BGRA CVPixelBuffer，这个新增入口也直接返回 nil，单参 FromCVPixelBuffer: 同样被前面的分支挡住了。应该只在 GL 分支保留模拟器短路，Metal 走正常包装路径。
There was a problem hiding this comment.
已修（3d4133f17）：TARGET_IPHONE_SIMULATOR 的短路现在只留在 OpenGL 分类的 FromCVPixelBuffer:context: 里。Metal 分类的 FromCVPixelBuffer:mtlDevice: 不再受这个限制，走正常的包装路径。单参 FromCVPixelBuffer: 按构建后端分发到对应实现，所以 arm64 模拟器的 Metal 构建也不会再被前面的分支挡住。
| // or GL before any context is current), the capture returns nullptr and the verification is | ||
| // bypassed by Devices::CanSampleFrom() (which trusts a null ref). Only report the missing | ||
| // capture as an error on backends where a captured identity is genuinely required (GL). | ||
| auto deviceRef = Devices::CaptureFromTexture(texture); |
There was a problem hiding this comment.
这里需要区分“没有捕获设备身份”和“纹理后端不匹配”。Metal 构建传入 GL BackendTexture 时，ToTGFX 保留 GL 标签，CaptureFromTexture 返回空，但 RequiresCapturedIdentity() 是 false，最后 PAGImage::FromTexture 仍然返回非空图片。真正绘制时 MetalGPU 拒绝 GL 纹理，图片就一直是空白，C 的 pag_image_from_backend_texture 也一样。
部分 Surface 工厂也只检查纹理描述有效，等到绘制才拒绝不匹配的后端。建议在资源工厂入口统一校验当前后端，不支持就返回空，不要返回一个永远画不出来的对象。通用 BackendTexture 描述本身可以保留多后端标签。
There was a problem hiding this comment.
已修（1ddf02fb0、3fa7d4024）：
- 新增
Devices::SupportsBackend()，在资源工厂入口统一校验纹理的后端标签，不匹配直接返回空并打日志，不再返回一个永远画不出来的对象。BackendTexture本身仍保留多后端标签。 - 覆盖的入口：
PAGImage::FromTexture、PAGSurface::MakeFrom(BackendTexture)/MakeFrom(BackendRenderTarget)、pagx::PAGSurface::MakeFrom的两个重载、pagx::PAGImage::MakeFromTexture。C 接口pag_image_from_backend_texture和pag_surface_make_from_texture走的就是这些入口。 PAGImage::FromTexture里把两种失败分开了：后端不匹配报 "different GPU backend"；后端匹配但 GL 下没有当前上下文、拿不到设备身份时，才报 "no current GPU context"。
验证：Metal 构建传入 GL 纹理、GL 构建传入 Metal 纹理，入口都返回空，每种情况只有一条对应的日志。
| * never actually renders. When the size changes, update layer.drawableSize and call updateSize | ||
| * on the PAGSurface. | ||
| */ | ||
| + (PAGSurface*)FromMetalLayer:(CAMetalLayer*)metalLayer; |
There was a problem hiding this comment.
之前 Metal 截图/回读的限制只补到了 PAGView.h，PAGSurface 这里也要同步。直接用 FromMetalLayer: 创建的 surface 走的是同一套 MetalWindow，present 后释放 drawable，后续 makeSnapshot 和 copyPixelsTo: 也可能拿到空白或旧帧，并不是只有 PAGView 会受影响。mac 的 PAGSurface.makeSnapshot 也一样。
请把这个限制写到对应的 Surface 读回接口上，并明确 getCVPixelBuffer 对 Metal layer 路径返回 nil；现在它的说明只提了 GL 的 FromLayer:/FromView:。这是同一问题的公开接口说明漏了一半，不是新的截图根因。
There was a problem hiding this comment.
已同步（3d4133f17、3fa7d4024）：
- iOS 和 mac 的
PAGSurface.h中，makeSnapshot（iOS 还有copyPixelsTo:rowBytes:）都补上了和PAGView.h一致的限制说明：用 CAMetalLayer 创建的 surface，每次 flush 都会 present 并释放 drawable，之后读回可能拿到空白或旧帧；需要可靠读回请用离屏 PAGSurface（MakeOffscreen:）。 getCVPixelBuffer的说明补上了 Metal：从 layer / view 创建的 surface 都返回 nil，包括 GL 的FromLayer:/FromView:和 Metal 的FromLayer:。
…s and validate backend tags
…ckend's Apple platform files.
|
@Hparty 7 条意见已全部处理，提交为 3d4133f、1ddf02fb0、3fa7d4024，逐条说明见行级回复。要点：
验证：mac GL / Metal 和 iOS 模拟器 GL / Metal 四种配置均编译通过；用 |
Hparty
left a comment
Hparty
left a comment
There was a problem hiding this comment.
本轮复审确认的新问题见下方行内评论。另有几处无法挂在 diff 行上的问题一并说明：
- [A2]
src/c/ext/pag_surface_ext.cpp:23-27（不在本 diff 内）：仍直接#include "tgfx/gpu/opengl/GLDevice.h"并调用tgfx::GLDevice::Make(sharedContext)。PAG_USE_C=ON+ Metal 构建下该文件会引用 OpenGL 专用符号导致编译/链接失败，建议改走pag::Devices收口。 - [A2]
pagx/wechat/src/PAGXView.cpp:171（该行不在 diff 内）：仍调用tgfx::GLDevice::Current()，与 Devices.h 声明的「后端调用统一收口」原则不一致，建议改为pag::Devices::AdoptCurrent().device。 - [A2]
test/src/PAGXTest.cpp:97-104（该行不在 diff 内）：GL 头文件未加TGFX_USE_OPENGL守卫，同 PR 已在 PAGXRuntimeTest.cpp / PAGBlendTest.cpp / TestUtils.cpp 统一加了守卫，唯独本文件遗漏，非 Apple 的非 GL 后端（Linux/Vulkan 等）会因找不到<OpenGL/gl3.h>直接编译失败。 - [A9]
src/pagx/runtime/OffscreenDrawable.h:26：注释仍写 "backs PAGSurface::MakeOffscreen with a fresh OpenGL device"，实现已改为后端无关的Devices::MakeDefault()。
旧问题复核：此前评论的公开头宏包裹、CanSampleFrom、MetalExternalDeviceRef 注释、设备去重、CMake 后端互斥、EXCLUDED_ARCHS 等问题均已修复。仍待确认：① MetalLayerHelper.h 的 framebufferOnly = YES 仍在，Metal 下 snapshot/readback 可靠性建议实测确认；② Metal 工厂失败时返回 nil 但无 LOGE（与之前回复的「nil+LOGE」不符）；③ PAGView 未显式设置 CAMetalLayer.device，MetalWindow::MakeFrom 对 nil device 的行为依赖 tgfx 侧，建议确认；④ include/pag/gpu.h 已在注释中声明 BackendSemaphore 布局二进制不兼容，破坏本身仍在，合入版本节奏请注意。
| } | ||
|
|
||
| DeviceLockScope::DeviceLockScope(std::shared_ptr<tgfx::Device> device) { | ||
| if (tlsLockDepth > 0) { |
There was a problem hiding this comment.
[A6] 嵌套 scope（tlsLockDepth > 0）时完全忽略入参 device，直接复用外层 context。注释称「MakeDefault 返回同一 dedup 实例所以忽略是安全的」，但 RasterUtils.cpp:216-218 的注释又明确说 GL 下每次 MakeDefault 都新建 GL context，两处注释自相矛盾，该前提未经验证。若外层与内层不是同一 device，内层会拿到错误的 GL context 且入参 device 从未被 lock。建议 TLS 同时记录外层锁定的 device，入参与之相同（或为空）才复用，否则走正常 lockContext()；并统一两处注释。
| case tgfx::Backend::Vulkan: | ||
| case tgfx::Backend::WebGPU: | ||
| case tgfx::Backend::D3D12: | ||
| LOGE("PAGSurface::flushInternal() cannot forward a semaphore of this backend yet."); |
There was a problem hiding this comment.
[A9] 日志文案引用了已不存在的方法名 PAGSurface::flushInternal()（现为 draw）。另外 Backend::Unknown 与未实现的 Vulkan/WebGPU/D3D12 共用同一分支：Metal 下若 flush() 未产出 semaphore，调用方传了 signalSemaphore 时每一帧都会打这条 ERROR。建议 Unknown 单独静默或降级为 LOGW，并修正函数名。
| * libpag builds without ARC, so ownership is managed manually: retain in the constructor and | ||
| * release in the destructor, which keeps the object alive for the lifetime of the ref. | ||
| */ | ||
| class MetalExternalDeviceRef : public ExternalDeviceRef { |
There was a problem hiding this comment.
[A12] MetalExternalDeviceRef 在 MRC 下手动 retain/release，但未删除拷贝构造与拷贝赋值，一旦被复制即 double release（GLExternalDeviceRef 是 POD 无此风险，属 GL/Metal 不对称）。建议 = delete 拷贝构造与赋值。
|
|
||
| BackendSemaphore::BackendSemaphore() | ||
| : _backend(Backend::MOCK), _glSync(nullptr), _isInitialized(false) { | ||
| BackendSemaphore::BackendSemaphore() : _backend(Backend::MOCK), _glSync(nullptr) { |
There was a problem hiding this comment.
[A7] BackendSemaphore() : _backend(MOCK), _glSync(nullptr) 只初始化 union 前 8 字节，_mtlInfo.value（偏移 8-15）为不确定值。当前 isInitialized() 对 MOCK 返回 false 使其不可达，但建议直接 _mtlInfo({}) 整体置零，避免后续改动踩到未初始化读取。
| // a no-op at the tgfx layer. Vulkan / D3D12 / WebGPU sync ABI extensions are deferred to | ||
| // the specific backend PRs that introduce user-facing sync APIs. | ||
| if (!semaphore.isInitialized()) { | ||
| return {}; |
There was a problem hiding this comment.
[A4] ToTGFX(const BackendSemaphore&) 仍是 glSync() → getMtlEventInfo() 逐后端试解包，而本 PR 已给 pag::BackendSemaphore 加了 backend() 访问器、PAGSurface.cpp 的转发也已改为 switch 分派。建议这里同样按 backend() 分派，与同文件 ToTGFX(BackendTexture) 的 switch 风格一致，避免后续新后端漏改时静默丢失同步信息。
| @@ -118,6 +178,11 @@ - (void)setContentScaleFactor:(CGFloat)scaleFactor { | |||
| CGFloat oldScaleFactor = self.contentScaleFactor; | |||
| [super setContentScaleFactor:scaleFactor]; | |||
| if (pagSurface != nil && oldScaleFactor != scaleFactor) { | |||
There was a problem hiding this comment.
[A4] mac 侧新增了 viewDidChangeBackingProperties（mac/PAGView.mm:153）处理 backingScale 变化，iOS 没有对应路径：view 移到外接屏或屏幕缩放变化时 contentScaleFactor 未必触发 setContentScaleFactor:，layer.drawableSize 会停留在旧值（模糊/裁切）。建议在 didMoveToWindow 或 traitCollectionDidChange: 中同样调用 updateLayerDrawableSize + [pagSurface updateSize]（需持锁）。
| - (void)setFrameSize:(NSSize)newSize { | ||
| NSSize oldSize = self.bounds.size; | ||
| [super setFrameSize:newSize]; | ||
| // Every size change funnels through setFrameSize:: setFrame: calls it internally, and |
There was a problem hiding this comment.
[A9] 注释称「所有尺寸变化都汇聚到 setFrameSize:」，但 :115 仍保留 setBounds: 重写，逻辑重复且与注释自相矛盾。建议只保留 setFrameSize: 一个入口（与 iOS 侧应做的简化对称）。
| BOOL status = _pagSurface->readPixels(pag::ColorType::BGRA_8888, pag::AlphaType::Premultiplied, | ||
| pixelBufferData, bytesPerRow); | ||
| CVPixelBufferUnlockBaseAddress(hardwareBuffer, 0); | ||
| if (!status) { |
There was a problem hiding this comment.
[A6] readPixels 失败时只 LOGE 仍返回 hardwareBuffer，调用方拿到未写入的空白 buffer 且无法区分失败（iOS 侧失败返回 nil）。建议失败时释放并返回 nil，与 iOS 对齐。
| set(PAG_USE_ANGLE OFF) | ||
| endif () | ||
|
|
||
| # The Metal backend requires iOS 13+ (see the tgfx deployment-target check) and supports the iOS |
There was a problem hiding this comment.
[A9] 这段注释描述的是「Metal 选中时自动提升部署目标」，但下方 if 块只有 NOT APPLE 的 FATAL_ERROR 和 EXCLUDED_ARCHS，真正的部署目标设置在文件顶部 9-11 行；且注释写 iOS 13+，代码与 README 实际是 15.0。建议删除或合并到 9-11 行处并修正版本号。
| ``` | ||
| ./gen_ios -DPAG_USE_METAL=ON | ||
| ./gen_simulator -DPAG_USE_METAL=ON | ||
| ``` |
There was a problem hiding this comment.
[C5] 英文版 ios/README.md 同段落多出一句中文版没有的提示（PAGViewer demo target 保持默认部署目标，需要在 Xcode 里手动升到 15.0），中英 README 不对齐，建议补上同义句。
4 participants
实现 libpag 的 Metal GPU 后端，覆盖 iOS 与 macOS。
核心改动：
验证：macOS / iOS 模拟器 / iOS 真机三端动画循环 + Retina 缩放 + 内容上屏通过，Metal HTMLTest 651 case 全过。
后续：device 注入 API（多 GPU 场景）留待 Vulkan/WebGPU 接入时一并定稿。