Update bgfx and migrate to explicit swap-chain APIs - #1879
bkaradzic-microsoft merged 8 commits into
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Updates Babylon Native’s bgfx integration to API 161 and migrates rendering to explicit swap-chain/back-buffer ownership, including improved capture/screenshot behavior and D3D11 external back-buffer imports.
Changes:
- Migrates framebuffer attachment flags and swap-chain handling to bgfx API 161 semantics.
- Adds explicit swap-chain/back-buffer management (including D3D11 external RTV/DSV imports) and coalesced screenshot/capture readback.
- Updates build/install wiring (bgfx.cmake sync, DXC runtime deployment) and adds new unit tests for swap-chain and callback behavior.
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Polyfills/Canvas/Source/FrameBufferPool.cpp | Updates attachment flag usage for bgfx 161 validation rules. |
| Polyfills/Canvas/Source/Canvas.cpp | Aligns framebuffer attachment flags with new bgfx API. |
| Plugins/ShaderTool/CMakeLists.txt | Deploys DXC runtime alongside ShaderTool when needed. |
| Plugins/ShaderCompiler/CMakeLists.txt | Introduces DXC runtime deployment helper for D3D12. |
| Plugins/NativeXr/Source/NativeXrImpl.cpp | Migrates XR texture import and attachment flags for new bgfx API. |
| Plugins/NativeEngine/Source/VertexArray.cpp | Adjusts instancing path to newer bgfx behavior. |
| Plugins/NativeEngine/Source/NativeEngine.cpp | Updates framebuffer attachment flags for bgfx 161. |
| Plugins/NativeEngine/CMakeLists.txt | Removes minz linkage with updated bgfx dependencies. |
| Plugins/NativeCamera/Source/WinRT/CameraDevice.cpp | Updates camera texture update signature to Graphics::Texture. |
| Plugins/NativeCamera/Source/Win32/CameraDevice.cpp | Updates camera texture update signature to Graphics::Texture. |
| Plugins/NativeCamera/Source/Unix/CameraDevice.cpp | Updates camera texture update signature to Graphics::Texture. |
| Plugins/NativeCamera/Source/NativeVideo.h | Switches video texture update API to Graphics::Texture. |
| Plugins/NativeCamera/Source/NativeVideo.cpp | Routes video updates through Graphics::Texture. |
| Plugins/NativeCamera/Source/NativeCamera.cpp | Updates JS binding path to pass Graphics::Texture. |
| Plugins/NativeCamera/Source/MediaStream.h | Updates MediaStream texture update API to Graphics::Texture. |
| Plugins/NativeCamera/Source/MediaStream.cpp | Updates MediaStream to forward Graphics::Texture. |
| Plugins/NativeCamera/Source/CameraDevice.h | Forward-declares Graphics::Texture and updates API signature. |
| Plugins/NativeCamera/Source/Apple/CameraDevice.mm | Migrates Metal camera import to explicit Graphics::Texture recreation/import. |
| Plugins/NativeCamera/Source/Android/CameraDevice.cpp | Migrates GL camera import to explicit Graphics::Texture recreation/import. |
| Plugins/ExternalTexture/Source/ExternalTexture_D3D11.cpp | Centralizes D3D11 format mapping via shared helper header. |
| Install/Install.cmake | Removes minz from installed bgfx libs list. |
| Documentation/Components.md | Documents new swap-chain/back-buffer ownership and capture behavior. |
| Dependencies/CMakeLists.txt | Adopts synchronized bgfx.cmake config and keeps video disabled. |
| Core/Graphics/Source/FrameBuffer.cpp | Routes default framebuffer handle through DeviceContext back-buffer handle. |
| Core/Graphics/Source/ExternalBackBufferD3D11.h | Adds D3D11 external back-buffer importer interface. |
| Core/Graphics/Source/ExternalBackBufferD3D11.cpp | Implements D3D11 RTV/DSV import and readback capture. |
| Core/Graphics/Source/DeviceImpl_visionOS.mm | Migrates platform setup to bgfx swap-chain API. |
| Core/Graphics/Source/DeviceImpl_macOS.mm | Migrates platform setup to bgfx swap-chain API. |
| Core/Graphics/Source/DeviceImpl_iOS.mm | Migrates platform setup to bgfx swap-chain API. |
| Core/Graphics/Source/DeviceImpl_WinRT.cpp | Migrates WinRT surface wiring to bgfx swap-chain API. |
| Core/Graphics/Source/DeviceImpl_Win32.cpp | Migrates Win32 surface wiring to bgfx swap-chain API. |
| Core/Graphics/Source/DeviceImpl_Unix.cpp | Migrates Unix surface wiring to bgfx swap-chain API. |
| Core/Graphics/Source/DeviceImpl_Android.cpp | Migrates Android renderer selection to bgfx init API. |
| Core/Graphics/Source/DeviceImpl.h | Adds back-buffer handle access and external back-buffer state. |
| Core/Graphics/Source/DeviceImpl.cpp | Implements explicit swap-chain ownership, capture coalescing, and external back-buffer readback. |
| Core/Graphics/Source/DeviceContext.cpp | Exposes back-buffer framebuffer handle via DeviceContext. |
| Core/Graphics/Source/BgfxCallback.cpp | Coalesces screenshot + capture behavior and supports multiple callbacks per frame. |
| Core/Graphics/InternalInclude/Babylon/Graphics/DeviceContext.h | Adds GetBackBufferHandle() API. |
| Core/Graphics/InternalInclude/Babylon/Graphics/D3D11Format.h | Adds shared D3D11 DXGI↔bgfx format mapping. |
| Core/Graphics/InternalInclude/Babylon/Graphics/BgfxCallback.h | Adds capture control helpers for screenshot-based capture. |
| Core/Graphics/Include/Shared/Babylon/Graphics/Device.h | Clarifies back-buffer update contract and window switching semantics. |
| Core/Graphics/CMakeLists.txt | Adds D3D11 external back-buffer sources and removes minz link. |
| CMakeLists.txt | Updates bgfx.cmake dependency pin to synced commit. |
| Apps/UnitTests/Source/Tests.ShaderCompilation.cpp | Adds ShaderCompiler smoke test when available. |
| Apps/UnitTests/Source/Tests.Device.SwapChain.cpp | Adds swap-chain resize/window replacement/capture regression tests. |
| Apps/UnitTests/Source/Tests.Device.D3D11.cpp | Expands D3D11 back-buffer tests for subresources, flags, and MSAA capture. |
| Apps/UnitTests/Source/Tests.BgfxCallback.cpp | Adds unit coverage for capture/screenshot coalescing behavior. |
| Apps/UnitTests/CMakeLists.txt | Wires new tests and DXC runtime deployment for D3D12 shader compilation. |
| Apps/Playground/CMakeLists.txt | Deploys DXC runtime when ShaderCompiler is enabled. |
| Apps/ModuleLoadTest/CMakeLists.txt | Deploys DXC runtime when ShaderCompiler is enabled. |
Suppressed comments (5)
Plugins/NativeXr/Source/NativeXrImpl.cpp:1
- The null-pointer asserts validate
view.*TexturePointer, but the imported pointers passed intocreateTexture2Dcome fromviewConfig.*TexturePointer. IfviewandviewConfigare not guaranteed to reference the same underlying values, the asserts may not protect the actual dereference. Make the asserts validate the same variables that are used for the import (or switch the import to useview.*TexturePointer).
Plugins/NativeXr/Source/NativeXrImpl.cpp:1 - The null-pointer asserts validate
view.*TexturePointer, but the imported pointers passed intocreateTexture2Dcome fromviewConfig.*TexturePointer. IfviewandviewConfigare not guaranteed to reference the same underlying values, the asserts may not protect the actual dereference. Make the asserts validate the same variables that are used for the import (or switch the import to useview.*TexturePointer).
Plugins/NativeXr/Source/NativeXrImpl.cpp:1 - The null-pointer asserts validate
view.*TexturePointer, but the imported pointers passed intocreateTexture2Dcome fromviewConfig.*TexturePointer. IfviewandviewConfigare not guaranteed to reference the same underlying values, the asserts may not protect the actual dereference. Make the asserts validate the same variables that are used for the import (or switch the import to useview.*TexturePointer).
Plugins/NativeEngine/Source/VertexArray.cpp:1 - The instancing capability check was removed, so the instancing path can run even when the backend reports instancing is unsupported. This can lead to invalid encoder state or undefined rendering on backends without instancing. Reintroduce a capability gate (or explicitly throw/early-out) when
m_vertexBufferInstancesis non-empty and instancing is not supported.
Plugins/ShaderTool/CMakeLists.txt:1 - The
deploy_shader_compiler_runtime()function is defined insidePlugins/ShaderCompiler/CMakeLists.txtbut invoked from other directories/targets (e.g., ShaderTool). This introduces a CMake directory-order dependency (the function must be defined before any use). To make configuration robust, move the function to a shared CMake module (or a top-level CMake file) that is included before these targets are added, and then call it from each target that needs the runtime.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
XR framebuffer routing and deferred Apple camera texture lifetime handling contain blocking correctness issues.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 51/51 changed files
- Comments generated: 2
- Review effort level: Balanced
24a0596 to
e51b48a
Compare
Pin the merged bgfx.cmake revision with current upstream build scripts, EGL headless fixes, and the D3D12 explicit-swap-chain MSAA correction. Own window framebuffers independently of the graphics device, retain and import D3D11 back-buffer views, and coalesce capture and screenshot readbacks. Update attachment flags, baseline instancing support, and camera/XR texture imports for the current bgfx API. Deploy the D3D12 shader runtime for native consumers, remove obsolete miniz target references, and preserve disabled video support through the new CMake option. Add swap-chain, capture, external-view, shader, and D3D12 debug-layer regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54aaa1e1-b4b3-46e7-9de4-5a56add4ac42
Keep the installed SDK smoke test limited to public headers and move internal external-backbuffer regressions into a UnitTests-only source. Replace shared-header ASTC macros with namespaced typed constants and clarify renderer selection for the headless bootstrap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54aaa1e1-b4b3-46e7-9de4-5a56add4ac42
Distinguish device-window routing from default framebuffer semantics so XR targets never fall back to the window, including after disposal. Replace deferred Apple camera wrapper pointers with generation-checked shared texture state that rejects updates after disposal or destruction. Add framebuffer routing and deferred texture lifetime regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54aaa1e1-b4b3-46e7-9de4-5a56add4ac42
Vulkan selects the X11 surface's current extent, which left explicit swap chains at the host window's original size after Device::UpdateSize. Resize and synchronize the X11 window before bgfx creates or updates its swap chain. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48fa7ec8-0cf6-48a0-b836-10b7ace38c1a
e51b48a to
7248189
Compare
Restore the existing table names, entries, comments and formatting so the API migration reads as a move rather than an unrelated cleanup. Keep ASTC values scoped to the shared header's namespace and retain the required lookup and depth-view helpers. Restore the original plugin format expression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48fa7ec8-0cf6-48a0-b836-10b7ace38c1a
Reject window depth-only views whose mip, array or read-only semantics cannot be represented by bgfx SwapChain::depth. Cover rejection and recovery without restricting explicit color/depth framebuffers. Remove the added D3D11Format and ExternalBackBufferD3D11 files. Keep the original mapping in the existing D3D11 device backend, share its query through DeviceQueries, and let DeviceImpl own back-buffer state. Replace the shader-specific DLL copier with imported DXC runtime dependencies and standard TARGET_RUNTIME_DLLS deployment, including ShaderTool and installation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48fa7ec8-0cf6-48a0-b836-10b7ace38c1a
…tate Use the thread-safe bgfx resource API on the camera update's calling thread, after Metal conversion completes. Retain only the imported Metal resource until the frame is processed; never queue wrapper access. Restore Texture's pre-mutex fields and methods and remove its deferred update token API and tests. Cover native-resource retention across wrapper disposal, destruction and replacement in the existing D3D11 tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48fa7ec8-0cf6-48a0-b836-10b7ace38c1a
Keep native Metal ownership with the Texture wrapper and release retired resources only after a full render boundary processes their destruction. Keep imports synchronous without adding wrapper locks or deferred access. Cover producer release, replacement, and disposal on D3D11 and Metal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48fa7ec8-0cf6-48a0-b836-10b7ace38c1a
Summary
Update to bgfx API 161 through merged bgfx.cmake commit
2e8efe6186d1bc311674153c8207c19774bc77a9(BabylonJS/bgfx.cmake#147), including the synchronized upstream CMake scripts and the EGL/D3D12 fixes in BabylonJS/bgfx#80.Local validation before opening this PR
Native Canvas pixel validation passes on both D3D11 and D3D12 (1/1 each). New regressions cover swap-chain resize/MSAA/depth modes, window replacement, disable/re-enable, screenshot/capture coalescing and normalization, D3D11 external-view imports, and D3D12 resource states. The debug-layer regression executes and reports zero errors.
Apple SDKs and Android device execution were unavailable locally and require CI/device coverage. Local D3D12 ModuleLoadTest reports an unexpected
cryptsp.dllmodule; that result is not counted as a pass or hidden by changing its allowlist.Latest rebase and Vulkan fix
bb01e283d83da8c043000862c76d9315f76c7a67, including the merged Canvas font packaging fix (Package Canvas text metrics test font #1881). The original three patches remain unchanged by the rebase.7248189a8b5f31cee975e7023b2eedf7da615900is green: run 35237002633 completed successfully. All 35 checks passed, includingUbuntu_Clang_Vulkan / build, with no failures or pending checks.Review follow-up
Gary's review is addressed in
6d7a68a906198e43cc280ba289eb3c9d100d0f9d:D3D11Format.handExternalBackBufferD3D11.h/.cppentirely from the PR. The existing D3D11 device implementation owns the back-buffer state and conversion code, with the shared query declared in existing internalDeviceQueries.h. The original 105-entry table is preserved verbatim.SwapChain::depthcannot represent. Supported default views and explicit color/depth framebuffer semantics remain intact. The restriction is documented in the public API.TARGET_RUNTIME_DLLSdeployment, including ShaderTool and installation. Uses the actual SDK dxcompiler import library and a runtime-only dxil dependency, not a placeholder import library.Validation: D3D11 RelWithDebInfo build and 27 Device/ExternalTexture tests passed in two shuffled runs; the new read-only rejection regression failed before the fix. D3D12 ShaderTool/ModuleLoadTest builds and real ShaderTool compilation passed. Paired DLL hashes, transitive deployment and generated installation entries were verified. CMake runtime validation used 3.31; 3.21 dependency propagation was checked in its implementation source. The previously documented local ModuleLoadTest
cryptsp.dllallowlist failure remains unchanged.Camera lifetime scope
8bf2c2203cf6c3197d2249bcf6faea02a2141181removed the broad Texture shared-state/mutex rewrite and deferred-update API. Apple camera conversion/import remains synchronous in the update call; no deferred callback accesses the C++ wrapper.bd2a08fb962d6fd1e99fce740a34f48b4baab78faddresses Gary's follow-up: the Metal backend borrows the imported object, so retaining it only until the first AfterRender was insufficient. Each import now supplies a native-resource owner held by Graphics::Texture. Camera Close/output replacement cannot release an object while the caller still owns its wrapper. On disposal/recreation, only native ownership crosses BeforeRender -> AfterRender, keeping the object alive until a full render boundary processes the queued handle destruction. No mutex or shared wrapper state is added.The replacement lifecycle regression is enabled for D3D11 and Metal and keeps the wrapper valid across multiple frames after releasing producer ownership. It covers disposal, wrapper destruction and replacement both before and from AfterRender. The old one-frame retention fails this regression; the final D3D11 build and all 28 focused Device/ExternalTexture tests pass twice. This models producer Close/replacement with native resources, not physical camera execution. Apple SDK/device validation is unavailable locally.
Current CI is green: run 35292522780 completed successfully for
bd2a08fb962d6fd1e99fce740a34f48b4baab78f; all 35 PR checks passed with none pending. All macOS/iOS build and installation jobs passed. Windows QuickJS passed all 314 validation scenes and 56 unit tests, including the new native-owner lifecycle regression. The macOS unit-test job uses the no-op Metal device and skips that native-resource regression, so Apple compilation is verified but real-Metal/physical-camera execution remains unverified. The superseded run for8bf2c220was canceled; its Windows QuickJS failure was all 314 snippets returning HTTP 403, while Windows Chakra passed all 314 scenes on the same revision. No validation exclusions, thresholds or retry rules were changed for that service failure.