On Fri, 16 Oct 2026 14:40:27 GMT, Lukasz Kostyra <[email protected]> wrote:
> ## Description > > This PR is for review and integration of Direct3D 12 Rendering Pipeline into > JavaFX mainline. We released three Early Access (EA) versions of this > implementation while keeping the development ongoing on [JavaFX > Sandbox](https://github.com/openjdk/jfx-sandbox/tree/direct3d12) and reached > a stage where it is ready to be integrated into mainline. Any further work > and potential fixes will be done on mainline. > > At the time of writing, this PR is identical to the [JavaFX Sandbox D3D12 > branch](https://github.com/openjdk/jfx-sandbox/tree/direct3d12) with only one > difference - the Sandbox defaults to D3D12 backend. This PR, however, does > *NOT* change the default rendering pipeline on Windows. D3D9 remains the > default rendering pipeline and D3D12 is optional - it can be selected by > providing `-Dprism.order=d3d12`. To verify which pipeline is actually in use > `-Dprism.verbose=true` is recommended. > > ## Details > > Below are general changes introduced by the backend: > > ### JSL changes > > - Introduced `HLSL6Backend` class which translates JSL shader code into HLSL6 > code. D3D12 introduced HLSL Shader Level 6 support and won't support existing > HLSL shaders used with D3D9, as those are compiled for unsupported Shader > Level 3. > - D3D12-specific shaders are kept in > `modules/javafx.graphics/src/main/native-prism-d3d12/hlsl6` > - Shader compilation is done with DXC instead of FXC (FXC does not support > SL6). DXC is bundled in Windows SDK which JavaFX already integrates with. > - Backend also generates C++ headers with Shader resource binding > information, used by the native-side > > ### Build changes > > - Added new build tasks for generation and compilation of HLSL6 shaders for > D3D12 > - Added tasks to compile D3D12 native backend code > > ### Prism changes > > - Added D3D12-specific Java classes (package `com.sun.prism.d3d12`) and a > Java-to-JNI layer (package `com.sun.prism.d3d12.ni`) > - Java implementation is mostly similar to D3D9 implementation, with only > some minor changes needed to accommodate slightly different presentation > behavior of the new runtime > - JNI interface is relatively high level to reduce the amount of JNI calls > for performance > - Added a native Prism backend implementation using D3D12 runtime (stored in > `modules/javafx.graphics/src/main/native-prism-d3d12`) > - Written in C++ 20 > - Uses only interfaces available via Windows API with no third-party > libraries - WinAPI, Windows COM interface system and "barebones" Direct3D 12 > API/runtime > - Native layer entry-points... First batch of comments on the java code, covering the `com.sun.prism.d3d12` package. Question: Have you tested this on a Windows Remote Desktop (RDP) session where you disconnect and later reconnect? We had bugs in this area in the D3D pipeline -- see [JDK-8239589](https://bugs.openjdk.java.net/browse/JDK-8239589). Is there anything similar to the DirectX 9 `D3DERR_DEVICEREMOVED` case? Do we need anything equivalent to the lost device check or D3DPipeline.reinitialize()? buildSrc/win.gradle line 401: > 399: ] > 400: WIN.prismD3D12.compiler = compiler > 401: WIN.prismD3D12.ccFlags = [ccFlags, "/std:c++20", > "/I${project("graphics").projectDir}/build/gensrc/d3d12-headers"].flatten() OK, especially in light of the impending change to use `std:c++20` everywhere for [JDK-8394578](https://bugs.openjdk.org/browse/JDK-8394578). Once that fix lands, we will want to remove this flag here to avoid duplication. modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Context.java line 130: > 128: > 129: @Override > 130: protected void updateTexture(int texUnit, Texture tex) { Compared to the D3D pipeline, this doesn't check wrap mode or filtering. Is that done elsewhere? modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Graphics.java line 31: > 29: import com.sun.prism.paint.Color; > 30: > 31: public class D3D12Graphics extends BaseShaderGraphics { D3DGraphics overrides `getPaintShaderTransform` and adds a 0.5 pixel transform. Is this not needed for D3D12? modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Pipeline.java line 69: > 67: isEnabled = nInit(PrismSettings.class); > 68: > 69: creator = Thread.currentThread(); Which thread do you expect this to be? The QuantumRenderer thread? And I presume that in all cases, that thread is used to initialize this class, call `D2D12Pipeline.init()`, and call `dispose()?` modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Pipeline.java line 113: > 111: private D3D12ResourceFactory createResourceFactory(int adapter, > Screen screen) { > 112: D3D12ResourceFactory factory = new > D3D12ResourceFactory(mInstance, adapter, screen); > 113: if (factory != null) { Redundant check, since `factory` can never be `null` modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Pipeline.java line 197: > 195: public boolean init() { > 196: if (!isEnabled) > 197: return false; In Java code, we don't allow a single-statement target of an `if`, `else`, `for`, etc., to be on a separate line. Either add curly braces (best) or move the `return false;` onto the same line as the `if`. modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Pipeline.java line 201: > 199: mInstance = new D3D12NativeInstance(); > 200: if (!mInstance.Init()) > 201: return false; add curly braces modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Pipeline.java line 217: > 215: public void dispose() { > 216: if (creator != Thread.currentThread()) { > 217: throw new IllegalStateException( Is this more of an assertion or are there cases where it might happen? modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Pipeline.java line 244: > 242: @Override > 243: public int getAdapterOrdinal(Screen screen) { > 244: return mInstance.getAdapterOrdinal(screen.getNativeScreen()); Can this even be called after dispose? if it is, it will throw NPE (which might be OK, if you don't expect it to ever happen). modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Pipeline.java line 300: > 298: } > 299: > 300: class D3D12ShutdownHook implements Runnable { It is an anti-pattern to have multiple top-level classes in a single java source file. I recommend making it a static nested class instead. modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Pipeline.java line 314: > 312: if (device != null) { > 313: QuantumToolkit.runInRenderThreadAndWait(() -> { > 314: device.waitUntilIdle(); If we ever hit this condition, a deadlock seems likely: `addNativeDevice` is typically called on the renderer thread. modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12RTTexture.java line 55: > 53: PixelFormat format, WrapMode > wrapMode, int aaSamples, boolean enableDirtyBBox) { > 54: D3D12NativeTexture tex = > context.getDevice().createTexture(physWidth, physHeight, format, > Usage.DEFAULT, wrapMode, aaSamples, false, true); > 55: D3D12NativeRenderTarget rt = > context.getDevice().createRenderTarget(tex, enableDirtyBBox); Is it possible in theory (not sure whether possible in practice) that one or both of these could fail. If the first succeeds, but the second fails, you might be left with an invalid texture that is partially created. The caller will return null without disposing it. Maybe consider checking here, and returning null if either creation fails, cleaning up the first if it was successful, but the second was not. modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12RTTexture.java line 77: > 75: boolean resize(int width, int height) { > 76: if (!resource.getResource().getNativeTexture().resize(width, > height)) > 77: return false; Surround with curly braces. modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Resource.java line 38: > 36: @Override > 37: public void free() { > 38: if (isValid()) { Should this check be `resource != null` instead, so this excluded freeing objects enqueued for deferred disposal? Both the D3D and MTL pipelines check the resource directly. modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Texture.java line 78: > 76: int srcscan, boolean skipFlush) { > 77: if (!resource.isValid()) { > 78: System.err.println("update cancelled, resource not valid"); Consider qualifying this with a test for `PrismSettings.verbose`. modules/javafx.graphics/src/main/java/com/sun/prism/d3d12/D3D12Texture.java line 165: > 163: public void update(MediaFrame frame, boolean skipFlush) { > 164: if (!resource.isValid()) { > 165: System.err.println("update(MediaFrame) cancelled, resource > not valid"); `PrismSettings.verbose`? modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/prism/PrRenderer.java line 71: > 69: return null; > 70: } > 71: isHW = pipe.supportsShaderModel(ShaderModel.SM3) | > pipe.supportsShaderModel(ShaderModel.SM6); Unless the use of bitwise-OR is intentional and needed (which I doubt), I recommend changing this to a logical-OR. ------------- PR Review: https://git.openjdk.org/jfx/pull/2346#pullrequestreview-5585298739 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4445754712 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4446861195 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4448446974 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4446356522 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4446487281 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4446664285 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4446666162 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4446564274 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4446577448 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4446125603 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4446979499 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4448658878 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4446730983 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4448528126 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4448931379 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4448937683 PR Review Comment: https://git.openjdk.org/jfx/pull/2346#discussion_r4325182601
