On Wed, 2 Apr 2025 18:28:07 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
> please add a test case for this scenario Sure, gonna look into it. > is it possible for pixelStride to be smaller than maxBandOff? Surprisingly, yes! Band offsets are arbitrary, even for `PixelInterleavedSampleModel`, the only restriction is that `maxBandOff - minBandOff <= pixelStride` **(which is wrong btw, there should be < instead)** - [see here](https://github.com/openjdk/jdk/blob/209e72d311234c8279289172dab2cbb255e4fed9/src/java.desktop/share/classes/java/awt/image/PixelInterleavedSampleModel.java#L94). That's where the ambiguity I mentioned comes from - the code doesn't assume any specific starting offset and we cannot expect band offsets to always be in `[0, pixelStride)` range. Therefore, whenever there is an unused padding at the beginning or end of the pixel, this creates some wiggle. Let's imagine we have some weird non-zero starting offset: x x x x x R G B x R G B x R G B x x x x x Knowing that `pixelStride=4` we might split it like this: x x x x | x R G B | x R G B | x R G B | x x x x x Or like this: x x x x x | R G B x | R G B x | R G B x | x x x x Both could be correct and there doesn't seem to be any other info, which could allow us to determine this unambiguously. This "real starting pixel offset" is implicitly assumed, when working with [known types in native code](https://github.com/openjdk/jdk/blob/209e72d311234c8279289172dab2cbb255e4fed9/src/java.desktop/share/classes/sun/awt/image/BufImgSurfaceData.java#L319), but is explicitly stored nowhere. _This is all theoretical though, I have never seen a pixel-interleaved image with a non-zero starting offset._ ### Regarding the `pixelStride` and `scanlineStride` I am reading the documentation for [SurfaceDataRasInfo](https://github.com/openjdk/jdk/blob/209e72d311234c8279289172dab2cbb255e4fed9/src/java.desktop/share/native/libawt/java2d/SurfaceData.h#L71): > Valid memory locations are required at: `*(pixeltype *) (((char *)rasBase) + y * scanStride + x * pixelStride)` for each x, y pair such that (bounds.x1 <= x < bounds.x2) and (bounds.y1 <= y < bounds.y2). So it doesn't require the end of the last scanline to be resident in memory, however `pixeltype` is not defined there, so it's hard to tell whether it's assumed that `sizeof(pixeltype) == pixelStride`, or that this question was considered at all. I am using a little thought experiment to figure this out: Imagine that we want to copy image data from a given `SurfaceDataRasInfo sdri`. 1. We iterate through scanlines from 0 to `sdri.bounds.y2 - srdi.bounds.y1` 2. Our scanline may have padding in the end, which may not be resident, so we cannot copy whole `scanlineStride` - so we copy `pixelStride * (sdri.bounds.x2 - srdi.bounds.x1)` instead. 3. But what if our last pixel also contains padding, which is not resident? Then we would have to repeat the [same procedure](https://github.com/openjdk/jdk/blob/130b0cdaa6604da47a893e5425547acf3d5253f4/src/java.desktop/share/classes/java/awt/image/ComponentSampleModel.java#L247) with finding the last used component and so on. But the problem with this is that there is no such info in `SurfaceDataRasInfo`, which could allow us figure out, which components are really used and which bytes are just padding. Moreover, `SurfaceDataRasInfo` doesn't even have a total `size` field, so we can't even clamp our copy range to some boundary. This leads me to conclusion that we indeed need to enforce whole `pixelStride` to be resident, including padding, but we should not do the same with `scanlineStride`, as explicitly mentioned in the `SurfaceDataRasInfo` documentation. > the check that caused the exception - pixelStride > data.length - is too > strict This check looks weird to me because it's not clear, which rule exactly it enforces. However in its current form it was useful, because it caught an issue with something that was implicitly assumed, but not really enforced. I mean, I don't know... I would probably vote for leaving the check as is. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24111#discussion_r2025517596
