On Wed, 2 Apr 2025 06:26:13 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
>> Nikita Gubarkov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> The previous approach was wrong for non-pixel interleaving.
>>
>> Just align the buffer size to the pixel stride instead, should be better.
>
> src/java.desktop/share/classes/java/awt/image/ComponentSampleModel.java line
> 285:
>
>> 283: // Align to the pixel stride.
>> 284: size = (size + pixelStride - 1) / pixelStride * pixelStride;
>> 285:
>
> Some thoughts I’d like to mention to ensure I understand the problem:
>
> This method should return the number of bytes required to store the image
> with given parameters—width, height, pixel stride, and scanline. It seems
> that the calculation could follow this approach:
>
> **starting offset + pointer to the last pixel + pixelStride**
>
> Is that a correct assumption?
>
> I have doubts that the logic in this method actually calculates the size
> properly, especially this line:
>
>
> int val = pixelStride * (width - 1);
> size += val;
>
> It seems like it should be:
>
> int val = pixelStride + pixelStride * (width - 1);
>
> or simply:
>
> int val = pixelStride * width;
>
> Or am I missing something?
As I understand, an important thing here is that this calculation needs to be
correct for any kind of pixel/scanline/band interleaving.
What if our channels are stored separately (complete image of only R followed
by complete image of G and so on) - your formula would only work if we change
"**pointer to the last pixel**" -> "**pointer to the last band of the last
pixel**". But then it would be wrong for the pixel-interleaving case (RGBA for
each pixel stored together), because adding `pixelStride=4` to the "**last band
of the last pixel**" would actually be 3 bytes more than needed.
The idea of the current approach, as I understand it is that we find the last
band of the first pixel, be it 3 for pixel-interleaved RGBA, or 30000 for
band-interleaved image, and then add 1 to it, meaning the size sufficient to
fit that last band:
int size = maxBandOff + 1;
At this point we can already fit the first pixel, so what's left is to fit the
remaining first scanline (hence `width - 1`):
int val = pixelStride * (width - 1);
size += val;
And the remaining rows (hence `height - 1`):
val = scanlineStride * (height - 1);
size += val;
### This gets us to our subject:
As current calculation only considers used bands by finding `maxBandOff`, it
doesn't really care when we have pixel interleaving and `pixelStride >
numBands`, it will happily cut any unused space at the end of the last pixel,
thus causing various problems when we just want to, say, make an RGBx
(`pixelStride=4`, `bandOffsets = {0, 1, 2}`) raster.
In my approach I just fixed that space which was cut by the previous
calculation by aligning the size up to `pixelStride`. Not that I find this
particularly elegant, but it seems to work and not cause regressions in
non-pixel interleaved variants.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24111#discussion_r2024265636
