On Fri, 25 Apr 2025 19:12:39 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
>> In your sample you provided an example, when raster fits whole pixels >> (`scanline * (height - 1) + pixelStride * width`), which, I agree, is a >> perfectly fine case and must be supported by accelerated pipelines. But the >> question is: should partial pixels be handled by our pipelines too >> (`scanline * (height - 1) + pixelStride * (width - 1) + maxBandOffset + 1` >> where `maxBandOffset + 1 < pixelStride`), or should whole pixels be >> enforced? I am for the latter. > > However, this is not a case of a "partial pixel"- the data for the pixel > itself is present. Therefore, the user can still manually create a buffer > that discards the tail of the image and it will be accepted: > > DataBuffer manualBuffer = new DataBufferByte( > scanlineStride * (SIZE - 1) + pixelStride * (SIZE - 1) + > 2/*maxBandOffset*/ + 1 /*size of last component*/ > ); > > The current logic seems to imply that: > > * The scanline stride doesn't matter for a single-line image, or for the last > line in the image. > * The pixel stride is irrelevant if there's only one pixel in the image, or > for the last pixel in a row. > > This behavior is validated in > src/java.desktop/share/classes/sun/awt/image/ByteComponentRaster.java#verify, > with the exception of the single-pixel image we mentioned earlier. > > I believe that exception for the 1-pixel image is a bug (pixelStride > > data.length) that was overlooked during the work on commit > [86104df](https://github.com/openjdk/jdk/commit/86104df#diff-f5003f18f0f4594a4859901ea950652467137fb1d07900208a84617036142746R891-L891), > where a similar check for scanlineStride > data.length was fixed. Ok, I found another argument as to why I think we should allocate memory according to `pixelStride`, including padding. I am currently optimizing the Vulkan blit and specifically reducing the amount of extra copies, so the issue is real. In order to do draw an image, we need a Vulkan image. To bring raster data into Vulkan image we are currently doing 2 copies: software raster -> VkBuffer -> VkImage. We could get rid of one copy if we imported raster memory as an external allocation and bind VkBuffer to it directly: VkBuffer(software raster) -> VkImage. In theory we could even wrap host memory as an image and use it directly, but I doubt that would be more performant. So, if we try to import raster data as an external allocation, it becomes useless for the purposes of copying or sampling, because Vulkan requires the whole memory region to be resident, it will copy `height` rows of `rowLength` bytes and there is no way it could work in that strange scenario when we have RGBx raster with 1 byte of padding in the end, mapping to RGBA Vulkan format, and there is just a single missing byte in the end of the whole raster ruining our optimization, forcing us to memcpy the whole raster into a temporary buffer, which is just 1 byte larger so that we could properly copy that into Vulkan image. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24111#discussion_r2071847084
