On Tue, 20 Oct 2026 03:44:32 GMT, John Hendrikx <[email protected]> wrote:
>> modules/javafx.graphics/src/main/java/com/sun/prism/sw/SWDrawingContext.java >> line 731: >> >>> 729: graphics.setPaint(prismFillPaint); >>> 730: graphics.resetPaintBounds(); >>> 731: graphics.fillRect((float)x, (float)y, (float)w, (float)h); >> >> `fillRect()` uses `PiscesRenderer.fillRect()` which will overflow with wide >> / tall images due to `<< 16`. Should it tile or explicitly call for >> width/height limitations in the constructor? > > This is a pre-existing limitation also for Canvas under the SW pipeline > (where it fails just as silently). I think this is better addressed > separately as a solution may be involved. I recall `Canvas` failing with smaller dimensions due to failed allocation, but I don't have the record. Even more, the `Canvas` is intended to be displayed on screen, so we don't expect it to be too large, right? With the DC API, we might actually expect users creating giant contexts. Should we explicitly mention the size limit in the constructor? >> modules/javafx.graphics/src/main/java/com/sun/prism/sw/SWDrawingContext.java >> line 1303: >> >>> 1301: >>> 1302: // Create a texture from the Prism image >>> 1303: Texture tex = resourceFactory.createTexture(prismImage, >>> Usage.DEFAULT, Texture.WrapMode.CLAMP_TO_EDGE); >> >> efficiency: this copies the whole image each time, even when drawing a small >> part of it. is it worth optimizing? > > I can add one small optimization if the underlying format is compatible, but > the bigger optimization I think is out of scope for this PR. It means that if > there is a performance problem, users would have a way out (use INT_ARGB > array backed images), or use smaller images instead of taking a small piece > of a super large image. > > The bigger optimization would be: > - Copy only the part we need (users can avoid by doing this before hand or > use a smaller image) > - Allow direct buffers in INT_ARGB format to also be used without copying > (requires native code changes) > > I would leave both out to not keep blowing this PR up further. Ensuring the > API is good I think is the priority, performance can be addressed later. > > One thing that did surface, and for which I will make a javadoc change, is > that since `drawImage` takes an image, the source/target could be the same > image (self drawing). As this complicates matters a lot, especially since we > can't detect this reliably with direct buffers which would then force a "just > in case" copy (not nice if you're trying to optimize performance), I will > document this as unspecified instead. If this is something users want to do, > they can either snapshot the image, or we can introduce an API specifically > intended for this (`copyRect`). Canvas has no such problems as self draw > there is only possible by making a snapshot in the first place. > > Note that Canvas avoids most of performance issues as it uses a cached > texture (something we can't easily do here as the cache for this is owned by > the render thread, while the implementation in this PR is doing drawing on > the caller thread). Canvas still makes a full copy though, so drawing a > small part of a huge image pays a memory cost you may also want to avoid. 1. I agree, an optimization can be undertaken in a follow-up. 2. Instead of "unspecified", should ie be "not supported"? throw an exception? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1969#discussion_r4328676626 PR Review Comment: https://git.openjdk.org/jfx/pull/1969#discussion_r4328759716
