RFR: 8352407: PixelInterleavedSampleModel with unused components throws RasterFormatException: Incorrect pixel stride [v2]

[Sergey Bylokhov]

On Fri, 2 May 2025 16:14:30 GMT, Nikita Gubarkov <ngubarkov at openjdk.org> wrote:

>> 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.

But even if we start allocating raster memory with padding internally, you will still need to support both cases anyway, right? So for now, maybe it's best to implement two versions of the blit: one optimized (assuming properly padded memory) and one fallback (handling the edge case with the missing padding).

At runtime, we can select the appropriate version depending on the actual memory layout. Then we can compare performance and see whether supporting both is actually worth it.

If the performance difference is significant, we can update our internal allocation strategy to always use the optimized layout, while still supporting custom rasters (with non-standard strides) using the slower path.

Just to clarify - are we talking only about padding per pixel, or do we also need full row padding to meet Vulkan’s alignment requirements?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24111#discussion_r2072266843