On Wed, 2 Apr 2025 13:28:53 GMT, Nikita Gubarkov <ngubar...@openjdk.org> wrote:
>>> Or maybe we should modify the code that relies on the current size? >> >> I have mixed feelings about that. It sounds logical "mathematically", so to >> speak, but in practice there are assumptions other code often relies on, >> which are so natural, that breaking them would be like shooting ourselves in >> the foot. >> >> This follows the same logic as C struct layout: unused padding is considered >> an integral part of the struct layout and its size - we can always `memcpy` >> a struct with its `sizeof` without calculating the position of the last byte >> which actually holds meaningful data. >> Following this: >> 1. `pixelStride` equals to the size of the pixel - it must always be >> resident in memory together with any padding it includes >> 2. `scanlineStride` equals to the size of the scanline - it must always be >> resident in memory together with any padding it includes >> 3. Derived from the latter: `dataSize >= height * scanlineStride` - yes, we >> could have unused space in the end of the scanline, but it is often assumed >> that we can copy the whole image with just `height * scanlineStride` without >> worrying about the last scanline ruining our day. >> >> I find this logic very natural and convenient, that's why I think we need to >> fix the size calculation and not its usage. > > Please see the related PR: https://github.com/openjdk/jdk/pull/24378 > It tries to solve the similar problem of calculating pixel-aligned starting > offset from given band offsets, so this discussion also applies there. >We definitely cannot do that because of the band-interleaved case: imagine if >there are like 10000 bytes of red samples followed by another 10000 bytes of >green samples - with pixelStride=1 and scanlineStride=100 if we take the first >component offset (0), our calculated size would only fit the red channel. That’s exactly what I’m missing right now. A concrete code example would make this clearer - please add a test case for this scenario. Also, is it possible for pixelStride to be smaller than maxBandOff? At least for PixelInterleavedSampleModel, it seems like this should be rejected. >Consider bandOffsets = {1, 2, 3} and pixelStride=4, is that a xRGB layout with >startingOffset=0, or RGBx with startingOffset=1? The answer is - we don't have >enough info as far as I'm concerned... Regarding the layout: - {0, 1, 2} with pixelStride=4: RGB pixel, then a gap - {1, 2, 3} with pixelStride=4: A gap, then the RGB pixel - {0, 2, 3} with pixelStride=4: R, then a gap, then GB And so on... So the starting offset of the first pixelStride is always 0 and then we have an offset of the first component? >earlier bandOffsets = {1, 2, 3} and pixelStride=4, just allocate 1 extra byte >at the end to prevent out-of-bounds access in any case. But you don’t need to allocate an extra byte - it should already be aligned to 4 bytes since the pixel data is positioned at the "end" of the pixelStride. >This follows the same logic as C struct layout: unused padding is considered >an integral part of the struct layout and its size - we can always memcpy a >struct with its sizeof without calculating the position of the last byte which >actually holds meaningful data. - Some images have gaps between pixels and rows where the gaps "may" contain unrelated data and should not be touched - Others, like BI.TYPE_INT_RGB, have padding where memcpy works safely. >Derived from the latter: dataSize >= height * scanlineStride - yes, we could >have unused space in the end of the scanline, but it is often assumed that we >can copy the whole image with just height * scanlineStride without worrying >about the last scanline ruining our day. The assumption that dataSize >= height * scanlineStride is not universally valid. There are three distinct cases we handle: - No gaps – One blit for the whole image. - Gaps between lines – One blit per line (looping over height). - Gaps between pixels – One blit per pixel (nested loops over width and height). This logic is already implemented in various places, such as native loops, cmm, and ogl. Even if we adjust the size calculation here, the "processing" code still needs to follow the same handling logic, since I am not sure it is possible to check that all possible implementations will have `size==height * scanlineStride`. Regardless of the decision we make, the check that caused the exception - pixelStride > data.length - is too strict. It should instead verify the actual size of the pixel without considering the gap. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24111#discussion_r2025376233
