On Tue, 20 Oct 2026 18:34:51 GMT, Andy Goryachev <[email protected]> wrote:
>> 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? For point 2: The problem is that you can't easily detect with 100% certainty that something is a self draw or not when native buffers are involved. We can detect it relatively easily for heap buffers, as we can get the `array` of both and if equal it's a self draw. However, with native buffers (`IntBuffer` direct), there is no way to know if the source (the image you provide) isn't a slice of the larger `IntBuffer` that is your target. So our options are then: - Leave self draw unspecified, as we can't detect it with 100% certainty from Java code; this gives us the most wiggle room to fix this later -- users have several easy options to avoid it to still get deterministic behavior if they want to "self draw" - Pre-emptively copy all direct `IntBuffer` images, just in case they are overlaps -> this is wasteful as it destroys the performance of the best path (which is to use native buffers for everything) - Have native code detect the overlap (before doing any drawing) and either: - Refuse the operation (Java can then retry with a copy) -> relatively fast as the initial "try" did no copy - Have native code solve the problem by altering the draw order (kinda like how overlapping array copies work, you either copy in forward or backward direction) -> this may be hard, and would touch the native renderer code, didn't investigate (out of scope) - Have native code allocate a temporary buffer and do the copy itself -> need to deal with OOM in native code, could also be quite involved (out of scope as well IMHO) This, and the fact that I think self draw is a very niche need, is why I think it is best to leave it unspecified for now. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1969#discussion_r4331635563
